Reject non-canonical trailing bits when decoding in BaseEncoding

[arshsmith1]

The base64/base32/base32hex decoders discard the low-order bits of the final character of a group instead of requiring them to be zero, so many distinct strings decode to the same bytes: base64().decode("AA=="), "AB==" and "AC==" all return {0x00}, "QE==".."QH==" all return {0x41}, and base32().decode("MZXW7===") equals base32().decode("MZXW6===") ("foo"). RFC 4648 section 3.5 says a conforming decoder should reject these non-zero padding bits; leaving them accepted is an encoding-malleability hole for code that decodes an attacker-supplied token/MAC/signature and relies on the decoded bytes being unique.

The check is added at every decode entry point so they stay consistent: StandardBaseEncoding.decodeTo and canDecode, the Base64Encoding.decodeTo override, and the shared decodingStream. Canonical input is unaffected (encode/decode round-trips and the RFC 4648 test vectors are unchanged); only inputs whose final character carries set bits that no decoded byte consumes are now rejected with a DecodingException.

Verified with the added BaseEncodingTest cases and the existing suite on both the guava and android source trees.

[gemini-code-assist]

Code Review

This pull request updates BaseEncoding to strictly enforce RFC 4648 section 3.5 by ensuring that any unused bits in the final character of an encoded input are zero. If unused bits are set, a DecodingException is thrown during decoding, and canDecode returns false. Comprehensive tests have been added to verify this behavior across Base64, Base64Url, Base32, and Base32Hex encodings. There are no review comments, so I have no feedback to provide.

[cpovirk]

Interesting.

My impression is that BaseEncoding follows java.util.Base64 here:

Base64.getDecoder().decode("Zg==") // => "f" in UTF-8
Base64.getDecoder().decode("Zh==") // => "f" in UTF-8

That's potentially important not just in the sense that we can point to the JDK and say "We're not any worse than they are!" but also in the sense that it can be risky for different systems to disagree on validity. (Plus, it's nice if BaseEncoding users can migrate to Base64 with minimal worries.)

I think that we have also more generally given up on requiring canonical forms, given that BaseEncoding.base64().decode(s) accepts any number of trailing = characters as padding already. Now, it's possible to opt out with omitPadding(). And it's possible that the better configuration option for us to offer would have been "canonicalEncoding()," which could have implemented the stricter behavior that RFC 4648 describes—albeit only as something that decoders "MAY" do or other specifications may require.

But I'm not sure that a behavior change in place would improve security or behavior more broadly, especially if it invalidates existing users' tests (as seems likely, though I'll check). Given that, my guess is that we won't do this, but I'm open to further arguments: This is not my area of expertise.

[cpovirk]

Failures from this change actually look remarkably widespread, or maybe I just got unlucky with the sample that I tested against. (That still wouldn't mean that we couldn't make the change if we were to come to believe that there's a security gain to be had.)

[arshsmith1]

That's fair. I don't have a stronger security argument than what's in the description. The malleability only bites callers who compare or key on the encoded string rather than the decoded bytes, and since java.util.Base64 accepts the same inputs, rejecting them here mostly creates a disagreement between two decoders people expect to behave alike. Given the breakage you're seeing, I agree an in-place change isn't worth it.

If an opt-in strict mode along the lines of the canonicalEncoding() idea would ever be welcome, I'm happy to rework this into that. Otherwise feel free to close it.