Anchor the identity XPaths to the root Response
Nobody has claimed this yet.
Assessment
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Newbie friendliness
- 64/100
- Issue type
- Bug
- Clarity
- Mostly clear
- Activity status
- Quiet
- Tech stack
- c, xml
- Domain
- authentication, security
Research direction
Start in src/saml.c, inspect the four identity XPath readers and saml_doc_attrs, then read saml_verified_identity_is_signed in src/sig.c for the existing protection. Anchor the readers to the root Response and first Assertion, then check real IdP output to confirm that identity and attributes still come from the expected assertion without sibling aggregation.
Written by the indexing model from the issue text.
Description
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_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.
- Dominant language
- Perl
- Stars
- 2
- Forks
- 3
- Avg merge
- 2d 20h
- Merged PRs (30d)
- 3
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from api7/lua-resty-saml
-
Difficulty 2/5 1-3 hours Newbie friendliness 82/100
api7/lua-resty-saml#63 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
api7/lua-resty-saml#62 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
api7/lua-resty-saml#45 · 1 comment · 1 reaction ·
-
Difficulty 3/5 1-2 days Newbie friendliness 68/100
api7/lua-resty-saml#64 ·
-
api7/lua-resty-saml#61 · 1 assignee ·
All issues in api7/lua-resty-saml
Similar issues
-
1.severity: security
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
-
Difficulty 1/5 Under an hour Newbie friendliness 68/100
-
Difficulty 1/5 Under an hour Newbie friendliness 82/100
-
Difficulty 1/5 Under an hour Newbie friendliness 72/100
-
Difficulty 1/5 Under an hour Newbie friendliness 60/100
conda-forge/perl-test-warn-feedstock#1 · 1 comment · 1 reaction ·