The identity readers compiled in src/saml.c all start with //:
//samlp:Response/saml:Assertion/saml:AttributeStatement/saml:Attribute
//samlp:Response/saml:Assertion/saml:Subject/saml:NameID
//samlp:Response/saml:Assertion/saml:AuthnStatement/@SessionIndex
//samlp:Response/saml:Assertion/saml:AuthnStatement/@SessionNotOnOrAfter
//samlp:Response matches a Response at any depth, so an assertion that is a direct child of a Response nested under saml:Advice is reader-visible. That is the root cause of the signature-wrapping exposure fixed in #32. That PR closes the hole from the other side, by requiring every reader-visible assertion to be a direct child of the root Response and covered by the verified signature.
Anchoring the readers instead would remove the need for that rule:
/samlp:Response/saml:Assertion[1]/...
Two reasons to do it:
- The invariant currently lives in
saml_verified_identity_is_signed (src/sig.c), a different function from the queries it protects. A future reader added with a // prefix, or an edit to one of the four above, silently reopens the gap. XPATH_ASSERTIONS sits next to them as a comment-level guard, which is weaker than the queries being incapable of reaching a nested assertion in the first place.
saml_doc_attrs reads every match rather than the first. Scoping it to saml:Assertion[1] makes the identity and the attributes come from one assertion, which is what the Web Browser SSO profile expects.
Behavior change to weigh: attributes spread across sibling assertions would stop being aggregated. Worth checking against real IdP output before changing.
The identity readers compiled in
src/saml.call start with//://samlp:Responsematches aResponseat any depth, so an assertion that is a direct child of aResponsenested undersaml:Adviceis reader-visible. That is the root cause of the signature-wrapping exposure fixed in #32. That PR closes the hole from the other side, by requiring every reader-visible assertion to be a direct child of the rootResponseand covered by the verified signature.Anchoring the readers instead would remove the need for that rule:
Two reasons to do it:
saml_verified_identity_is_signed(src/sig.c), a different function from the queries it protects. A future reader added with a//prefix, or an edit to one of the four above, silently reopens the gap.XPATH_ASSERTIONSsits next to them as a comment-level guard, which is weaker than the queries being incapable of reaching a nested assertion in the first place.saml_doc_attrsreads every match rather than the first. Scoping it tosaml:Assertion[1]makes the identity and the attributes come from one assertion, which is what the Web Browser SSO profile expects.Behavior change to weigh: attributes spread across sibling assertions would stop being aggregated. Worth checking against real IdP output before changing.