Fix signature validation bypass via KeyInfo injection#658
Closed
aashh wants to merge 2 commits intocrewjam:mainfrom
Closed
Fix signature validation bypass via KeyInfo injection#658aashh wants to merge 2 commits intocrewjam:mainfrom
aashh wants to merge 2 commits intocrewjam:mainfrom
Conversation
Previously KeyInfo was only stripped when X509Certificate was absent, leaving a bypass if an attacker included a dummy cert. Now KeyInfo is unconditionally removed and each trusted certificate from IdP metadata is tried individually, since goxmldsig only auto-selects from the certificate store when there is exactly one root.
Author
|
Closing this PR — the cert loop had a variable scoping bug that broke XSW detection tests. I also overestimated the security impact; |
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.
Signature validation conditionally strippedKeyInfofrom the response signature: only when noX509Certificatewas present. This left a bypass where an attacker could inject their own certificate in the signature's KeyInfo element.Additionally, when an IdP has multiple signing certificates in its metadata, goxmldsig's automatic cert selection only works when there's exactly one root certificate in the store. With multiple certs, validation would fail even for valid signatures.Fix:Always stripKeyInfobefore validation to prevent attacker-controlled certificates from being usedTry each metadata cert individually by adding them one at a time to the validation store, since goxmldsig can only auto-select when there's one rootChanges:service_provider.go: Unconditionally removeKeyInfo, loop over IdP certsservice_provider_test.go: AddKeyInfoinjection test and multi-cert IdP testTesting:
KeyInfoinjection attack (attacker cert in signature): rejectedMulti-cert IdP metadata: valid signature acceptedSingle-cert IdP (existing behavior): still worksAll existing tests passFixes aashh#10