api7 / api7/lua-resty-saml

Anchor the identity XPaths to the root Response

Open
#34 1 comment 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Perl
Stars
2
Forks
3
Avg merge
2d 20h
Merged PRs (30d)
3

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_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.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

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.

Assessment

Tech stack
c, xml
Domain
authentication, security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
64/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.