Limit length of base64 encoding buffers by kinkie · Pull Request #2447 · squid-cache/squid

kinkie · 2026-06-20T08:46:56Z

Check bounds before base64-encoding data into fixed-size buffers.

Adds testExtAclBase64Overflow, a CppUnit test suite that confirms the stack buffer overflow present in three production sites: src/adaptation/icap/ModXact.cc makeRequestHeaders() (site A) src/http.cc httpFixupAuthentication() PASS/PROXYPASS (site B) src/http.cc httpFixupAuthentication() '*'-prefix path (site C) All three allocate a 175-byte stack buffer (base64_encode_len(MAX_LOGIN_SZ)) then pass unchecked extacl_user/extacl_passwd sizes to base64_encode_update(). When a helper returns values totalling >= 130 bytes, the encoded output (176 bytes) overflows the buffer. The tests replicate each encoding pattern using an oversized scratch buffer to count emitted bytes without triggering undefined behaviour, then assert that the byte count exceeds the production allocation. Ten cases are covered: buffer-size anchor, safe/boundary/overflow inputs for sites A&B, the exact 130-byte overflow threshold, large-input confirmation, and site C's two-segment (user + peer_login_suffix) path.

When extacl_user and extacl_passwd are set by an external ACL helper, makeRequestHeaders() encodes them as a Basic Proxy-Authorization credential into a fixed 175-byte stack buffer: char base64buf[base64_encode_len(MAX_LOGIN_SZ)]; // 175 bytes The encoding calls previously passed extacl_user.size() and extacl_passwd.size() directly to base64_encode_update() without checking that the combined plain-text length fits within MAX_LOGIN_SZ (128 bytes). A helper returning values totalling >= 130 bytes causes the encoded output (>= 176 bytes) to overflow the 175-byte stack buffer. Fix: add a pre-encoding guard that throws a TextException when plainLen (user + ':' + passwd) exceeds MAX_LOGIN_SZ. The exception propagates through the existing JobDialer catch, calls ModXact::callException(), tears down the ICAP transaction cleanly, and returns an error answer to the HTTP client. The guard uses the same throw-on-violation pattern already used elsewhere in ModXact.cc. The guard is deliberately conservative (fires at plainLen > 128, i.e., >= 129) to ensure the 3-byte base64_encode_final padding slot is always available within the buffer. Update testExtAclBase64Overflow to document the guard's threshold: - testGuardAllowsExactLimit: plainLen=128 must not fire the guard - testGuardRejectsOneByteOver: plainLen=129 triggers the guard (conservative; actual encoding overflow starts at plainLen=130)

httpFixupAuthentication() shares a single 175-byte stack buffer (loginbuf[base64_encode_len(MAX_LOGIN_SZ)]) across three encoding branches, none of which previously checked that the plain-text input fit within MAX_LOGIN_SZ (128 bytes) before writing. Affected branches and their plain-text inputs: Site B — PASS/PROXYPASS: extacl_user + ':' + extacl_passwd Both fields are helper-controlled; identical overflow to site A. Guard: if (userLen + 1 + passwdLen > MAX_LOGIN_SZ) throw Site C — '*'-prefix: extacl_user + peer_login suffix after '*' Username is helper-controlled; suffix is operator-controlled. Guard: if (usernameLen + suffixLen > MAX_LOGIN_SZ) throw Fallback — plain peer_login literal Entirely operator-controlled, but uses the same buffer. Guard: if (loginLen > MAX_LOGIN_SZ) throw All three guards use throw TexcHere(...), which propagates through JobDialer::dial()'s catch to AsyncJob::callException() -> mustStop("exception"), tearing the HTTP-to-peer connection down cleanly and returning an error to the client. This is the same pattern used in the site A fix (ModXact.cc) and throughout http.cc. The guards are deliberately conservative (fire at > MAX_LOGIN_SZ, i.e. >= 129) matching the site A fix, ensuring the 3-byte base64_encode_final padding slot is always available. Extend testExtAclBase64Overflow with four new cases (16 total): testSiteB_GuardAllowsExactLimit — plainLen=128 does not fire testSiteB_GuardRejectsOneByteOver — plainLen=129 fires (conservative) testSiteC_GuardAllowsExactLimit — username+suffix=128 does not fire testSiteC_GuardRejectsOneByteOver — username+suffix=129 fires

rousskov

I support this temporary and ugly band-aid because a proper long-term solution would require significant changes (that may not be appropriate for backporting). Thank you for posting this PR. I only have two minor polishing requests.

rousskov · 2026-06-20T14:33:02Z

+        throw TexcHere("peer_login too long");
+    blen = base64_encode_update(&ctx, loginbuf, loginLen, reinterpret_cast<const uint8_t*>(request->peer_login));
    blen += base64_encode_final(&ctx, loginbuf+blen);
    httpHeaderPutStrf(hdr_out, header, "Basic %.*s", (int)blen, loginbuf);


Please add Assure(request->url.userInfo().length() < MAX_URL*2) or a similar assertion to HttpStateData::httpBuildRequestHeader().

Please also adjust SSP_MakeChallenge() and peer_proxy_negotiate_auth() cases.

Good spot, however those cases should be restricted by MAX_AUTHTOKEN_SZ which is the SSPI buffer size for encoded credential tokens.

So Assure(base64_encode_len(loginLen) <= MAX_AUTHTOKEN_LEN);

Good spot, however those cases should be restricted by MAX_AUTHTOKEN_SZ which is the SSPI buffer size for encoded credential tokens.

I do not recommend changing the size of the existing assembly buffer in this PR. The current size is MAX_URL*2, not MAX_AUTHTOKEN_SZ (which is presumably a typo for MAX_AUTHTOKEN_LEN).

If you do increase that size despite my recommendation, please update or remove the corresponding "should be big enough for a single URI segment" comment as well: MAX_AUTHTOKEN_LEN is much bigger than MAX_URL or even MAX_URL*2, so the comment would become even more wrong/misleading (and you become responsible for updating or removing it if you change the size).

Please also adjust SSP_MakeChallenge

There's a problem here: SSP_MakeChallenge is in lib/ and Assure in src/ , and from what I remember (and see) lib/ is not supposed to depend on src/ .
I think in this case, the best option is to make the overflow impossible by expanding the output buffer: the maximum size of a NTLM packet is cbMaxToken, which is at most 64k. After applying the 4/3 expansion by base64 encoding, it's still not too bad.

Right now, an overflow would trigger an assertion.
What do you think @rousskov , @yadij ?

To assert in code that should assert but is not ready for (or should not use) Assure(), use assert().

The primary goal of this PR is to make this bad code safe(r). The exact mechanism for maintaining (including asserting) key invariants may vary, of course.

Thank you for adjusting HttpStateData::httpBuildRequestHeader() and peer_proxy_negotiate_auth() cases. AFAIK, only SSP_MakeChallenge() is left now, as far as this change request is concerned.

rousskov · 2026-06-20T15:47:10Z

I polished PR description a little, primarily to avoid promising to abort the transaction. I agree that the corresponding transaction is likely to be aborted in most or even all of these cases, but this fix does not need on that specific outcome, so it is best not to mention it, to avoid a false implication that the fix does rely on that outcome.

rousskov · 2026-06-20T16:19:39Z

I only have two minor polishing requests.

I found two more cases that probably should be updated and added them to #2447 (comment) in my earlier review.

yadij

The MAX_LOGIN_SZ is an artifact of the old Squid char* implementation. The protocol sets no limitation, and some use-cases are already pushing at the value hard-coded by Squid.

I believe we should swap all these char[] buffers to SBuf and use the pointer from auto loginbuf = rawAppendStart(base64_encode_len(...));.

rousskov · 2026-06-21T14:47:22Z

The MAX_LOGIN_SZ is an artifact of the old Squid char* implementation. The protocol sets no limitation, and some use-cases are already pushing at the value hard-coded by Squid.

IMO, the above facts do not imply that this PR has to implement the changes requested below. This PR can and, IMO, should continue to limit supported input lengths in the affected cases.

I believe we should swap all these char[] buffers to SBuf and use the pointer from auto loginbuf = rawAppendStart(base64_encode_len(...));.

I am against such a change. It is clear that base64-related code should be refactored, but rawAppendStart() is not the right long-term solution, and this PR should not be used for implementing the right long-term solution. @yadij, please do not block this PR to request adding rawAppendStart() to more base64 callers.

kinkie · 2026-06-21T19:57:28Z

The MAX_LOGIN_SZ is an artifact of the old Squid char* implementation. The protocol sets no limitation, and some use-cases are already pushing at the value hard-coded by Squid.

IMO, the above facts do not imply that this PR has to implement the changes requested below. This PR can and, IMO, should continue to limit supported input lengths in the affected cases.

I believe we should swap all these char[] buffers to SBuf and use the pointer from auto loginbuf = rawAppendStart(base64_encode_len(...));.
I am against such a change. It is clear that base64-related code should be refactored, but rawAppendStart() is not the right long-term solution, and this PR should not be used for implementing the right long-term solution. @yadij, please do not block this PR to request adding rawAppendStart() to more base64 callers.

IMO the ideal long term solution (out of scope with this PR) would be to have a Base64Encoder class, implementing istream and ostream interfaces, using a SBuf as backing store, and abstracting this all away.
But that's not for this PR: this is purely mean to cover a current bug as quickly as possible.
In other words, I agree with @rousskov , considering development verlocity as a key goal here.

kinkie · 2026-06-21T20:24:02Z

MAX_LOGIN_SZ

I don't have any problem in making this bigger if you think it's useful, but I'd do as a separate PR to avoid scope creep - that's a separate problem to address than what is being done here

rousskov · 2026-06-22T00:34:59Z

IMO the ideal long term solution (out of scope with this PR) would be to have a Base64Encoder class, implementing istream and ostream interfaces, using a SBuf as backing store, and abstracting this all away. But that's not for this PR: this is purely mean to cover a current bug as quickly as possible. In other words, I agree with @rousskov , considering development verlocity as a key goal here.

FWIW, I disagree regarding "istream and ostream interfaces" part, but I agree with everything else, and that future interface disagreement itself is outside this PR scope (although it can be used as an illustration why this PR does the right thing by avoiding refactoring). Divide and conquer!

yadij

My request resolves all the controversy over appropriate size to allocate, placing writable buffers on the stack, and all the other issues related to char buffers - that we have long tried to solve in Squid.

This should not be a controversial change. Below are the two trivial changes being requested - repeat the same for the other buffers.

yadij · 2026-06-22T06:36:22Z

-        resultLen += base64_encode_update(&ctx, base64buf+resultLen, request->extacl_passwd.size(), reinterpret_cast<const uint8_t*>(request->extacl_passwd.rawBuf()));
+        resultLen += base64_encode_update(&ctx, base64buf+resultLen, passwdLen, reinterpret_cast<const uint8_t*>(request->extacl_passwd.rawBuf()));
        resultLen += base64_encode_final(&ctx, base64buf+resultLen);
        buf.appendf("Proxy-Authorization: Basic %.*s\r\n", (int)resultLen, base64buf);


Suggested change

buf.appendf("Proxy-Authorization: Basic %.*s\r\n", (int)resultLen, base64buf);

encoded.rawAppendFinish(base64buf, resultLen);

buf.appendf("Proxy-Authorization: Basic " SQUIDSBUFPH "\r\n", SQUIDSBUFPRINT(encoded));

rousskov · 2026-06-22T13:00:31Z

My request resolves all the controversy over appropriate size to allocate, placing writable buffers on the stack, and all the other issues related to char buffers - that we have long tried to solve in Squid.

Your request does not resolve all those problems and adds risky/dangerous code. Different code is needed to properly address the problem that triggered this PR.

This should not be a controversial change.

Out-of-scope changes to a surgical bug-fixing PR are controversial.

yadij · 2026-06-26T09:54:58Z

My request resolves all the controversy over appropriate size to allocate, placing writable buffers on the stack, and all the other issues related to char buffers - that we have long tried to solve in Squid.

Your request does not resolve all those problems and adds risky/dangerous code.

Which issues does it not resolve and why?

Different code is needed to properly address the problem that triggered this PR.

This should not be a controversial change.

Out-of-scope changes to a surgical bug-fixing PR are controversial.

Huh? this is comparison to two surgical approaches.

Old behaviour; crash on 128 byte inputs.
New behaviour; throw exception for over 128 byte input
Requested alternative; accept anything and throw exception on over 2 MB inputs.

rousskov · 2026-06-26T15:49:00Z

+        throw TexcHere("peer_login too long");
+    blen = base64_encode_update(&ctx, loginbuf, loginLen, reinterpret_cast<const uint8_t*>(request->peer_login));
    blen += base64_encode_final(&ctx, loginbuf+blen);
    httpHeaderPutStrf(hdr_out, header, "Basic %.*s", (int)blen, loginbuf);


Good spot, however those cases should be restricted by MAX_AUTHTOKEN_SZ which is the SSPI buffer size for encoded credential tokens.

I do not recommend changing the size of the existing assembly buffer in this PR. The current size is MAX_URL*2, not MAX_AUTHTOKEN_SZ (which is presumably a typo for MAX_AUTHTOKEN_LEN).

If you do increase that size despite my recommendation, please update or remove the corresponding "should be big enough for a single URI segment" comment as well: MAX_AUTHTOKEN_LEN is much bigger than MAX_URL or even MAX_URL*2, so the comment would become even more wrong/misleading (and you become responsible for updating or removing it if you change the size).

rousskov · 2026-06-26T16:21:16Z

My request resolves all the controversy over appropriate size to allocate, placing writable buffers on the stack, and all the other issues related to char buffers - that we have long tried to solve in Squid.

Your request does not resolve all those problems and adds risky/dangerous code.

Which issues does it not resolve and why?

For example, it does not remove the problematic need to manually keep two (or more) sizes in sync: The allocated buffer size and the size of element(s) added to the buffer. A trivial code refactoring change can re-introduce the same kind of problem. As for "why", the answer is "because it is not possible to fully fix this API problem with caller code changes alone".

Different code is needed to properly address the problem that triggered this PR.

This should not be a controversial change.

Out-of-scope changes to a surgical bug-fixing PR are controversial.

Huh? this is comparison to two surgical approaches.

Old behaviour; crash on 128 byte inputs.

New behaviour; throw exception for over 128 byte input

Requested alternative; accept anything and throw exception on over 2 MB inputs.

The above summary does not include other negative side effects of the proposed out-of-scope changes, but it already makes clear that the proposed introduction of rawAppendStart() calls may have an impact far beyond fixing the bug in question. For example, it may feed much larger strings to other code that may or may not be prepared to handle them. That complication/change is unnecessary and, hence, unwelcome, in this surgical PR.

And even if you are convinced that your approach is "surgical enough", you still do not have enough ammunition to insist on these changes. Current PR approach is valid/acceptable/smaller/simpler/etc. Francesco should be allowed to use it.

P.S. SBuf::maxSize is approximately 256 MB, not 2 MB.

Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>

kinkie · 2026-07-07T20:48:47Z

The MAX_LOGIN_SZ is an artifact of the old Squid char* implementation. The protocol sets no limitation, and some use-cases are already pushing at the value hard-coded by Squid.

I believe we should swap all these char[] buffers to SBuf and use the pointer from auto loginbuf = rawAppendStart(base64_encode_len(...));.

I don't want to do that here. PR #2452 will eventually implement a "auto s=Base64Encode(Sbuf)" and this will make all memory safety problems in calling code go away

rousskov · 2026-07-08T13:01:02Z

+        throw TexcHere("peer_login too long");
+    blen = base64_encode_update(&ctx, loginbuf, loginLen, reinterpret_cast<const uint8_t*>(request->peer_login));
    blen += base64_encode_final(&ctx, loginbuf+blen);
    httpHeaderPutStrf(hdr_out, header, "Basic %.*s", (int)blen, loginbuf);


To assert in code that should assert but is not ready for (or should not use) Assure(), use assert().

The primary goal of this PR is to make this bad code safe(r). The exact mechanism for maintaining (including asserting) key invariants may vary, of course.

Thank you for adjusting HttpStateData::httpBuildRequestHeader() and peer_proxy_negotiate_auth() cases. AFAIK, only SSP_MakeChallenge() is left now, as far as this change request is concerned.

rousskov · 2026-07-08T13:23:28Z

    const HttpHeaderEntry *e = nullptr;
    HttpHeaderPos pos = HttpHeaderInitPos;
    assert (hdr_out->owner == hoRequest);
+    Assure(request->url.userInfo().length() < MAX_URL*2);


Please move this assertion much lower, placing it immediately above the base64_encode_update() call.

Rule of thumb: Assert where the invariant is used.

rousskov · 2026-07-08T13:24:50Z

 {
    /* building buffer for complex strings */
-#define BBUF_SZ (MAX_URL+32)
+    constexpr auto BBUF_SZ = MAX_URL + 32;


Please revert branch commit c037cd4 or justify that surprising and controversial change.

Suggested change

constexpr auto BBUF_SZ = MAX_URL + 32;

#define BBUF_SZ (MAX_URL+32)

rousskov · 2026-07-08T13:27:48Z

        static char b64buf[8192]; // XXX: 8KB only because base64_encode_bin() used to.
        struct base64_encode_ctx ctx;
        base64_encode_init(&ctx);
+        Assure(output_token.length < sizeof(b64buf));


This PR-added assertion is wrong because, in this special case, b64buf is not defined using base64_encode_len(). I recommend keeping the existing problematic buffer definition in this surgical PR and just fixing the PR-added assertion:

Suggested change

Assure(output_token.length < sizeof(b64buf));

Assure(base64_encode_len(output_token.length) < sizeof(b64buf));

If you insist on "improving" the definition, here is an untested sketch:

const auto output_token_limit = 8192; // XXX: 8KB only because base64_encode_bin() used to. static char b64buf[base64_encode_len(output_token_limit)]; ... Assure(output_token.length < output_token_limit); size_t blen = base64_encode_update(&ctx, b64buf, output_token.length, reinterpret_cast<const uint8_t*>(output_token.value));

N.B. Here and elsewhere in this PR, I assume that base64_encode_len() provides space for a 0-terminator (i.e. ASCII NUL character), even though

base64_encode_final() itself does not add NUL;

most base64_encode_final() callers add NUL;

many base64_encode_len() callers add an extra 1 when allocating the output buffer!

A yet another base64 encoding API problem that a followup PR should address...

kinkie added 6 commits June 15, 2026 23:45

Merge remote-tracking branch 'upstream/master' into fix-base64encode

1c410dc

Apply aaa

f6c053e

Remove test harness

ef95154

rousskov requested changes Jun 20, 2026

View reviewed changes

Use TextException

d7fa149

yadij requested changes Jun 21, 2026

View reviewed changes

rousskov requested a review from yadij June 21, 2026 14:47

yadij requested changes Jun 22, 2026

View reviewed changes

rousskov reviewed Jun 26, 2026

View reviewed changes

kinkie mentioned this pull request Jun 27, 2026

[DISCUSS] Prototype a Base64Encoder #2452

Draft

squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Jun 30, 2026

kinkie and others added 2 commits July 5, 2026 23:25

Update src/adaptation/icap/ModXact.cc

5044dd7

Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>

Merge branch 'master' into fix-base64encode

9c81411

kinkie added 2 commits July 7, 2026 22:05

define -> constexpr auto

c037cd4

Add field size checks

8503055

rousskov requested changes Jul 8, 2026

View reviewed changes

	buf.appendf("Proxy-Authorization: Basic %.*s\r\n", (int)resultLen, base64buf);
	encoded.rawAppendFinish(base64buf, resultLen);
	buf.appendf("Proxy-Authorization: Basic " SQUIDSBUFPH "\r\n", SQUIDSBUFPRINT(encoded));

	constexpr auto BBUF_SZ = MAX_URL + 32;
	#define BBUF_SZ (MAX_URL+32)

	Assure(output_token.length < sizeof(b64buf));
	Assure(base64_encode_len(output_token.length) < sizeof(b64buf));

Uh oh!

Conversation

kinkie commented Jun 20, 2026 • edited by rousskov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rousskov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rousskov Jun 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadij Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

kinkie Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov commented Jun 20, 2026

Uh oh!

rousskov commented Jun 20, 2026

Uh oh!

yadij left a comment

Choose a reason for hiding this comment

Uh oh!

rousskov commented Jun 21, 2026

Uh oh!

kinkie commented Jun 21, 2026

Uh oh!

kinkie commented Jun 21, 2026

Uh oh!

rousskov commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yadij left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yadij Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

rousskov commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yadij commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rousskov Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rousskov commented Jun 26, 2026

Uh oh!

kinkie commented Jul 7, 2026

Uh oh!

rousskov Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

rousskov Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

rousskov Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

kinkie commented Jun 20, 2026 •

edited by rousskov

Loading

rousskov left a comment •

edited

Loading

rousskov Jun 20, 2026 •

edited

Loading

yadij Jun 26, 2026 •

edited

Loading

kinkie Jul 7, 2026 •

edited

Loading

rousskov Jul 8, 2026 •

edited

Loading

rousskov commented Jun 22, 2026 •

edited

Loading

rousskov commented Jun 22, 2026 •

edited

Loading

yadij commented Jun 26, 2026 •

edited

Loading

rousskov Jul 8, 2026 •

edited

Loading