Merge pull request from GHSA-q547-gmf8-8jr7

Improved Signature object shape validation
russellhaering · Sep 29, 2020 · 6f318b2 · 6f318b2
2 parents 2e1fbc2 + f6188fe
commit 6f318b2
Show file tree

Hide file tree

Showing 2 changed files with 216 additions and 8 deletions.
diff --git a/validate.go b/validate.go
@@ -21,6 +21,7 @@ var (
 	// ErrMissingSignature indicates that no enveloped signature was found referencing
 	// the top level element passed for signature verification.
 	ErrMissingSignature = errors.New("Missing signature referencing the top-level element")
+	ErrInvalidSignature = errors.New( "Invalid Signature")
 )
 
 type ValidationContext struct {
@@ -296,20 +297,42 @@ func contains(roots []*x509.Certificate, cert *x509.Certificate) bool {
 	return false
 }
 
+// In most places, we use etree Elements, but while deserializing the Signature, we use
+// encoding/xml unmarshal directly to convert to a convenient go struct. This presents a problem in some cases because
+// when an xml element repeats under the parent, the last element will win and/or be appended. We need to assert that
+// the Signature object matches the expected shape of a Signature object.
+func validateShape(signatureEl *etree.Element) error {
+	children := signatureEl.ChildElements()
+
+	childCounts := map[string]int{}
+	for _, child := range children {
+		childCounts[child.Tag]++
+	}
+
+	validateCount := childCounts[SignedInfoTag] == 1 && childCounts[KeyInfoTag] <= 1 && childCounts[SignatureValueTag] == 1
+	if !validateCount {
+		return ErrInvalidSignature
+	}
+	return nil
+}
+
 // findSignature searches for a Signature element referencing the passed root element.
-func (ctx *ValidationContext) findSignature(el *etree.Element) (*types.Signature, error) {
-	idAttr := el.SelectAttr(ctx.IdAttribute)
+func (ctx *ValidationContext) findSignature(root *etree.Element) (*types.Signature, error) {
+	idAttr := root.SelectAttr(ctx.IdAttribute)
 	if idAttr == nil || idAttr.Value == "" {
 		return nil, errors.New("Missing ID attribute")
 	}
 
 	var sig *types.Signature
 
 	// Traverse the tree looking for a Signature element
-	err := etreeutils.NSFindIterate(el, Namespace, SignatureTag, func(ctx etreeutils.NSContext, el *etree.Element) error {
-
+	err := etreeutils.NSFindIterate(root, Namespace, SignatureTag, func(ctx etreeutils.NSContext, signatureEl *etree.Element) error {
+		err := validateShape(signatureEl)
+		if err != nil {
+			return err
+		}
 		found := false
-		err := etreeutils.NSFindChildrenIterateCtx(ctx, el, Namespace, SignedInfoTag,
+		err = etreeutils.NSFindChildrenIterateCtx(ctx, signatureEl, Namespace, SignedInfoTag,
 			func(ctx etreeutils.NSContext, signedInfo *etree.Element) error {
 				detachedSignedInfo, err := etreeutils.NSDetatch(ctx, signedInfo)
 				if err != nil {
@@ -355,8 +378,8 @@ func (ctx *ValidationContext) findSignature(el *etree.Element) (*types.Signature
 					return fmt.Errorf("invalid CanonicalizationMethod on Signature: %s", c14NAlgorithm)
 				}
 
-				el.RemoveChild(signedInfo)
-				el.AddChild(canonicalSignedInfo)
+				signatureEl.RemoveChild(signedInfo)
+				signatureEl.AddChild(canonicalSignedInfo)
 
 				found = true
 
@@ -372,7 +395,7 @@ func (ctx *ValidationContext) findSignature(el *etree.Element) (*types.Signature
 
 		// Unmarshal the signature into a structured Signature type
 		_sig := &types.Signature{}
-		err = etreeutils.NSUnmarshalElement(ctx, el, _sig)
+		err = etreeutils.NSUnmarshalElement(ctx, signatureEl, _sig)
 		if err != nil {
 			return err
 		}