Refactor to help eliminate potential vulnerabilities:

- Obtain references from only the verified signed info bytes
- Return the actual bytes that were verified, but reparsed into a new etree document

Author: ahacker1-securesaml

validate.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"crypto/x509"
 	"encoding/base64"
+	"encoding/xml"
 	"errors"
 	"fmt"
 	"regexp"
@@ -169,6 +170,7 @@ func (ctx *ValidationContext) transform(
 	return el, canonicalizer, nil
 }
 
+// deprecated
 func (ctx *ValidationContext) digest(el *etree.Element, digestAlgorithmId string, canonicalizer Canonicalizer) ([]byte, error) {
 	data, err := canonicalizer.Canonicalize(el)
 	if err != nil {
@@ -189,6 +191,33 @@ func (ctx *ValidationContext) digest(el *etree.Element, digestAlgorithmId string
 	return hash.Sum(nil), nil
 }
 
+func (ctx *ValidationContext) getCanonicalSignedInfo(sig *types.Signature) ([]byte, error) {
+	signatureElement := sig.UnderlyingElement()
+
+	nsCtx, err := etreeutils.NSBuildParentContext(signatureElement)
+	if err != nil {
+		return nil, err
+	}
+
+	signedInfo, err := etreeutils.NSFindOneChildCtx(nsCtx, signatureElement, Namespace, SignedInfoTag)
+	if err != nil {
+		return nil, err
+	}
+
+	if signedInfo == nil {
+		return nil, errors.New("Missing SignedInfo")
+	}
+
+	// Canonicalize the xml
+	canonical, err := canonicalSerialize(signedInfo)
+	if err != nil {
+		return nil, err
+	}
+
+	return canonical, nil
+}
+
+// deprecated
 func (ctx *ValidationContext) verifySignedInfo(sig *types.Signature, canonicalizer Canonicalizer, signatureMethodId string, cert *x509.Certificate, decodedSignature []byte) error {
 	signatureElement := sig.UnderlyingElement()
 
@@ -226,6 +255,48 @@ func (ctx *ValidationContext) verifySignedInfo(sig *types.Signature, canonicaliz
 }
 
 func (ctx *ValidationContext) validateSignature(el *etree.Element, sig *types.Signature, cert *x509.Certificate) (*etree.Element, error) {
+
+	// Actually verify the 'SignedInfo' was signed by a trusted source
+	signatureMethod := sig.SignedInfo.SignatureMethod.Algorithm
+
+	canonicalSignedInfoBytes, err := ctx.getCanonicalSignedInfo(sig)
+	if err != nil {
+		return nil, errors.New("Could not obtain canonical signed info bytes")
+	}
+
+	if canonicalSignedInfoBytes == nil {
+		return nil, errors.New("Missing SignedInfo")
+	}
+
+	algo, ok := x509SignatureAlgorithmByIdentifier[signatureMethod]
+	if !ok {
+		return nil, errors.New("Unknown signature method: " + signatureMethod)
+	}
+
+	if sig.SignatureValue == nil {
+		return nil, errors.New("Signature could not be verified")
+	}
+
+	// Decode the 'SignatureValue' so we can compare against it
+	decodedSignature, err := base64.StdEncoding.DecodeString(sig.SignatureValue.Data)
+	if err != nil {
+		return nil, errors.New("Could not decode signature")
+	}
+
+	err = cert.CheckSignature(algo, canonicalSignedInfoBytes, decodedSignature)
+	if err != nil {
+		return nil, err
+	}
+
+	// only use the verified canonicalSignedInfoBytes
+	// unmarshal canonicalSignedInfoBytes into a new SignedInfo type
+	// to obtain the reference
+	signedInfo := &types.SignedInfo{}
+	err = xml.Unmarshal(canonicalSignedInfoBytes, signedInfo)
+	if err != nil {
+		return nil, err
+	}
+
 	idAttrEl := el.SelectAttr(ctx.IdAttribute)
 	idAttr := ""
 	if idAttrEl != nil {
@@ -235,53 +306,69 @@ func (ctx *ValidationContext) validateSignature(el *etree.Element, sig *types.Si
 	var ref *types.Reference
 
 	// Find the first reference which references the top-level element
-	for _, _ref := range sig.SignedInfo.References {
+	for _, _ref := range signedInfo.References {
 		if _ref.URI == "" || _ref.URI[1:] == idAttr {
 			ref = &_ref
 		}
 	}
 
+	// prevents null pointer deref
+	if ref == nil {
+		return nil, errors.New("Missing reference")
+	}
+
+	digestAlgorithmId := ref.DigestAlgo.Algorithm
+	signedDigestValue, err := base64.StdEncoding.DecodeString(ref.DigestValue)
+	if err != nil {
+		return nil, err
+	}
+
 	// Perform all transformations listed in the 'SignedInfo'
 	// Basically, this means removing the 'SignedInfo'
 	transformed, canonicalizer, err := ctx.transform(el, sig, ref)
 	if err != nil {
 		return nil, err
 	}
 
-	digestAlgorithm := ref.DigestAlgo.Algorithm
-
-	// Digest the transformed XML and compare it to the 'DigestValue' from the 'SignedInfo'
-	digest, err := ctx.digest(transformed, digestAlgorithm, canonicalizer)
+	referencedBytes, err := canonicalizer.Canonicalize(transformed)
 	if err != nil {
 		return nil, err
 	}
 
-	decodedDigestValue, err := base64.StdEncoding.DecodeString(ref.DigestValue)
+	// use a known digest hashing algorithm
+	hashAlgorithm, ok := digestAlgorithmsByIdentifier[digestAlgorithmId]
+	if !ok {
+		return nil, errors.New("Unknown digest algorithm: " + digestAlgorithmId)
+	}
+
+	hash := hashAlgorithm.New()
+	_, err = hash.Write(referencedBytes)
 	if err != nil {
 		return nil, err
 	}
 
-	if !bytes.Equal(digest, decodedDigestValue) {
-		return nil, errors.New("Signature could not be verified")
-	}
-	if sig.SignatureValue == nil {
+	computedDigest := hash.Sum(nil)
+	/* Digest the transformed XML and compare it to the 'DigestValue' from the 'SignedInfo'
+	digest, err := ctx.digest(transformed, digestAlgorithm, canonicalizer)
+	*/
+
+	if !bytes.Equal(computedDigest, signedDigestValue) {
 		return nil, errors.New("Signature could not be verified")
 	}
 
-	// Decode the 'SignatureValue' so we can compare against it
-	decodedSignature, err := base64.StdEncoding.DecodeString(sig.SignatureValue.Data)
-	if err != nil {
-		return nil, errors.New("Could not decode signature")
+	if !(len(computedDigest) >= 20) {
+		return nil, errors.New("Computed digest is less than 20 something went wrong")
 	}
 
-	// Actually verify the 'SignedInfo' was signed by a trusted source
-	signatureMethod := sig.SignedInfo.SignatureMethod.Algorithm
-	err = ctx.verifySignedInfo(sig, canonicalizer, signatureMethod, cert, decodedSignature)
+	// now only the referencedBytes is verified,
+	// unmarshal into new etree
+	doc := etree.NewDocument()
+	err = doc.ReadFromBytes(referencedBytes)
 	if err != nil {
 		return nil, err
 	}
 
-	return transformed, nil
+	return doc.Root(), nil
 }
 
 func contains(roots []*x509.Certificate, cert *x509.Certificate) bool {