connector/saml: fix validation bug with multiple Assertion elements

When a SAML response provided multiple Assertion elements, only the
first one is checked for a valid signature. If the Assertion is
verified, the original Assertion is removed and the canonicalized
version is prepended to the Response. However, if there were
multiple assertions, the second assertion could end up first in the
list of Assertions, even if it was unsigned.

For example this:

    <Response>
      <!--
         Response unsigned. According to SAML spec must check
         assertion signature.
      -->
      <Assertion>
        <Signature>
          <!-- Correrctly signed assertion -->
        </Signature>
      </Assertion>

      <Assertion>
        <!-- Unsigned assertion inserted by attacker-->
      </Assertion>
    </Response>

could be verified then re-ordered to the following:

    <Response>
      <!--
         Response unsigned. According to SAML spec must check
         assertion signature.
      -->
      <Assertion>
        <!-- Unsigned assertion inserted by attacker-->
      </Assertion>

      <Assertion>
        <!-- Canonicalized, correrctly signed assertion -->
      </Assertion>
    </Response>

Fix this by removing all unverified child elements of the Response,
not just the original assertion.

connector/saml/saml.go

@@ -130,9 +130,7 @@ func (c *Config) Open(logger logrus.FieldLogger) (connector.Connector, error) {
 	return c.openConnector(logger)
 }
 
-func (c *Config) openConnector(logger logrus.FieldLogger) (interface {
+func (c *Config) openConnector(logger logrus.FieldLogger) (*provider, error) {
-	connector.SAMLConnector
-}, error) {
 	requiredFields := []struct {
 		name, val string
 	}{
@@ -274,8 +272,11 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str
 	if err != nil {
 		return ident, fmt.Errorf("decode response: %v", err)
 	}
+
+	rootElementSigned := true
 	if p.validator != nil {
-		if rawResp, err = verify(p.validator, rawResp); err != nil {
+		rawResp, rootElementSigned, err = verifyResponseSig(p.validator, rawResp)
+		if err != nil {
 			return ident, fmt.Errorf("verify signature: %v", err)
 		}
 	}
@@ -285,24 +286,26 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str
 		return ident, fmt.Errorf("unmarshal response: %v", err)
 	}
 
-	if p.issuer != "" && resp.Issuer != nil && resp.Issuer.Issuer != p.issuer {
+	if rootElementSigned {
-		return ident, fmt.Errorf("expected Issuer value %s, got %s", p.issuer, resp.Issuer.Issuer)
+		if p.issuer != "" && resp.Issuer != nil && resp.Issuer.Issuer != p.issuer {
-	}
+			return ident, fmt.Errorf("expected Issuer value %s, got %s", p.issuer, resp.Issuer.Issuer)
+		}
 
-	// Verify InResponseTo value matches the expected ID associated with
+		// Verify InResponseTo value matches the expected ID associated with
-	// the RelayState.
+		// the RelayState.
-	if resp.InResponseTo != inResponseTo {
+		if resp.InResponseTo != inResponseTo {
-		return ident, fmt.Errorf("expected InResponseTo value %s, got %s", inResponseTo, resp.InResponseTo)
+			return ident, fmt.Errorf("expected InResponseTo value %s, got %s", inResponseTo, resp.InResponseTo)
-	}
+		}
 
-	// Destination is optional.
+		// Destination is optional.
-	if resp.Destination != "" && resp.Destination != p.redirectURI {
+		if resp.Destination != "" && resp.Destination != p.redirectURI {
-		return ident, fmt.Errorf("expected destination %q got %q", p.redirectURI, resp.Destination)
+			return ident, fmt.Errorf("expected destination %q got %q", p.redirectURI, resp.Destination)
 
-	}
+		}
 
-	if err = p.validateStatus(&resp); err != nil {
+		if err = p.validateStatus(&resp); err != nil {
-		return ident, err
+			return ident, err
+		}
 	}
 
 	assertion := resp.Assertion
@@ -481,41 +484,57 @@ func (p *provider) validateConditions(assertion *assertion) error {
 	return nil
 }
 
-// verify checks the signature info of a XML document and returns
+// verifyResponseSig attempts to verify the signature of a SAML response or
-// the signed elements.
+// the assertion.
-// The Validate function of the goxmldsig library only looks for
+//
-// signatures on the root element level. But a saml Response is valid
+// If the root element is properly signed, this method returns it.
-// if the complete message is signed, or only the Assertion is signed,
+//
-// or but elements are signed. Therefore we first check a possible
+// The SAML spec requires supporting responses where the root element is
-// signature of the Response than of the Assertion. If one of these
+// unverified, but the sub <Assertion> elements are signed. In these cases,
-// is successful the Response is considered as valid.
+// this method returns rootVerified=false to indicate that the <Assertion>
-func verify(validator *dsig.ValidationContext, data []byte) (signed []byte, err error) {
+// elements should be trusted, but all other elements MUST be ignored.
+//
+// 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) {
 	doc := etree.NewDocument()
 	if err = doc.ReadFromBytes(data); err != nil {
-		return nil, fmt.Errorf("parse document: %v", err)
+		return nil, false, fmt.Errorf("parse document: %v", err)
 	}
-	verified := false
+
 	response := doc.Root()
 	transformedResponse, err := validator.Validate(response)
 	if err == nil {
-		verified = true
+		// Root element is verified, return it.
 		doc.SetRoot(transformedResponse)
+		signed, err = doc.WriteToBytes()
+		return signed, true, err
 	}
+
 	// 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 {
-		return nil, fmt.Errorf("response does not contain an Assertion element")
+		return nil, false, fmt.Errorf("response does not contain an Assertion element")
 	}
 	transformedAssertion, err := validator.Validate(assertion)
-	if err == nil {
+	if err != nil {
-		verified = true
+		return nil, false, fmt.Errorf("response does not contain a valid signature element: %v", err)
-		response.RemoveChild(assertion)
-		response.AddChild(transformedAssertion)
 	}
-	if verified != true {
+
-		return nil, fmt.Errorf("response does not contain a valid Signature element")
+	// Verified an assertion but not the response. Can't trust any child elements,
+	// except the assertion. Remove them all.
+	for _, el := range response.ChildElements() {
+		response.RemoveChild(el)
 	}
-	return doc.WriteToBytes()
+
+	// 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
 }
 
 func uuidv4() string {