TW: Added a unit test to validate that the XML parsing behavior used …

…is not vulnerable to a comment injection attack.

decode_response.go

@@ -136,8 +136,7 @@ func (sp *SAMLServiceProvider) decryptAssertions(el *etree.Element) error {
 			return fmt.Errorf("unable to decrypt encrypted assertion: %v", derr)
 		}
 
-		doc := etree.NewDocument()
-		err = doc.ReadFromBytes(raw)
+		doc, _, err := parseResponse(raw)
 		if err != nil {
 			return fmt.Errorf("unable to create element from decrypted assertion bytes: %v", derr)
 		}
@@ -218,25 +217,10 @@ func (sp *SAMLServiceProvider) ValidateEncodedResponse(encodedResponse string) (
 		return nil, err
 	}
 
-	doc := etree.NewDocument()
-	err = doc.ReadFromBytes(raw)
+	// Parse the raw response
+	doc, el, err := parseResponse(raw)
 	if err != nil {
-		// Attempt to inflate the response in case it happens to be compressed (as with one case at saml.oktadev.com)
-		buf, err := ioutil.ReadAll(flate.NewReader(bytes.NewReader(raw)))
-		if err != nil {
-			return nil, err
-		}
-
-		doc = etree.NewDocument()
-		err = doc.ReadFromBytes(buf)
-		if err != nil {
-			return nil, err
-		}
-	}
-
-	el := doc.Root()
-	if el == nil {
-		return nil, fmt.Errorf("unable to parse response")
+		return nil, err
 	}
 
 	var responseSignatureValidated bool
@@ -292,3 +276,29 @@ func (sp *SAMLServiceProvider) ValidateEncodedResponse(encodedResponse string) (
 
 	return decodedResponse, nil
 }
+
+// parseResponse is a helper function that was refactored out so that the XML parsing behavior can be isolated and unit tested
+func parseResponse(xml []byte) (*etree.Document, *etree.Element, error) {
+	doc := etree.NewDocument()
+	err := doc.ReadFromBytes(xml)
+	if err != nil {
+		// Attempt to inflate the response in case it happens to be compressed (as with one case at saml.oktadev.com)
+		buf, err := ioutil.ReadAll(flate.NewReader(bytes.NewReader(xml)))
+		if err != nil {
+			return nil, nil, err
+		}
+
+		doc = etree.NewDocument()
+		err = doc.ReadFromBytes(buf)
+		if err != nil {
+			return nil, nil, err
+		}
+	}
+
+	el := doc.Root()
+	if el == nil {
+		return nil, nil, fmt.Errorf("unable to parse response")
+	}
+
+	return doc, el, nil
+}

saml_test.go

@@ -11,6 +11,7 @@ import (
 	"compress/flate"
 
 	"github.com/beevik/etree"
+	"github.com/russellhaering/gosaml2/types"
 	"github.com/russellhaering/goxmldsig"
 	require "github.com/stretchr/testify/require"
 )
@@ -253,3 +254,35 @@ func TestInvalidResponseNoElement(t *testing.T) {
 	require.EqualError(t, err, "unable to parse response")
 	require.Nil(t, response)
 }
+func TestSAMLCommentInjection(t *testing.T) {
+	/*
+		Explanation:
+
+		See: https://duo.com/blog/duo-finds-saml-vulnerabilities-affecting-multiple-implementations
+
+		The TLDR is that XML canonicalization may result in a different value being signed from the one being retrieved.
+		The target of this is the NameID in the Subject of the SAMLResponse Assertion
+
+		Example:
+			 The following Subject
+			 ```<Subject>
+				<NameID>[email protected]<!---->.evil.com</NameID>
+			</Subject>```
+			would get canonicalized to
+			```
+			<Subject>
+				<NameID>[email protected]</NameID>
+			</Subject>
+			```
+			Many XML parsers have a behavior where they pull the first text element, so in the example with the comment, a vulnerable XML parser would return `[email protected]`, ignoring the text after the comment.
+			Knowing this, a user ([email protected]) can attack a vulnerable SP by manipulating their signed SAMLResponse with a comment that turns their username into another one.
+	*/
+
+	// To show that we are not vulnerable, we want to prove that we get the canonicalized value using our parser
+	_, el, err := parseResponse([]byte(commentInjectionAttackResponse))
+	require.NoError(t, err)
+	decodedResponse := &types.Response{}
+	err = xmlUnmarshalElement(el, decodedResponse)
+	require.NoError(t, err)
+	require.Equal(t, "[email protected]", decodedResponse.Assertions[0].Subject.NameID.Value, "The full, canonacalized NameID should be returned.")
+}