[Uid] Fix the weak-secret guard in Uuid47Transformer for secrets longer than 16 bytes - #65105
Merged
Merged
Conversation
…er than 16 bytes
The guard compares the secret against str_repeat($secret[0], 16), so it only
ever matches a 16-byte secret. A secret made of identical bytes but longer
than that, e.g. str_repeat("\x00", 32), passes as if it were sound.
Compare against the actual length instead.
Contributor
Author
|
A note on the red Fabbot check, since it is not caused by this PR. Fabbot asks to rewrite six pre-existing lines in PHP does not allow assign-op on a string offset, and I opened #65106 to remove the pattern the rule trips on. Once that lands I will rebase this one and the check should go green. Happy to reorder or combine them if you prefer. |
Member
|
Thank you @semx. |
Merged
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Uuid47Transformerrejects a secret made of identical bytes, but the check compares againststr_repeat($secret[0], 16), so it can only ever match a secret of exactly 16 bytes. One byte longer and the same secret passes:A secret longer than 16 bytes is then folded through
sha256, so the derived key looks perfectly random while carrying no entropy at all. The caller gets no hint that the guard did not apply to them.The existing
provideWeakKeysdata set states what the guard is meant to catch -- all NUL, all0xFF, all'a'-- but every case in it is exactly 16 bytes, which is why this went unnoticed. This PR adds the same three shapes at 17, 32 and 64 bytes: they fail on 8.1 and pass with the fix.What this is not: the secret is still secret, and
Uuid47Transformerdocuments itself as timestamp obfuscation rather than authenticated encryption, so nothing here is remotely exploitable by an attacker. It is the guard that is inconsistent -- the very same weak secret is refused at 16 bytes and accepted at 17 -- so I am sending it as an ordinary bug fix rather than through the security channel.