Skip to content

Harden Basic Auth credential buffers and verify A25 privacy - #192

Merged
stevehu merged 3 commits into
masterfrom
fix/basic-auth-credentials-191
Oct 29, 2026
Merged

stevehu merged 3 commits into
masterfrom
fix/basic-auth-credentials-191

Conversation

@stevehu

@stevehu stevehu commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Closes #191

Basic authentication created immutable decoded credential/password Strings and left comparison arrays uncleared (A20, Excel row 98). Local authentication now uses request-owned buffers cleared on exit. The existing LDAP String API and extension signature remain unchanged and are documented as a residual limit.

The scheme is removed at a character boundary, preserving Unicode case-insensitive scheme compatibility. Base64-decoded malformed UTF-8 retains the former replacement behavior for both local comparison and LDAP, using cleared character/byte buffers instead of immutable credential Strings. Basic and shared scheme checks use one trim-equivalent scanner without creating credential substrings.

A25 rows 90–97 were already mitigated in both branch baselines; the initial TRACE privacy regression passed against each unmodified implementation. No password/header logging behavior change is needed. Regression checks now include distinct password prefix/suffix fragments and the Unicode suffix, full encoded credentials, log arguments/MDC/throwables and returned status content. Configured username/path diagnostics remain.

Validation on each branch: 34 focused tests passed, zero failures/errors/skips; pre-commit hooks and whitespace checks passed. A package-local parser boundary lets tests observe the actual credential buffers used by handleBasicAuth without replacing its logic; tests verify cleanup after success, invalid/empty/reserved/unknown credentials, malformed input, denied paths, and LDAP returns/exceptions. Compatibility tests cover Unicode scheme variants and malformed UTF-8.

Negative controls independently confirmed tests fail for byte-offset scheme stripping, missing UTF-8 replacement normalization, omitted middleware try-with-resources, and password-prefix logging. Earlier controls also caught full password logging and omitted buffer cleanup. Source/test/doc changes match across branches.

mvn -o clean test -Dtest=BasicAuthMiddlewareTest,BasicAuthCredentialsTest,BasicAuthPrivacyTest,AuthorizationSchemeCredentialsTest,BasicAuthMiddlewareLifecycleTest,BasicAnonymousBearerTest,BasicBearerNoUserTest

The request header, configured password, decoder/JVM internals and LDAP library may retain credentials; this does not promise full heap erasure. No live LDAP bind, AWS operation, native-image build or deployment was performed. Fresh remote CI on the updated head is required.

Charset-buffer follow-up: UTF-8 conversion uses the incremental encoder/decoder API with explicit output arrays sized from maximum expansion, and clears their full capacity in finally blocks. This avoids output resizing and discarded earlier conversion buffers. Comparison encoding uses the same owned-buffer path. New mixed-width Unicode and worst-case malformed-byte expansion cases pass on both branches. Fresh remote CI and native builds remain unqualified.

Copilot AI balanced review requested due to automatic review settings October 29, 2026 12:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@stevehu

stevehu commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Code review (xhigh) — head 88b1490 (master)

Static review of the diff and BasicAuthMiddleware. I did not run tests. The master (#192) and 2.x (#193) diffs are identical apart from where the README line sits, so this applies to both. Five findings, most severe first.

What looks right: status codes and decision order are unchanged, raw username:password and lenient Base64 are preserved, comparison still uses MessageDigest.isEqual, the new hasCredentials scan is equivalent to the old trim() check, and parse() hands ownership to the closeable correctly on both the normal and constructor-failure paths.

  1. Byte offset instead of character offset. parse() skips the scheme with a UTF-8 byte offset of 6, but AuthorizationScheme.parse() matches the scheme by character with regionMatches(ignoreCase). Basıc user:pass (dotless i) passes as BASIC. The old auth.substring(6) gave user:pass. The new code starts at the space, so the username becomes " user" and the request gets a 401. I confirmed this with a standalone snippet. It fails closed, so it is a compatibility regression, not a bypass. Base64 payloads are unaffected. Fix by deriving the offset from the scheme's character length, or by tightening parse() to ASCII.
  2. Cleanup is not tested through the middleware. The buffer-clearing tests use BasicAuthCredentials alone. Nothing observes buffers from a real handleBasicAuth call, so dropping the try-with-resources would still pass every test.
  3. Weak canary check. BasicAuthPrivacyTest looks only for the substring secret-canary and the encoded forms. A log line that printed a password prefix (lambda) or just the trailing ü would pass.
  4. Invalid UTF-8 now compares differently. The comparison uses raw bytes instead of a decode and re-encode round trip. That is stricter, but it is documented only as "valid UTF-8" and no test pins it.
  5. Duplicated whitespace scan. The trim-equivalent loop is written in both execute() and AuthorizationScheme.hasCredentials().

Remote CI and native builds are unqualified, as stated in the PR.

@stevehu

stevehu commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up review (xhigh) — head 65b64df (master)

All five earlier findings are fixed. Verified by reading the code; I ran the Basic-auth and scheme test classes on master (26 tests, all passed).

  • Scheme offset: parse() wraps auth from character index 6, matching the old substring(6). Tests cover Basic, bAsIc, dotless i, dotted I and long s.
  • Invalid UTF-8: the Base64 path decodes and re-encodes the bytes, so the historical replacement behaviour is preserved. A U+FFFD fixture pins it for local comparison and LDAP.
  • Cleanup through the middleware: the parseCredentials() seam lets BasicAuthMiddlewareLifecycleTest observe the buffers used by the real handler on every authentication and authorization return, and when LDAP throws.
  • Canary check: now rejects prefix and suffix fragments, with a test showing it does.
  • Duplicate scan: both callers use AuthorizationScheme.hasTrimmedContent.

One new minor point, not blocking: UTF_8.encode and UTF_8.decode grow their output buffers internally for non-ASCII input and drop the smaller earlier buffer without zeroing it. ASCII-only credentials never grow, and the final buffers are zeroed. Pre-sizing a CharsetEncoder buffer would avoid it, or the docs can name it as a residual.

Fresh CI and native builds are still unqualified, as stated in the PR.

@stevehu

stevehu commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up review (xhigh) — head 6d3b1ff (master)

The charset-buffer finding is fixed. No new findings.

Encoding and decoding now use a CharsetEncoder/CharsetDecoder with explicitly sized output arrays (maxBytesPerChar and maxCharsPerByte), so no buffer is resized and dropped. The temporary arrays are zeroed in finally, and only an exactly sized result is returned. matches() uses the same helper for the configured password. Malformed input is still replaced, and lone surrogates still become ?, as getBytes did.

I ran the Basic-auth and scheme test classes on master (28 tests, all passed). Fresh CI and native builds are still unqualified, as stated in the PR.

@stevehu
stevehu merged commit 68fc849 into master Oct 29, 2026
1 check passed
@stevehu
stevehu deleted the fix/basic-auth-credentials-191 branch October 29, 2026 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harden Basic Auth credential buffer ownership (A20); verify A25 logging mitigation

2 participants