-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Base 64 decoder, reject input when unused bits are not 0 #105262
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Tagging subscribers to this area: @dotnet/area-system-memory |
buyaa-n
requested review from
jeffhandley,
GrabYourPitchforks,
stephentoub and
tannergooding
July 22, 2024 17:49
stephentoub
reviewed
Jul 22, 2024
...braries/System.Private.CoreLib/src/System/Buffers/Text/Base64Helper/Base64ValidatorHelper.cs
Show resolved
Hide resolved
This was referenced Jul 23, 2024
stephentoub
reviewed
Jul 23, 2024
...libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Helper/Base64DecoderHelper.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Jul 23, 2024
...braries/System.Private.CoreLib/src/System/Buffers/Text/Base64Helper/Base64ValidatorHelper.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Jul 23, 2024
src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Url/Base64UrlValidator.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Jul 26, 2024
...libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Helper/Base64DecoderHelper.cs
Show resolved
Hide resolved
stephentoub
reviewed
Jul 26, 2024
...braries/System.Private.CoreLib/src/System/Buffers/Text/Base64Helper/Base64ValidatorHelper.cs
Outdated
Show resolved
Hide resolved
stephentoub
approved these changes
Jul 26, 2024
buyaa-n
commented
Jul 26, 2024
...libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Helper/Base64DecoderHelper.cs
Outdated
Show resolved
Hide resolved
buyaa-n
commented
Jul 26, 2024
...libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Helper/Base64DecoderHelper.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When the input is not multiple of 3 some bits of the encoded values are not used, Base64 encoder sets those bits to 0, but decoder currently doesn't check those bits and allows any combination of values. Therefore multiple input could decoded to same value, for example when the input is 1 byte character
'A'
, encoder encodes it to 2 base64 characters and 2 padding"QQ=="
, the last 4 bits of the 2ndQ
is not used and set to 0s, but decoder doesn't validate that and allows 2^4 = 16 values decoded into same value, for example: "QQ==", "QR==", "QS==", "QT==", "QV==", "QU==", "QW==", "QX==", "QY==", "QZ==", "Qa==", "Qb==", "Qc==", "Qd==", "Qe==", "Qf" will be decoded to a same value, 65, ascii of 'A'.The spec mentions that unused bits MUST be set to zero by conforming encoders. It also mentions that decoders may reject an input if pad bits have not been set to zero. We don't see any reason to keep allowing non-zero value for those other combinations that produce same result when encoders are expected to produce only one value.
This doesn't seem to be a breaking change, my quick research did not find any encoder, that produces output that doesn't set unused bits to 0. Though it could break tests that randomly generates Base64 encoded text.
Further we should fix this for
Convert.FromBase64XYZ(...)
overloadsRelated to #50233 (comment)