From a6717fa1d0cf93bec32fe18b191936ec141d97b5 Mon Sep 17 00:00:00 2001 From: SecureSAML Date: Mon, 2 Jun 2025 19:35:54 -0400 Subject: [PATCH 1/2] Refactor Signature Verification Logic (part 1): - Ensures return data is parsed solely from signed contents Signed-off-by: Alexander Tan --- connector/saml/saml.go | 63 +++++++++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 19 deletions(-) diff --git a/connector/saml/saml.go b/connector/saml/saml.go index 3e44b477..d44ee0cf 100644 --- a/connector/saml/saml.go +++ b/connector/saml/saml.go @@ -580,44 +580,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 elements are signed. In these cases, -// this method returns rootVerified=false to indicate that the -// elements should be trusted, but all other elements MUST be ignored. +// unverified, but the sub elements are signed. +// In these cases, the method returns the assertion, however, the signedResponse will be nil // // Note: we still don't support multiple 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 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() + isolatedDoc.SetRoot(signedAssertionElement) + signedAssertion, err := assertionIsolatedDoc.WriteToBytes() + if err != nil || signedAssertion == nil { + return nil, nil, fmt.Errorf("serialize document: %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, @@ -626,12 +650,13 @@ func verifyResponseSig(validator *dsig.ValidationContext, data []byte) (signed [ response.RemoveChild(el) } - // We still return the full element, even though it's unverified - // because the 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 document: %v", err) + } + return signedAssertion, nil, nil } // before determines if a given time is before the current time, with an From faee7bf69e5a3fa92b0a34b3b1e831971bc971dd Mon Sep 17 00:00:00 2001 From: SecureSAML Date: Mon, 2 Jun 2025 19:54:38 -0400 Subject: [PATCH 2/2] Refactor Signature Verification Logic (part 2): - Get identity from only the signed assertion - Fixes bugs in code Signed-off-by: Alexander Tan Signed-off-by: Alexander Tan --- connector/saml/saml.go | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/connector/saml/saml.go b/connector/saml/saml.go index d44ee0cf..f5bf8e37 100644 --- a/connector/saml/saml.go +++ b/connector/saml/saml.go @@ -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) } @@ -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 @@ -608,8 +622,8 @@ func verifyResponseSig(validator *dsig.ValidationContext, data []byte) (signedAs isolatedDoc := etree.NewDocument() isolatedDoc.SetRoot(transformedResponse) signedResponse, err := isolatedDoc.WriteToBytes() - if err != nil || signedResponse != nil { - return nil, nil, fmt.Errorf("serialize document: %v", err) + if err != nil || signedResponse == nil { + return nil, nil, fmt.Errorf("serialize response document: %v", err) } // signedAssertionElement part of transformedResponse @@ -619,10 +633,10 @@ func verifyResponseSig(validator *dsig.ValidationContext, data []byte) (signedAs } assertionIsolatedDoc := etree.NewDocument() - isolatedDoc.SetRoot(signedAssertionElement) + assertionIsolatedDoc.SetRoot(signedAssertionElement) signedAssertion, err := assertionIsolatedDoc.WriteToBytes() if err != nil || signedAssertion == nil { - return nil, nil, fmt.Errorf("serialize document: %v", err) + return nil, nil, fmt.Errorf("serialize assertion node: %v", err) } return signedAssertion, signedResponse, nil } @@ -654,7 +668,7 @@ func verifyResponseSig(validator *dsig.ValidationContext, data []byte) (signedAs newDoc.SetRoot(transformedAssertion) signedAssertion, err = newDoc.WriteToBytes() if err != nil || signedAssertion == nil { - return nil, nil, fmt.Errorf("serialize document: %v", err) + return nil, nil, fmt.Errorf("serialize signed assertion: %v", err) } return signedAssertion, nil, nil }