Merge pull request #90 from michaelschiff/issue/59

Fix: Crash on nil-pointer dereference with malformed input

decode_response.go

@@ -204,7 +204,11 @@ func (sp *SAMLServiceProvider) validateAssertionSignatures(el *etree.Element) er
 	signedAssertions := 0
 	unsignedAssertions := 0
 	validateAssertion := func(ctx etreeutils.NSContext, unverifiedAssertion *etree.Element) error {
-		if unverifiedAssertion.Parent() != el {
+		parent := unverifiedAssertion.Parent()
+		if parent == nil {
+			return fmt.Errorf("parent is nil")
+		}
+		if parent != el {
 			return fmt.Errorf("found assertion with unexpected parent element: %s", unverifiedAssertion.Parent().Tag)
 		}
 

decode_response_test.go

@@ -18,6 +18,7 @@ import (
 	"bytes"
 	"crypto/tls"
 	"crypto/x509"
+	"encoding/base64"
 	"encoding/pem"
 	"io/ioutil"
 	"testing"
@@ -29,7 +30,8 @@ import (
 	rtvalidator "github.com/mattermost/xml-roundtrip-validator"
 )
 
-const idpCert = `
+const (
+	idpCert = `
 -----BEGIN CERTIFICATE-----
 MIIDODCCAiCgAwIBAgIUQH54kyyeacU69J2iwz9bzeLmMaswDQYJKoZIhvcNAQEL
 BQAwHTEbMBkGA1UEAwwSY29sbGVnZS5jY2N0Y2EuZWR1MB4XDTE1MDYwNDIyMTAz
@@ -52,7 +54,7 @@ CQ1CF8ZDDJ0XV6Ab
 -----END CERTIFICATE-----
 `
 
-const oktaCert = `
+	oktaCert = `
 -----BEGIN CERTIFICATE-----
 MIIDPDCCAiQCCQDydJgOlszqbzANBgkqhkiG9w0BAQUFADBgMQswCQYDVQQGEwJVUzETMB
 EGA1UECBMKQ2FsaWZvcm5pYTEWMBQGA1UEBxMNU2FuIEZyYW5jaXNjbzEQMA4GA1UEChMH
@@ -73,6 +75,31 @@ ojLdrotYLGd9JOsoQhElmz+tMfCFQUFLExinPAyy7YHlSiVX13QH2XTu/iQQ==
 -----END CERTIFICATE-----
 `
 
+	oktaCert2 = `
+-----BEGIN CERTIFICATE-----
+MIIDpDCCAoygAwIBAgIGAWxzAwX1MA0GCSqGSIb3DQEBCwUAMIGSMQswCQYDVQQGEwJVUzETMBEG
+A1UECAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNU2FuIEZyYW5jaXNjbzENMAsGA1UECgwET2t0YTEU
+MBIGA1UECwwLU1NPUHJvdmlkZXIxEzARBgNVBAMMCmRldi05MDUyNTExHDAaBgkqhkiG9w0BCQEW
+DWluZm9Ab2t0YS5jb20wHhcNMTkwODA4MjA1MzMzWhcNMjkwODA4MjA1NDMzWjCBkjELMAkGA1UE
+BhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWExFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xDTALBgNV
+BAoMBE9rdGExFDASBgNVBAsMC1NTT1Byb3ZpZGVyMRMwEQYDVQQDDApkZXYtOTA1MjUxMRwwGgYJ
+KoZIhvcNAQkBFg1pbmZvQG9rdGEuY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA
+m+ZZF6aEG6ehLLIV6RPA+i1z6ss3HBG2bZD3efwKCDDXYUkp59AE7JsjVHMtpJPHhzHuScuHDMlu
+HmkBQTW7j9XpnaRn8SfZXkwlCUHTo+HAC9lwbQxO4d4wnwgnm6FAjm1I/gbfFAobd8BR9pDxHuXE
+MQ0DtQu/W3WbDUrz/bhSxPJAoVy2koQn9G0y3unm7eRwYWHeuW6GdPWV2szTtDS0c3qtUXVF5Ugg
+iQYlwQu6xkfy4l8iGJL7ETa2BmJzwCFecMIct87SqNhYQwCBH54MBaHcaSsCKyimNvMY9B7RmC+H
+4+awePPA1q3R/UQ3Pfom8mx6yDdKIWqlkG3MsQIDAQABMA0GCSqGSIb3DQEBCwUAA4IBAQAiURCZ
+P4oJWcf1o5nm4yG15UH01g/S6Y4OUWMi6BFJy9fCrJ0h/2BZKi68SQ0uMAbdK6anxCzq3Rr5MSzW
+OWPQ1Zljn3LGPsiTFdFca/GVRen5IYQ7Dr2Mvhtm+QVscEY9TDjtETbTAHEVEjwXmB21wtdIhizv
+sQS7wz0A8LV+Atpbev45RiV6COmB6T6vJuFQ7ZsDZMSHZriTYiETTJvHBGd7PtbCxYNc6LRB2JDb
+wlekRhVEjR0UhnM+nn2sqqbv7tDEPs63lZSDXCnR1PhscHrEuQ04rHI3OL0gCULVQFvJrj85IAZF
+1QQuGUK8ozfOyFpQWAJUW71INnF/SLWv
+-----END CERTIFICATE-----
+`
+
+	badInput = `<saml2:Assertion ID="id1684056077776386493060641"IssueInstant="2019-08-12T12:00:52.718Z"Version="2.0"xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion"xmlns:xs="http://www.w3.org/2001/XMLSchema"><saml2:Issuer Format="urn:oasis:names:tc:SAML:2.0:nameid-format:entity"xmlns="">http://www.okta.com/exk133onomIuOW98z357</l><ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#"><ds:SignedInfo><ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/><ds:SignatureMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/><ds:Reference URI="#id1684056077776386493060641"><ds:Transforms><ds:Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/><ds:Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"><ec:InclusiveNamespaces PrefixList="xs"xmlns:ec="http://www.w3.org/2001/10/xml-exc-c14n#"/></m></s><ds:DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256"/><ds:DigestValue>dC1cm0pLLjIWZC6G2Pmf0JogmqHztp9W1euXPd/TUHo=</e></e></o><ds:SignatureValue>YRSCFLIkIgjbbYLyfCIc8jsP2MUJPjn+nYWRdlVIDdXtYXXxklYqdBXQsxDwNcsOAIGS75PeVGryml3oBkUDg/MfK7z/fFPLXX7c7xgh7/DBAFlSXbwlJQxuXQ5eZcGesgG6nYRwU1hpW+yN7C2ODN9KHi5TUdiEhvy8vdlFSfxdy4Mn68nG/UZBqmHHIZdRG2/Hpcs29YyaVVZUCZ0w22b7zsPuOXHuStOSTQ6isxI2R268+ZNKERYaNMCAGX4zNlT3mHBV0NnZkbO3wmlOfKksL+Qx7L64xFc3PaervxWuPqh2FoWpTCqFdliLdvUfFDszKXJKhO0bj1U0aSrdzg==</e><s><s><s></X></X></o></e><saml2:Subject xmlns=""><saml2:NameID Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified">steven.james.johnstone@gmail.com</l><saml2:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer"><saml2:SubjectConfirmationData InResponseTo="_40a419f5-5c1c-43d0-5834-5caf268a5f01"NotOnOrAfter="2019-08-12T12:05:52.718Z"Recipient="https://127.0.0.1/login"/></l></l><saml2:Conditions NotBefore="2019-08-12T11:55:52.718Z"NotOnOrAfter="2019-08-12T12:05:52.718Z"xmlns=""><saml2:AudienceRestriction><saml2:Audience>37a8eec1ce19687d132fe29051dca629d164e2c4958ba141d5f4133a33f0688f.jazznetworks.com</l></l></l><saml2:AuthnStatement AuthnInstant="2019-08-12T12:00:52.718Z"SessionIndex="_40a419f5-5c1c-43d0-5834-5caf268a5f01"xmlns=""><saml2:AuthnContext><saml2:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport</l></l></l><saml2:AttributeStatement xmlns=""><saml2:Attribute Name="FirstName"NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:unspecified"><saml2:AttributeValue xmlns=""xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"xsi:type="xs:string">Steven</l></l><saml2:Attribute Name="LastName"NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:unspecified"><saml2:AttributeValue xmlns=""xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"xsi:type="xs:string">Johnstone</l></l><saml2:Attribute Name="Email"NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:unspecified"><saml2:AttributeValue xmlns=""xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"xsi:type="xs:string">steven.james.johnstone@gmail.com`
+)
+
 func testEncryptedAssertion(t *testing.T, validateEncryptionCert bool) {
 	var err error
 	cert, err := tls.LoadX509KeyPair("./testdata/test.crt", "./testdata/test.key")
@@ -155,4 +182,25 @@ func TestDecodeDoubleColonInjectionAttackResponse(t *testing.T) {
 
 	_, _, err := parseResponse([]byte(doubleColonAssertionInjectionAttackResponse))
 	require.Error(t, err)
+}
+
+func TestMalFormedInput(t *testing.T) {
+	block, _ := pem.Decode([]byte(oktaCert2))
+	idpCert, err := x509.ParseCertificate(block.Bytes)
+	require.NoError(t, err, "couldn't parse okta cert pem block")
+
+	certStore := dsig.MemoryX509CertificateStore{
+		Roots: []*x509.Certificate{idpCert},
+	}
+
+	sp := &SAMLServiceProvider{
+		Clock: dsig.NewFakeClock(clockwork.NewFakeClockAt(time.Date(2019, 8, 12, 12, 00, 52, 718, time.UTC))),
+		AssertionConsumerServiceURL: "https://saml2.test.astuart.co/sso/saml2",
+		SignAuthnRequests:           true,
+		IDPCertificateStore:         &certStore,
+		ValidateEncryptionCert:      true,
+	}
+	base64Input := base64.StdEncoding.EncodeToString([]byte(badInput))
+	_, err = sp.RetrieveAssertionInfo(base64Input)
+	require.Errorf(t, err, "parent is nil")
 }