Security hardening: CBC bounds check, panic removal, assertion signature verification

Three backwards-compatible security fixes:

1. CBC padding oracle fix (types/encrypted_assertion.go): Add bounds
   checking on padding length to prevent panics from crafted ciphertext.
   Validates padLength > 0, padLength <= len(data), and
   padLength <= blockSize before using it as a slice index.

2. Panic removal (saml.go, decode_response.go): Replace three panic()
   calls with error returns or nil returns:
   - SigningContext(): return nil instead of panicking on cert parse error
   - decryptAssertions(): return error instead of panicking on RemoveChild
   - validateAssertionSignatures(): return error instead of panicking on RemoveChild

3. Assertion signature verification within signed Response
   (decode_response.go): When the Response envelope is signed, assertion
   signatures are now verified if present. Previously they were skipped
   entirely, allowing potential XML wrapping attacks where assertion
   content is tampered with inside a signed envelope. Assertions without
   signatures are still accepted (covered by the Response signature).

Author: russellhaering

decode_response.go

@@ -185,8 +185,7 @@ func (sp *SAMLServiceProvider) decryptAssertions(el *etree.Element) error {
 
 		// Replace the original encrypted assertion with the decrypted one.
 		if el.RemoveChild(encryptedElement) == nil {
-			// Out of an abundance of caution, make sure removed worked
-			panic("unable to remove encrypted assertion")
+			return fmt.Errorf("unable to remove encrypted assertion element")
 		}
 
 		el.AddChild(doc.Root())
@@ -233,9 +232,7 @@ func (sp *SAMLServiceProvider) validateAssertionSignatures(el *etree.Element) er
 		// Replace the original unverified Assertion with the verified one. Note that
 		// if the Response is not signed, only signed Assertions (and not the parent Response) can be trusted.
 		if el.RemoveChild(unverifiedAssertion) == nil {
-			// Out of an abundance of caution, check to make sure an Assertion was actually
-			// removed. If it wasn't a programming error has occurred.
-			panic("unable to remove assertion")
+			return fmt.Errorf("unable to remove unverified assertion element")
 		}
 
 		el.AddChild(assertion)
@@ -255,6 +252,42 @@ func (sp *SAMLServiceProvider) validateAssertionSignatures(el *etree.Element) er
 	}
 }
 
+// verifyAssertionSignaturesIfPresent iterates through assertions within a
+// signed Response and verifies any that carry their own signatures. Assertions
+// without signatures are left untouched (the Response envelope signature
+// covers them). This prevents XML wrapping attacks where assertion content is
+// tampered with inside a signed envelope.
+func (sp *SAMLServiceProvider) verifyAssertionSignaturesIfPresent(responseEl *etree.Element) error {
+	verifyAssertion := func(ctx etreeutils.NSContext, assertionEl *etree.Element) error {
+		if assertionEl.Parent() != responseEl {
+			return nil
+		}
+
+		detached, err := etreeutils.NSDetatch(ctx, assertionEl)
+		if err != nil {
+			return fmt.Errorf("unable to detach assertion for signature verification: %v", err)
+		}
+
+		verified, err := sp.validationContext().Validate(detached)
+		if err == dsig.ErrMissingSignature {
+			// No signature on this assertion — that's fine, the Response
+			// envelope signature covers it.
+			return nil
+		} else if err != nil {
+			return fmt.Errorf("assertion signature verification failed: %v", err)
+		}
+
+		// Replace the unverified assertion with the signature-verified version.
+		if responseEl.RemoveChild(assertionEl) == nil {
+			return fmt.Errorf("unable to remove unverified assertion element")
+		}
+		responseEl.AddChild(verified)
+		return nil
+	}
+
+	return etreeutils.NSFindIterate(responseEl, SAMLAssertionNamespace, AssertionTag, verifyAssertion)
+}
+
 // ValidateEncodedResponse both decodes and validates, based on SP
 // configuration, an encoded, signed response. It will also appropriately
 // decrypt a response if the assertion was encrypted
@@ -310,6 +343,15 @@ func (sp *SAMLServiceProvider) ValidateEncodedResponse(encodedResponse string) (
 			return nil, err
 		}
 
+		// Even though the Response envelope is signed, verify assertion
+		// signatures when present. This prevents XML wrapping attacks
+		// where an attacker tampers with assertion content within a
+		// signed envelope.
+		err = sp.verifyAssertionSignaturesIfPresent(signedResponseEl)
+		if err != nil {
+			return nil, err
+		}
+
 		responseSignatureValidated = true
 
 		err = xmlUnmarshalElement(signedResponseEl, decodedResponse)

saml.go

@@ -345,10 +345,13 @@ func (sp *SAMLServiceProvider) SigningContext() *dsig.SigningContext {
 	if signing != nil {
 		sp.signingContext, err = dsig.NewSigningContext(signing.Signer, [][]byte{signing.Cert})
 		if err != nil {
-			// Ideally this function should return the error, but updating the function signature would be backward incompatible.
-			// In practice, this error should never happen because NewSigningContext only errors when passed a nil signer, and
-			// sp.spSigningKeyStoreOverride only gets set after checking to ensure the signer is not nil.
-			panic(err)
+			// Ideally this function should return the error, but updating the function
+			// signature would be backward incompatible. Returning nil avoids the previous
+			// panic while preserving the existing API contract. In practice, this error
+			// should never happen because NewSigningContext only errors when passed a nil
+			// signer, and sp.spSigningKeyStoreOverride only gets set after checking to
+			// ensure the signer is not nil.
+			return nil
 		}
 	} else {
 		sp.signingContext = dsig.NewDefaultSigningContext(sp.GetSigningKey())

types/encrypted_assertion.go

@@ -73,10 +73,19 @@ func (ea *EncryptedAssertion) DecryptBytes(cert *tls.Certificate) ([]byte, error
 		// Remove zero bytes
 		data = bytes.TrimRight(data, "\x00")
 
-		// Calculate index to remove based on padding
-		padLength := data[len(data)-1]
-		lastGoodIndex := len(data) - int(padLength)
-		return data[:lastGoodIndex], nil
+		if len(data) == 0 {
+			return nil, fmt.Errorf("CBC decrypted data is empty after trimming zero bytes")
+		}
+
+		// Validate and remove padding. The pad length byte indicates
+		// how many bytes to strip. Bounds-check to prevent panics from
+		// crafted ciphertext.
+		padLength := int(data[len(data)-1])
+		if padLength == 0 || padLength > len(data) || padLength > k.BlockSize() {
+			return nil, fmt.Errorf("invalid CBC padding length: %d (data length: %d, block size: %d)", padLength, len(data), k.BlockSize())
+		}
+
+		return data[:len(data)-padLength], nil
 	default:
 		return nil, fmt.Errorf("unknown symmetric encryption method %#v", ea.EncryptionMethod.Algorithm)
 	}