Limit length of base64 encoding buffers#2447
Conversation
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
| 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); |
There was a problem hiding this comment.
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.
|
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. |
I found two more cases that probably should be updated and added them to #2447 (comment) in my earlier review. |
yadij
left a comment
yadij
left a comment
There was a problem hiding this comment.
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(...));.
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 am against such a change. It is clear that base64-related code should be refactored, but |
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. |
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 |
3 participants
Check bounds before base64-encoding data into fixed-size buffers.