Skip to content

Fix saml signature verification #4165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ahacker1-securesaml wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix saml signature verification #4165

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6717fa
Refactor Signature Verification Logic (part 1):
ahacker1-securesaml Jun 2, 2025
faee7bf
Refactor Signature Verification Logic (part 2):
ahacker1-securesaml Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 67 additions & 28 deletions connector/saml/saml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,22 +304,30 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str
}

// Root element is allowed to not be signed if the Assertion element is.
rootElementSigned := true
var rawSignedAssertion []byte
var rawSignedResponse []byte

if p.validator != nil {
rawResp, rootElementSigned, err = verifyResponseSig(p.validator, rawResp)
// if raw assertion must be
rawSignedAssertion, rawSignedResponse, err = verifyResponseSig(p.validator, rawResp)
if err != nil {
return ident, fmt.Errorf("verify signature: %v", err)
}
}
} else {
// no validator? I'm rejecting it for now, because it's unsafe.

var resp response
if err := xml.Unmarshal(rawResp, &resp); err != nil {
return ident, fmt.Errorf("unmarshal response: %v", err)
}

// If the root element isn't signed, there's no reason to inspect these
// elements. They're not verified.
if rootElementSigned {

// we got a signed response, let's carry about business logic checks on it
if rawSignedResponse != nil {
var resp response
if err := xml.Unmarshal(rawSignedResponse, &resp); err != nil {
return ident, fmt.Errorf("unmarshal response: %v", err)
}

if p.ssoIssuer != "" && resp.Issuer != nil && resp.Issuer.Issuer != p.ssoIssuer {
return ident, fmt.Errorf("expected Issuer value %s, got %s", p.ssoIssuer, resp.Issuer.Issuer)
}
Expand All @@ -345,11 +353,17 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str
}
}

assertion := resp.Assertion
if assertion == nil {
if rawSignedAssertion == nil {
return ident, fmt.Errorf("response did not contain an assertion")
}

// assertion, use the rawSignedAssertion

var assertion assertion
if err := xml.Unmarshal(rawSignedAssertion, &assertion); err != nil {
return ident, fmt.Errorf("unmarshal response: %v", err)
}

// Subject is usually optional, but we need it for the user ID, so complain
// if it's not present.
subject := assertion.Subject
Expand Down Expand Up @@ -580,44 +594,68 @@ func (p *provider) validateConditions(conditions *conditions) error {

// verifyResponseSig attempts to verify the signature of a SAML response or
// the assertion.
// Using the signed contents from the signature, it attempts to obtain both an assertion and if the response is signed
// the response
//
// If the root element is properly signed, this method returns it.
// If the root response is signed: returns the first assertion, along with the root response
//
// The SAML spec requires supporting responses where the root element is
// unverified, but the sub <Assertion> elements are signed. In these cases,
// this method returns rootVerified=false to indicate that the <Assertion>
// elements should be trusted, but all other elements MUST be ignored.
// unverified, but the sub <Assertion> elements are signed.
// In these cases, the method returns the assertion, however, the signedResponse will be nil
Copy link

@DemiMarie DemiMarie Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to create a response that contains only the assertion, data derived from the assertion, and data not under attacker control?

Copy link
Author

@ahacker1-securesaml ahacker1-securesaml Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a case where only the assertion is signed like:

<Response InResponseTo="...">
<Assertion><Signature/>
Subject ...
</Assertion>
</Response>

What we can tell is signed is the canonical assertion string:

<Assertion>
subject
</Assertion>

In that case I just reparse the Assertino node and return it.

I think you're interested in also returning a Response.
In this case, I actually return nil for the Response since we can't tell it's signed. (And further on we don't actually use the Response element, we just process the assertion element)

Previously dexidp, kept the original response ,but then removed all the child elements and replaced them with our signed assertion node.

This is unsafe because you end up with something like

etree {
   "rootElement": {
         "name": "Response",
         "attrs": ["USER CONTROLLED"], 
        "childElements": [SignedAssertionNode]
    }, 
   "directives": ["USER_CONTROLLED"], 
   "other xml parts": "USERCONTROLLED"
}

When serializing our etree element, the USER_CONTROLLED could round trip and actually become another Assertion node. When reparsing it sees our USER_CONTROLLED data.

So now I don't even bother to return the response if only assertion is signed

Copy link

@DemiMarie DemiMarie Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant is to return something like

<Response>
<Assertion>
stuff
</Assertion>
</Response>

where no components are attacker controlled.

Copy link
Author

@ahacker1-securesaml ahacker1-securesaml Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, that could work.
However, I think it's simpler if we return (assertion, response) as a pair. It's simpler since the we only need the assertion for obtaining the user identity.

The response element is quite different, and sometimes unsigned. So it's better to separate the two.

//
// Note: we still don't support multiple <Assertion> tags. If there are
// multiple present this code will only process the first.
func verifyResponseSig(validator *dsig.ValidationContext, data []byte) (signed []byte, rootVerified bool, err error) {
func verifyResponseSig(validator *dsig.ValidationContext, data []byte) (signedAssertion []byte, signedResponse []byte, err error) {
doc := etree.NewDocument()
if err = doc.ReadFromBytes(data); err != nil {
return nil, false, fmt.Errorf("parse document: %v", err)
return nil, nil, fmt.Errorf("parse document: %v", err)
}

response := doc.Root()
if response == nil {
return nil, false, fmt.Errorf("parse document: empty root")
return nil, nil, fmt.Errorf("parse document: empty root")
}
// transformedResponse is signed, show return value is parsed solely from transformedResponse
transformedResponse, err := validator.Validate(response)
if err == nil {
// Root element is verified, return it.
doc.SetRoot(transformedResponse)
signed, err = doc.WriteToBytes()
return signed, true, err
// signedResponse is the serialization of ONLY transformedResponse
isolatedDoc := etree.NewDocument()
isolatedDoc.SetRoot(transformedResponse)
signedResponse, err := isolatedDoc.WriteToBytes()
if err != nil || signedResponse == nil {
return nil, nil, fmt.Errorf("serialize response document: %v", err)
}

// signedAssertionElement part of transformedResponse
signedAssertionElement, err := etreeutils.NSSelectOne(transformedResponse, "urn:oasis:names:tc:SAML:2.0:assertion", "Assertion")
if err != nil || signedAssertionElement == nil {
return nil, nil, fmt.Errorf("response does not contain an Assertion element")
}

assertionIsolatedDoc := etree.NewDocument()
assertionIsolatedDoc.SetRoot(signedAssertionElement)
signedAssertion, err := assertionIsolatedDoc.WriteToBytes()
if err != nil || signedAssertion == nil {
return nil, nil, fmt.Errorf("serialize assertion node: %v", err)
}
return signedAssertion, signedResponse, nil
}

// Case 2: Assertion signed

// Ensures xmlns are copied down to the assertion element when they are defined in the root
//
// TODO: Only select from child elements of the root.
assertion, err := etreeutils.NSSelectOne(response, "urn:oasis:names:tc:SAML:2.0:assertion", "Assertion")
if err != nil || assertion == nil {
return nil, false, fmt.Errorf("response does not contain an Assertion element")
return nil, nil, fmt.Errorf("response does not contain an Assertion element")
}

// transformedAssertion is signed

transformedAssertion, err := validator.Validate(assertion)
if err != nil {
return nil, false, fmt.Errorf("response does not contain a valid signature element: %v", err)
return nil, nil, fmt.Errorf("response does not contain a valid signature element: %v", err)
}

// Verified an assertion but not the response. Can't trust any child elements,
Expand All @@ -626,12 +664,13 @@ func verifyResponseSig(validator *dsig.ValidationContext, data []byte) (signed [
response.RemoveChild(el)
}

// We still return the full <Response> element, even though it's unverified
// because the <Assertion> element is not a valid XML document on its own.
// It still requires the root element to define things like namespaces.
response.AddChild(transformedAssertion)
signed, err = doc.WriteToBytes()
return signed, false, err
newDoc := etree.NewDocument()
newDoc.SetRoot(transformedAssertion)
signedAssertion, err = newDoc.WriteToBytes()
if err != nil || signedAssertion == nil {
return nil, nil, fmt.Errorf("serialize signed assertion: %v", err)
}
return signedAssertion, nil, nil
}

// before determines if a given time is before the current time, with an
Expand Down