Conversation
The four tests named for purpose and subject binding asserted nothing about binding. Each called Issue once per purpose or subject, so the two tokens were independent CSPRNG draws and the hashes differed because the tokens differed. Deleting writeField(mac, purpose) from computeHash left all four green. The control was defended, but by accident: the sabotage failed TestComputeHash_LengthPrefixSurvivesEmbeddedNULs and one seed of FuzzVerify, two tests about NUL handling that happen to hold purpose and subject fixed. Narrowing either would have removed the only real coverage with every test still named "binding" staying green. The Issue tests now compare computeHash directly with the token held fixed, so exactly one field varies. The Verify tests are written as the attack they are about: one real credential replayed into another flow, same token and same stored hash, only the purpose or the subject different. Presenting a different token against a different hash, which is what they did, fails on the token alone. Each Verify test also asserts the credential verifies under its own purpose and subject first, so a Verify that rejected everything cannot pass them. Measured after: dropping purpose fails the two purpose tests and leaves the subject tests green, and dropping subject does the reverse. Each test is now about the field it is named after. Also removed a stray `_ = time.Now()` and its import. Closes #358 Signed-off-by: Jose <75870284+Jaro-c@users.noreply.github.com>
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.
The four tests named for purpose and subject binding asserted nothing about binding. Each called
Issueonce per purpose or subject, so the two tokens were independent CSPRNG draws and the hashes differed because the tokens differed. DeletingwriteField(mac, purpose)fromcomputeHashleft all four green.TestIssue_DifferentPurposesDifferentHashesTestIssue_DifferentSubjectsDifferentHashesTestVerify_WrongPurposeRejectedTestVerify_WrongSubjectRejectedThe control was defended, but by accident. The sabotage failed
TestComputeHash_LengthPrefixSurvivesEmbeddedNULsand one seed ofFuzzVerify, two tests about NUL handling and length prefixing that happen to hold purpose and subject fixed. Narrowing either one would have removed the only real coverage while every test still carrying the word "binding" stayed green.What changed
The
Issuetests now comparecomputeHashdirectly with the token held fixed, so exactly one field varies and a pass is evidence about that field.The
Verifytests are written as the attack they are named for: one real credential replayed into another flow. Same token, same stored hash, only the purpose or the subject different. Presenting a different token against a different hash, which is what they did, fails on the token alone and proves nothing.Each
Verifytest now also asserts the credential verifies under its own purpose and subject before asserting the cross-flow rejection. Without that, aVerifythat rejected everything would pass.Measured after
Dropping
purposefromcomputeHashfails the two purpose tests and leaves the subject tests green. Droppingsubjectdoes the reverse. Each test is now about the field it is named after, and the sabotages compiled in both directions.Also removed a stray
_ = time.Now()and its now-unused import.Provenance
Found by a MiniMax-M3 pass over the package. It predicted both halves before anything was run: that all four tests would stay green, and that the NUL tests would catch the sabotage incidentally. Both held.
Closes #358