From 7db3cb8b6152a15dbabcebbfab9aadbbb28d0599 Mon Sep 17 00:00:00 2001 From: Serge Lysenko Date: Wed, 21 Sep 2022 15:57:55 +0300 Subject: [PATCH 1/6] fix incorrect element removing in enveloped-signature trasform --- main_test.go | 46 ++++++++++++++++++++++++++++++++++++++++++++++ validate.go | 25 ++++++++++++------------- 2 files changed, 58 insertions(+), 13 deletions(-) create mode 100644 main_test.go diff --git a/main_test.go b/main_test.go new file mode 100644 index 0000000..2fca4b5 --- /dev/null +++ b/main_test.go @@ -0,0 +1,46 @@ +package dsig + +import ( + "crypto/x509" + "testing" + + "github.com/beevik/etree" + "github.com/stretchr/testify/require" +) + +func TestDocumentedExample(t *testing.T) { + + // Generate a key and self-signed certificate for signing + randomKeyStore := RandomKeyStoreForTest() + ctx := NewDefaultSigningContext(randomKeyStore) + elementToSign := &etree.Element{ + Tag: "ExampleElement", + } + elementToSign.CreateAttr("ID", "id1234") + + dataValue := elementToSign.CreateElement("XData") + dataValue.CreateAttr("kind", "test") + dataValue.SetText("zip: 586a6289e2ff09b0826dd1daeab5237735a3a728afc48d11976bbed1fbaeaf0a") + + // Sign the element + signedElement, err := ctx.SignEnveloped(elementToSign) + require.NoError(t, err) + + // Validate + _, certData, err := ctx.KeyStore.GetKeyPair() + require.NoError(t, err) + + cert, err := x509.ParseCertificate(certData) + require.NoError(t, err) + + // Construct a signing context with one or more roots of trust. + vctx := NewDefaultValidationContext(&MemoryX509CertificateStore{ + Roots: []*x509.Certificate{cert}, + }) + + // It is important to only use the returned validated element. + // See: https://www.w3.org/TR/xmldsig-bestpractices/#check-what-is-signed + validated, err := vctx.Validate(signedElement) + require.NoError(t, err) + require.NotEmpty(t, validated) +} diff --git a/validate.go b/validate.go index 2c65ca1..4af45cd 100644 --- a/validate.go +++ b/validate.go @@ -80,25 +80,24 @@ func mapPathToElement(tree, el *etree.Element) []int { } func removeElementAtPath(el *etree.Element, path []int) bool { + if len(path) == 0 { return false } - if len(el.Child) <= path[0] { - return false - } - - childElement, ok := el.Child[path[0]].(*etree.Element) - if !ok { - return false - } + if path[0] < len(el.Child) { - if len(path) == 1 { - el.RemoveChild(childElement) - return true + if len(path) == 1 { + el.RemoveChildAt(path[0]) + return true + } + + childElement, ok := el.Child[path[0]].(*etree.Element) + if ok { + return removeElementAtPath(childElement, path[1:]) + } } - - return removeElementAtPath(childElement, path[1:]) + return false } // Transform returns a new element equivalent to the passed root el, but with From ac4a7b921c014e90e0d474c01abbe8e5a8288a68 Mon Sep 17 00:00:00 2001 From: Serge Lysenko Date: Tue, 27 Sep 2022 19:06:44 +0300 Subject: [PATCH 2/6] classify validation errors; error strings should not be capitalized (ST1005) --- etreeutils/canonicalize.go | 2 +- etreeutils/namespace.go | 6 ++-- sign.go | 2 +- tls_keystore.go | 4 +-- validate.go | 62 +++++++++++++++++++++----------------- 5 files changed, 41 insertions(+), 35 deletions(-) diff --git a/etreeutils/canonicalize.go b/etreeutils/canonicalize.go index 8437fe4..dba247b 100644 --- a/etreeutils/canonicalize.go +++ b/etreeutils/canonicalize.go @@ -31,7 +31,7 @@ func transformExcC14n(ctx, declared NSContext, el *etree.Element, inclusiveNames } visiblyUtilizedPrefixes := map[string]struct{}{ - el.Space: struct{}{}, + el.Space: {}, } filteredAttrs := []etree.Attr{} diff --git a/etreeutils/namespace.go b/etreeutils/namespace.go index baf1124..61ef762 100644 --- a/etreeutils/namespace.go +++ b/etreeutils/namespace.go @@ -2,9 +2,7 @@ package etreeutils import ( "errors" - "fmt" - "sort" "github.com/beevik/etree" @@ -302,7 +300,7 @@ func NSFindOne(el *etree.Element, namespace, tag string) (*etree.Element, error) func NSFindOneCtx(ctx NSContext, el *etree.Element, namespace, tag string) (*etree.Element, error) { var found *etree.Element - err := NSFindIterateCtx(ctx, el, namespace, tag, func(ctx NSContext, el *etree.Element) error { + err := NSFindIterateCtx(ctx, el, namespace, tag, func(_ NSContext, el *etree.Element) error { found = el return ErrTraversalHalted }) @@ -376,7 +374,7 @@ func NSFindOneChild(el *etree.Element, namespace, tag string) (*etree.Element, e func NSFindOneChildCtx(ctx NSContext, el *etree.Element, namespace, tag string) (*etree.Element, error) { var found *etree.Element - err := NSFindChildrenIterateCtx(ctx, el, namespace, tag, func(ctx NSContext, el *etree.Element) error { + err := NSFindChildrenIterateCtx(ctx, el, namespace, tag, func(_ NSContext, el *etree.Element) error { found = el return ErrTraversalHalted }) diff --git a/sign.go b/sign.go index 2be34b7..8f3996d 100644 --- a/sign.go +++ b/sign.go @@ -35,7 +35,7 @@ func NewDefaultSigningContext(ks X509KeyStore) *SigningContext { func (ctx *SigningContext) SetSignatureMethod(algorithmID string) error { hash, ok := signatureMethodsByIdentifier[algorithmID] if !ok { - return fmt.Errorf("Unknown SignatureMethod: %s", algorithmID) + return fmt.Errorf("unknown SignatureMethod: %s", algorithmID) } ctx.Hash = hash diff --git a/tls_keystore.go b/tls_keystore.go index 79a1a4d..45f479b 100644 --- a/tls_keystore.go +++ b/tls_keystore.go @@ -8,8 +8,8 @@ import ( //Well-known errors var ( - ErrNonRSAKey = fmt.Errorf("Private key was not RSA") - ErrMissingCertificates = fmt.Errorf("No public certificates provided") + ErrNonRSAKey = fmt.Errorf("private key was not RSA") + ErrMissingCertificates = fmt.Errorf("no public certificates provided") ) //TLSCertKeyStore wraps the stdlib tls.Certificate to return its contained key diff --git a/validate.go b/validate.go index 4af45cd..d50e1bd 100644 --- a/validate.go +++ b/validate.go @@ -20,8 +20,12 @@ var whiteSpace = regexp.MustCompile("\\s+") 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") + ErrMissingSignature = errors.New("missing signature referencing the top-level element") + + ErrUnsupportedMethod = errors.New("dsig: unsupported algorithm") + ErrInvalidSignature = errors.New("dsig: invalid signature") + ErrBadCertificate = errors.New("dsig: bad certificate") + ErrInvalidDigest = errors.New("dsig: digest was broken") ) type ValidationContext struct { @@ -80,7 +84,7 @@ func mapPathToElement(tree, el *etree.Element) []int { } func removeElementAtPath(el *etree.Element, path []int) bool { - + if len(path) == 0 { return false } @@ -91,7 +95,7 @@ func removeElementAtPath(el *etree.Element, path []int) bool { el.RemoveChildAt(path[0]) return true } - + childElement, ok := el.Child[path[0]].(*etree.Element) if ok { return removeElementAtPath(childElement, path[1:]) @@ -126,7 +130,7 @@ func (ctx *ValidationContext) transform( switch AlgorithmID(algo) { case EnvelopedSignatureAltorithmId: if !removeElementAtPath(el, signaturePath) { - return nil, nil, errors.New("Error applying canonicalization transform: Signature not found") + return nil, nil, fmt.Errorf("%w: error applying canonicalization transform: Signature not found", ErrInvalidSignature) } case CanonicalXML10ExclusiveAlgorithmId: @@ -158,7 +162,7 @@ func (ctx *ValidationContext) transform( canonicalizer = MakeC14N10WithCommentsCanonicalizer() default: - return nil, nil, errors.New("Unknown Transform Algorithm: " + algo) + return nil, nil, fmt.Errorf("%w: transform: %s", ErrUnsupportedMethod, algo) } } @@ -177,13 +181,13 @@ func (ctx *ValidationContext) digest(el *etree.Element, digestAlgorithmId string digestAlgorithm, ok := digestAlgorithmsByIdentifier[digestAlgorithmId] if !ok { - return nil, errors.New("Unknown digest algorithm: " + digestAlgorithmId) + return nil, fmt.Errorf("%w: digest: %s", ErrUnsupportedMethod, digestAlgorithmId) } hash := digestAlgorithm.New() _, err = hash.Write(data) if err != nil { - return nil, err + return nil, fmt.Errorf("%w: %v", ErrUnsupportedMethod, err) } return hash.Sum(nil), nil @@ -203,7 +207,7 @@ func (ctx *ValidationContext) verifySignedInfo(sig *types.Signature, canonicaliz } if signedInfo == nil { - return errors.New("Missing SignedInfo") + return fmt.Errorf("%w: missing SignedInfo", ErrInvalidSignature) } // Canonicalize the xml @@ -214,26 +218,26 @@ func (ctx *ValidationContext) verifySignedInfo(sig *types.Signature, canonicaliz signatureAlgorithm, ok := signatureMethodsByIdentifier[signatureMethodId] if !ok { - return errors.New("Unknown signature method: " + signatureMethodId) + return fmt.Errorf("%w: signature method: %s", ErrUnsupportedMethod, signatureMethodId) } hash := signatureAlgorithm.New() _, err = hash.Write(canonical) if err != nil { - return err + return fmt.Errorf("%w: %v", ErrUnsupportedMethod, err) } hashed := hash.Sum(nil) pubKey, ok := cert.PublicKey.(*rsa.PublicKey) if !ok { - return errors.New("Invalid public key") + return fmt.Errorf("%w: invalid public key", ErrBadCertificate) } // Verify that the private key matching the public key from the cert was what was used to sign the 'SignedInfo' and produce the 'SignatureValue' err = rsa.VerifyPKCS1v15(pubKey, signatureAlgorithm, hashed[:], decodedSignature) if err != nil { - return err + return fmt.Errorf("%w [%v]", ErrInvalidDigest, err) } return nil @@ -272,20 +276,21 @@ func (ctx *ValidationContext) validateSignature(el *etree.Element, sig *types.Si decodedDigestValue, err := base64.StdEncoding.DecodeString(ref.DigestValue) if err != nil { - return nil, err + return nil, fmt.Errorf("could not decode reference: %w", ErrInvalidDigest) } if !bytes.Equal(digest, decodedDigestValue) { - return nil, errors.New("Signature could not be verified") + return nil, fmt.Errorf("reference could not be verified: %w", ErrInvalidDigest) } + if sig.SignatureValue == nil { - return nil, errors.New("Signature could not be verified") + return nil, fmt.Errorf("%w: missing SignatureValue", ErrInvalidSignature) } // 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") + return nil, fmt.Errorf("could not decode signature: %w", ErrInvalidDigest) } // Actually verify the 'SignedInfo' was signed by a trusted source @@ -356,7 +361,7 @@ func (ctx *ValidationContext) findSignature(root *etree.Element) (*types.Signatu } if c14NMethod == nil { - return errors.New("missing CanonicalizationMethod on Signature") + return errors.New("missing CanonicalizationMethod") } c14NAlgorithm := c14NMethod.SelectAttrValue(AlgorithmAttr, "") @@ -383,7 +388,7 @@ func (ctx *ValidationContext) findSignature(root *etree.Element) (*types.Signatu canonicalSignedInfo = canonicalPrep(detachedSignedInfo, map[string]struct{}{}, true, true) default: - return fmt.Errorf("invalid CanonicalizationMethod on Signature: %s", c14NAlgorithm) + return fmt.Errorf("%w: canonicalization: %s", ErrUnsupportedMethod, c14NAlgorithm) } signatureEl.RemoveChild(signedInfo) @@ -398,7 +403,7 @@ func (ctx *ValidationContext) findSignature(root *etree.Element) (*types.Signatu } if !found { - return errors.New("Missing SignedInfo") + return errors.New("missing SignedInfo") } // Unmarshal the signature into a structured Signature type @@ -421,7 +426,10 @@ func (ctx *ValidationContext) findSignature(root *etree.Element) (*types.Signatu }) if err != nil { - return nil, err + if errors.Is(err, ErrInvalidSignature) || errors.Is(err, ErrUnsupportedMethod) { + return nil, err + } + return nil, fmt.Errorf("%w: %v", ErrInvalidSignature, err) } if sig == nil { @@ -444,35 +452,35 @@ func (ctx *ValidationContext) verifyCertificate(sig *types.Signature) (*x509.Cer if sig.KeyInfo != nil { // If the Signature includes KeyInfo, extract the certificate from there if len(sig.KeyInfo.X509Data.X509Certificates) == 0 || sig.KeyInfo.X509Data.X509Certificates[0].Data == "" { - return nil, errors.New("missing X509Certificate within KeyInfo") + return nil, fmt.Errorf("%w: missing X509Certificate within KeyInfo", ErrInvalidSignature) } certData, err := base64.StdEncoding.DecodeString( whiteSpace.ReplaceAllString(sig.KeyInfo.X509Data.X509Certificates[0].Data, "")) if err != nil { - return nil, errors.New("Failed to parse certificate") + return nil, fmt.Errorf("%w: failed to decode certificate: %v", ErrInvalidSignature, err) } cert, err = x509.ParseCertificate(certData) if err != nil { - return nil, err + return nil, fmt.Errorf("%w: failed to parse certificate: %v", ErrInvalidSignature, err) } } else { // If the Signature doesn't have KeyInfo, Use the root certificate if there is only one if len(roots) == 1 { cert = roots[0] } else { - return nil, errors.New("Missing x509 Element") + return nil, fmt.Errorf("%w: missing x509 Element", ErrInvalidSignature) } } // Verify that the certificate is one we trust if !contains(roots, cert) { - return nil, errors.New("Could not verify certificate against trusted certs") + return nil, fmt.Errorf("%w: could not verify against trusted certs", ErrBadCertificate) } if now.Before(cert.NotBefore) || now.After(cert.NotAfter) { - return nil, errors.New("Cert is not valid at this time") + return nil, fmt.Errorf("%w: cert is not valid at this time", ErrBadCertificate) } return cert, nil From 6382b843da3db52a5fa663bdaf043ca87d5371af Mon Sep 17 00:00:00 2001 From: Serge Lysenko Date: Fri, 23 Sep 2022 20:44:26 +0300 Subject: [PATCH 3/6] Add Manifest support according to the W3 standard --- main_test.go | 37 +++++++ sign.go | 122 +++++++++++++++++++-- sign_test.go | 50 +++++++++ types/signature.go | 7 ++ validate.go | 257 ++++++++++++++++++++++++++++++++++++--------- xml_constants.go | 10 +- 6 files changed, 419 insertions(+), 64 deletions(-) diff --git a/main_test.go b/main_test.go index 2fca4b5..0e709c3 100644 --- a/main_test.go +++ b/main_test.go @@ -1,6 +1,7 @@ package dsig import ( + "crypto" "crypto/x509" "testing" @@ -44,3 +45,39 @@ func TestDocumentedExample(t *testing.T) { require.NoError(t, err) require.NotEmpty(t, validated) } + +func TestManifestExample(t *testing.T) { + + // Generate a key and self-signed certificate for signing + randomKeyStore := RandomKeyStoreForTest() + ctx := NewDefaultSigningContext(randomKeyStore) + + test := []byte{0x45, 0xf1, 0xab, 0xd7, 0x8a, 0x6f, 0x92, 0xe6, 0xa4, 0xb6, 0x8e, 0xba, 0x8f, 0xe7, 0x91, 0x96, 0xe0, 0xb2, 0x16, 0xd6, 0x0b, 0x82, 0x1b, 0x00, 0x45, 0xfa, 0xb8, 0xad, 0xd4, 0xfa, 0xff, 0xf9} + + sig := ctx.CreateSignature("id1234") + err := ctx.AddManifestRef(sig, "package", crypto.SHA256, test) + require.NoError(t, err) + + // Sign the signature + signed, err := ctx.Sign(sig) + require.NoError(t, err) + + // Validate + _, certData, err := ctx.KeyStore.GetKeyPair() + require.NoError(t, err) + + cert, err := x509.ParseCertificate(certData) + require.NoError(t, err) + + // Construct a signing context with one or more roots of trust. + vctx := NewDefaultValidationContext(&MemoryX509CertificateStore{ + Roots: []*x509.Certificate{cert}, + }) + + // It is important to only use the returned validated element. + // See: https://www.w3.org/TR/xmldsig-bestpractices/#check-what-is-signed + manifest, err := vctx.ValidateManifest(signed) + + require.NoError(t, err) + require.NotEmpty(t, manifest) +} diff --git a/sign.go b/sign.go index 8f3996d..f46aeb4 100644 --- a/sign.go +++ b/sign.go @@ -43,6 +43,47 @@ func (ctx *SigningContext) SetSignatureMethod(algorithmID string) error { return nil } +func (ctx *SigningContext) CreateSignature(id string) *etree.Element { + + sig := &etree.Element{ + Tag: SignatureTag, + Space: ctx.Prefix, + } + + xmlns := "xmlns" + if ctx.Prefix != "" { + xmlns += ":" + ctx.Prefix + } + + sig.CreateAttr(xmlns, Namespace) + + if ctx.IdAttribute != "" && id != "" { + sig.CreateAttr(ctx.IdAttribute, id) + } + return sig +} + +func (ctx *SigningContext) AddManifestRef(sig *etree.Element, name string, hash_id crypto.Hash, digest []byte) error { + + digestAlgorithmIdentifier, ok := digestAlgorithmIdentifiers[hash_id] + if !ok { + return ErrUnsupportedMethod + } + + manifest := ctx.constructManifest(sig) + reference := ctx.createNamespacedElement(manifest, ReferenceTag) + if name != "" { + reference.CreateAttr(URIAttr, name) + } + digestMethod := ctx.createNamespacedElement(reference, DigestMethodTag) + digestMethod.CreateAttr(AlgorithmAttr, digestAlgorithmIdentifier) + + digestValue := ctx.createNamespacedElement(reference, DigestValueTag) + digestValue.SetText(base64.StdEncoding.EncodeToString(digest)) + + return nil +} + func (ctx *SigningContext) digest(el *etree.Element) ([]byte, error) { canonical, err := ctx.Canonicalizer.Canonicalize(el) if err != nil { @@ -58,6 +99,33 @@ func (ctx *SigningContext) digest(el *etree.Element) ([]byte, error) { return hash.Sum(nil), nil } +func (ctx *SigningContext) constructManifest(sig *etree.Element) *etree.Element { + + man := sig.FindElementPath(ctx.manifestPath(sig)) + + if man == nil { + object := ctx.createNamespacedElement(sig, ObjectTag) + man = ctx.createNamespacedElement(object, ManifestTag) + if ctx.IdAttribute != "" { + man.CreateAttr(ctx.IdAttribute, ManifestPrefix+sig.SelectAttrValue(ctx.IdAttribute, "")) + } + } + return man +} + +func (ctx *SigningContext) manifestPath(sig *etree.Element) etree.Path { + + if ctx.IdAttribute != "" { + val := ManifestPrefix + sig.SelectAttrValue(ctx.IdAttribute, "") + pstr := fmt.Sprintf("%s/%s[@%s='%s']", ObjectTag, ManifestTag, ctx.IdAttribute, val) + if path, err := etree.CompilePath(pstr); err == nil { + return path + } + } + path, _ := etree.CompilePath(ObjectTag + "/" + ManifestTag) + return path +} + func (ctx *SigningContext) constructSignedInfo(el *etree.Element, enveloped bool) (*etree.Element, error) { digestAlgorithmIdentifier := ctx.GetDigestAlgorithmIdentifier() if digestAlgorithmIdentifier == "" { @@ -90,6 +158,11 @@ func (ctx *SigningContext) constructSignedInfo(el *etree.Element, enveloped bool // /SignedInfo/Reference reference := ctx.createNamespacedElement(signedInfo, ReferenceTag) + // additional signature syntax + if el.Tag == ManifestTag { + reference.CreateAttr(TypeAttr, ManifestRefType) + } + dataId := el.SelectAttrValue(ctx.IdAttribute, "") if dataId == "" { reference.CreateAttr(URIAttr, "") @@ -119,22 +192,13 @@ func (ctx *SigningContext) constructSignedInfo(el *etree.Element, enveloped bool } func (ctx *SigningContext) ConstructSignature(el *etree.Element, enveloped bool) (*etree.Element, error) { + signedInfo, err := ctx.constructSignedInfo(el, enveloped) if err != nil { return nil, err } - sig := &etree.Element{ - Tag: SignatureTag, - Space: ctx.Prefix, - } - - xmlns := "xmlns" - if ctx.Prefix != "" { - xmlns += ":" + ctx.Prefix - } - - sig.CreateAttr(xmlns, Namespace) + sig := ctx.CreateSignature("") sig.AddChild(signedInfo) // When using xml-c14n11 (ie, non-exclusive canonicalization) the canonical form @@ -160,6 +224,11 @@ func (ctx *SigningContext) ConstructSignature(el *etree.Element, enveloped bool) return nil, err } + return ctx.signing(sig, sigNSCtx, signedInfo) +} + +func (ctx *SigningContext) signing(sig *etree.Element, sigNSCtx etreeutils.NSContext, signedInfo *etree.Element) (*etree.Element, error) { + // Finally detatch the SignedInfo in order to capture all of the namespace // declarations in the scope we've constructed. detatchedSignedInfo, err := etreeutils.NSDetatch(sigNSCtx, signedInfo) @@ -209,6 +278,37 @@ func (ctx *SigningContext) createNamespacedElement(el *etree.Element, tag string return child } +func (ctx *SigningContext) Sign(sig *etree.Element) (*etree.Element, error) { + + // First get the default context + rootNSCtx := etreeutils.DefaultNSContext + + // Followed by declarations on the Signature (which we just added above) + sigNSCtx, err := rootNSCtx.SubContext(sig) + if err != nil { + return nil, err + } + + man := sig.FindElementPath(ctx.manifestPath(sig)) + if err != nil { + return nil, err + } + + manifest, err := etreeutils.NSDetatch(sigNSCtx, man) + if err != nil { + return nil, err + } + + signedInfo, err := ctx.constructSignedInfo(manifest, false) + if err != nil { + return nil, err + } + + sig.AddChild(signedInfo) + + return ctx.signing(sig, sigNSCtx, signedInfo) +} + func (ctx *SigningContext) SignEnveloped(el *etree.Element) (*etree.Element, error) { sig, err := ctx.ConstructSignature(el, true) if err != nil { diff --git a/sign_test.go b/sign_test.go index febfec6..595a18f 100644 --- a/sign_test.go +++ b/sign_test.go @@ -126,3 +126,53 @@ func TestSignNonDefaultID(t *testing.T) { refURI := ref.SelectAttrValue("URI", "") require.Equal(t, refURI, "#"+id) } + +func TestSignManifest(t *testing.T) { + randomKeyStore := RandomKeyStoreForTest() + ctx := NewDefaultSigningContext(randomKeyStore) + + test := []byte {0x45, 0xf1, 0xab, 0xd7, 0x8a, 0x6f, 0x92, 0xe6, 0xa4, 0xb6, 0x8e, 0xba, 0x8f, 0xe7, 0x91, 0x96, 0xe0, 0xb2, 0x16, 0xd6, 0x0b, 0x82, 0x1b, 0x00, 0x45, 0xfa, 0xb8, 0xad, 0xd4, 0xfa, 0xff, 0xf9} + digest := []byte {0x8b, 0xba, 0x7c, 0x7d, 0xbc, 0x28, 0xab, 0x55, 0xd0, 0xf5, 0x52, 0xd3, 0xa4, 0xf1, 0xdd, 0xa6, 0x0e, 0xbf, 0xfc, 0x59, 0x59, 0x2b, 0x5e, 0xfb, 0x22, 0x02, 0xf9, 0x45, 0xfd, 0xcb, 0xdc, 0x11} + + sig := ctx.CreateSignature("id1234") + err := ctx.AddManifestRef(sig, "FirstRef", crypto.SHA256, test) + require.NoError(t, err) + + man := sig.FindElementPath(ctx.manifestPath(sig)) + require.NotNil(t, man) + + err = ctx.AddManifestRef(sig, "SecondRef", crypto.SHA256, test) + require.NoError(t, err) + + id := man.SelectAttr(ctx.IdAttribute) + require.NotEmpty(t, id) + + signed, err := ctx.Sign(sig) + require.NoError(t, err) + + signedInfo := signed.FindElement("//" + SignedInfoTag) + require.NotEmpty(t, signedInfo) + + referenceElement := signedInfo.FindElement("//" + ReferenceTag) + require.NotEmpty(t, referenceElement) + + idAttr := referenceElement.SelectAttr(URIAttr) + require.NotEmpty(t, idAttr) + require.Equal(t, "#"+id.Value, idAttr.Value) + + typeAttr := referenceElement.SelectAttr(TypeAttr) + require.NotEmpty(t, typeAttr) + require.Equal(t, "http://www.w3.org/2000/09/xmldsig#Manifest", typeAttr.Value) + + digestMethodElement := referenceElement.FindElement("//" + DigestMethodTag) + require.NotEmpty(t, digestMethodElement) + + digestMethodAttr := digestMethodElement.SelectAttr(AlgorithmAttr) + require.NotEmpty(t, digestMethodElement) + require.Equal(t, "http://www.w3.org/2001/04/xmlenc#sha256", digestMethodAttr.Value) + + digestValueElement := referenceElement.FindElement("//" + DigestValueTag) + require.NotEmpty(t, digestValueElement) + require.Equal(t, base64.StdEncoding.EncodeToString(digest), digestValueElement.Text()) + +} diff --git a/types/signature.go b/types/signature.go index 17fd3d7..d9f73fb 100644 --- a/types/signature.go +++ b/types/signature.go @@ -30,11 +30,17 @@ type DigestMethod struct { type Reference struct { XMLName xml.Name `xml:"http://www.w3.org/2000/09/xmldsig# Reference"` URI string `xml:"URI,attr"` + Type string `xml:"Type,attr"` DigestValue string `xml:"DigestValue"` DigestAlgo DigestMethod `xml:"DigestMethod"` Transforms Transforms `xml:"Transforms"` } +type Manifest struct { + XMLName xml.Name `xml:"http://www.w3.org/2000/09/xmldsig# Manifest"` + References []Reference `xml:"Reference"` +} + type CanonicalizationMethod struct { XMLName xml.Name `xml:"http://www.w3.org/2000/09/xmldsig# CanonicalizationMethod"` Algorithm string `xml:"Algorithm,attr"` @@ -78,6 +84,7 @@ type Signature struct { SignatureValue *SignatureValue `xml:"SignatureValue"` KeyInfo *KeyInfo `xml:"KeyInfo"` el *etree.Element + RootRef *Reference } // SetUnderlyingElement will be called with a reference to the Element this Signature diff --git a/validate.go b/validate.go index d50e1bd..57f5c49 100644 --- a/validate.go +++ b/validate.go @@ -2,9 +2,11 @@ package dsig import ( "bytes" + "crypto" "crypto/rsa" "crypto/x509" "encoding/base64" + "encoding/xml" "errors" "fmt" "regexp" @@ -14,7 +16,7 @@ import ( "github.com/russellhaering/goxmldsig/types" ) -var uriRegexp = regexp.MustCompile("^#[a-zA-Z_][\\w.-]*$") +var uriRegexp = regexp.MustCompile("^[/#][a-zA-Z_][\\w.-]*$") var whiteSpace = regexp.MustCompile("\\s+") var ( @@ -28,6 +30,19 @@ var ( ErrInvalidDigest = errors.New("dsig: digest was broken") ) +func wrapError(err error) error { + + if errors.Is(err, ErrMissingSignature) || + errors.Is(err, ErrUnsupportedMethod) || + errors.Is(err, ErrInvalidSignature) || + errors.Is(err, ErrBadCertificate) || + errors.Is(err, ErrInvalidDigest) { + return err + } + // by default wrap all unknow errors as invalid signature + return fmt.Errorf("%w: %v", ErrInvalidSignature, err) +} + type ValidationContext struct { CertificateStore X509CertificateStore IdAttribute string @@ -112,6 +127,10 @@ func (ctx *ValidationContext) transform( el *etree.Element, sig *types.Signature, ref *types.Reference) (*etree.Element, Canonicalizer, error) { + + if ref == nil { + return nil, nil, ErrMissingSignature + } transforms := ref.Transforms.Transforms // map the path to the passed signature relative to the passed root, in @@ -119,9 +138,6 @@ func (ctx *ValidationContext) transform( // transform signaturePath := mapPathToElement(el, sig.UnderlyingElement()) - // make a copy of the passed root - el = el.Copy() - var canonicalizer Canonicalizer for _, transform := range transforms { @@ -129,6 +145,7 @@ func (ctx *ValidationContext) transform( switch AlgorithmID(algo) { case EnvelopedSignatureAltorithmId: + el = el.Copy() // make a copy of the passed root if !removeElementAtPath(el, signaturePath) { return nil, nil, fmt.Errorf("%w: error applying canonicalization transform: Signature not found", ErrInvalidSignature) } @@ -193,7 +210,18 @@ func (ctx *ValidationContext) digest(el *etree.Element, digestAlgorithmId string return hash.Sum(nil), nil } -func (ctx *ValidationContext) verifySignedInfo(sig *types.Signature, canonicalizer Canonicalizer, signatureMethodId string, cert *x509.Certificate, decodedSignature []byte) error { +func (ctx *ValidationContext) verifySignedInfo(sig *types.Signature, cert *x509.Certificate) error { + + if sig.SignatureValue == nil { + return errors.New("missing SignatureValue") + } + + // Decode the 'SignatureValue' so we can compare against it + decodedSignature, err := base64.StdEncoding.DecodeString(sig.SignatureValue.Data) + if err != nil { + return fmt.Errorf("could not decode signature: %w", ErrInvalidDigest) + } + signatureElement := sig.UnderlyingElement() nsCtx, err := etreeutils.NSBuildParentContext(signatureElement) @@ -207,7 +235,7 @@ func (ctx *ValidationContext) verifySignedInfo(sig *types.Signature, canonicaliz } if signedInfo == nil { - return fmt.Errorf("%w: missing SignedInfo", ErrInvalidSignature) + return errors.New("missing SignedInfo") } // Canonicalize the xml @@ -216,6 +244,7 @@ func (ctx *ValidationContext) verifySignedInfo(sig *types.Signature, canonicaliz return err } + signatureMethodId := sig.SignedInfo.SignatureMethod.Algorithm signatureAlgorithm, ok := signatureMethodsByIdentifier[signatureMethodId] if !ok { return fmt.Errorf("%w: signature method: %s", ErrUnsupportedMethod, signatureMethodId) @@ -244,58 +273,27 @@ func (ctx *ValidationContext) verifySignedInfo(sig *types.Signature, canonicaliz } func (ctx *ValidationContext) validateSignature(el *etree.Element, sig *types.Signature, cert *x509.Certificate) (*etree.Element, error) { - idAttrEl := el.SelectAttr(ctx.IdAttribute) - idAttr := "" - if idAttrEl != nil { - idAttr = idAttrEl.Value - } - - var ref *types.Reference - - // Find the first reference which references the top-level element - for _, _ref := range sig.SignedInfo.References { - if _ref.URI == "" || _ref.URI[1:] == idAttr { - ref = &_ref - } - } // Perform all transformations listed in the 'SignedInfo' // Basically, this means removing the 'SignedInfo' - transformed, canonicalizer, err := ctx.transform(el, sig, ref) + transformed, canonicalizer, err := ctx.transform(el, sig, sig.RootRef) 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) + data, err := canonicalizer.Canonicalize(transformed) if err != nil { return nil, err } - decodedDigestValue, err := base64.StdEncoding.DecodeString(ref.DigestValue) - if err != nil { - return nil, fmt.Errorf("could not decode reference: %w", ErrInvalidDigest) - } - - if !bytes.Equal(digest, decodedDigestValue) { - return nil, fmt.Errorf("reference could not be verified: %w", ErrInvalidDigest) - } - - if sig.SignatureValue == nil { - return nil, fmt.Errorf("%w: missing SignatureValue", ErrInvalidSignature) - } - - // Decode the 'SignatureValue' so we can compare against it - decodedSignature, err := base64.StdEncoding.DecodeString(sig.SignatureValue.Data) + // Digest the transformed XML and compare it to the 'DigestValue' from the 'SignedInfo' + err = ctx.VerifyReference(sig.RootRef, data) if err != nil { - return nil, fmt.Errorf("could not decode signature: %w", ErrInvalidDigest) + return nil, err } // Actually verify the 'SignedInfo' was signed by a trusted source - signatureMethod := sig.SignedInfo.SignatureMethod.Algorithm - err = ctx.verifySignedInfo(sig, canonicalizer, signatureMethod, cert, decodedSignature) + err = ctx.verifySignedInfo(sig, cert) if err != nil { return nil, err } @@ -332,6 +330,7 @@ func validateShape(signatureEl *etree.Element) error { } // findSignature searches for a Signature element referencing the passed root element. +// otherwise, it returns the first found Signature in the tree, RootRef will be nil in this case func (ctx *ValidationContext) findSignature(root *etree.Element) (*types.Signature, error) { idAttrEl := root.SelectAttr(ctx.IdAttribute) idAttr := "" @@ -415,21 +414,24 @@ func (ctx *ValidationContext) findSignature(root *etree.Element) (*types.Signatu // Traverse references in the signature to determine whether it has at least // one reference to the top level element. If so, conclude the search. - for _, ref := range _sig.SignedInfo.References { - if ref.URI == "" || ref.URI[1:] == idAttr { - sig = _sig - return etreeutils.ErrTraversalHalted + for idx := range _sig.SignedInfo.References { + ref := &_sig.SignedInfo.References[idx] + if ref.URI == "" || uriRegexp.MatchString(ref.URI) { + if ref.URI == "" || ref.URI[1:] == idAttr { + sig = _sig + sig.RootRef = ref + return etreeutils.ErrTraversalHalted + } + } else { + return fmt.Errorf("%w: reference: %s", ErrUnsupportedMethod, ref.URI) } } - + sig = _sig return nil }) if err != nil { - if errors.Is(err, ErrInvalidSignature) || errors.Is(err, ErrUnsupportedMethod) { - return nil, err - } - return nil, fmt.Errorf("%w: %v", ErrInvalidSignature, err) + return nil, wrapError(err) } if sig == nil { @@ -486,6 +488,87 @@ func (ctx *ValidationContext) verifyCertificate(sig *types.Signature) (*x509.Cer return cert, nil } +func (ctx *ValidationContext) validateEx(root *etree.Element, sig *types.Signature, cert *x509.Certificate) (*types.Manifest, error) { + + var manifest *types.Manifest + + // First get the context surrounding the element we are verifying. + rootNSCtx, err := etreeutils.NSBuildParentContext(root) + if err != nil { + return nil, err + } + + // then capture any declarations on the Signature itself. + sigNSCtx, err := rootNSCtx.SubContext(sig.UnderlyingElement()) + if err != nil { + return nil, err + } + + // pass through all references of Signature + for idx := range sig.SignedInfo.References { + + var el *etree.Element + ref := &sig.SignedInfo.References[idx] + + if !uriRegexp.MatchString(ref.URI) { + return nil, fmt.Errorf("%w: reference: %s", ErrUnsupportedMethod, ref.URI) + } + // looking for referenced element + pstr := ref.URI + if pstr[0] == '#' { + pstr = fmt.Sprintf("//[@%s='%s']", ctx.IdAttribute, ref.URI[1:]) + } + if path, err := etree.CompilePath(pstr); err == nil { + el = root.FindElementPath(path) + } + if el == nil { + return nil, fmt.Errorf("could not find reference: %s", ref.URI) + } + + // detach this element from the root tree and make transformations + detached, err := etreeutils.NSDetatch(sigNSCtx, el) + if err != nil { + return nil, err + } + + // TODO: support more that one transformation + detached, canonicalizer, err := ctx.transform(detached, sig, ref) + if err != nil { + return nil, err + } + transformed, err := canonicalizer.Canonicalize(detached) + if err != nil { + return nil, err + } + + // caclculate and compare digest of referenced element + err = ctx.VerifyReference(ref, transformed) + if err != nil { + return nil, err + } + + // Process manifest reference - unmarshal it into a structured Manifest type + if ref.Type == ManifestRefType { + if manifest != nil { + return nil, errors.New("more that one manifest reference") + } + manifest = &types.Manifest{} + err = xml.Unmarshal(transformed, manifest) + if err != nil { + return nil, err + } + } + } + + // Actually verify the 'SignedInfo' was signed by a trusted source + err = ctx.verifySignedInfo(sig, cert) + if err != nil { + return nil, err + } + + return manifest, nil +} + // Validate verifies that the passed element contains a valid enveloped signature // matching a currently-valid certificate in the context's CertificateStore. func (ctx *ValidationContext) Validate(el *etree.Element) (*etree.Element, error) { @@ -504,3 +587,73 @@ func (ctx *ValidationContext) Validate(el *etree.Element) (*etree.Element, error return ctx.validateSignature(el, sig, cert) } + +// Validate verifies that the passed element contains a valid signatures +// matching a currently-valid certificate in the context's CertificateStore. +func (ctx *ValidationContext) ValidateManifest(el *etree.Element) (*types.Manifest, error) { + // Make a copy of the element to avoid mutating the one we were passed. + el = el.Copy() + + for { + + sig, err := ctx.findSignature(el) + if err != nil { + return nil, err + } + + cert, err := ctx.verifyCertificate(sig) + if err != nil { + return nil, err + } + + if sig.RootRef == nil { + manifest, err := ctx.validateEx(el, sig, cert) + if err != nil { + return nil, wrapError(err) + } + return manifest, nil + } + el, err = ctx.validateSignature(el, sig, cert) + if err != nil { + return nil, wrapError(err) + } + } +} + +// Caclculate and compare digest of referenced element +func (ctx *ValidationContext) VerifyReference(ref *types.Reference, data []byte) error { + + digestAlgorithm, ok := digestAlgorithmsByIdentifier[ref.DigestAlgo.Algorithm] + if !ok { + return fmt.Errorf("%w: digest: %s", ErrUnsupportedMethod, ref.DigestAlgo.Algorithm) + } + + decodedDigestValue, err := base64.StdEncoding.DecodeString(ref.DigestValue) + if err != nil { + return fmt.Errorf("could not decode reference: %w", ErrInvalidDigest) + } + + hash := digestAlgorithm.New() + _, err = hash.Write(data) + if err != nil { + return fmt.Errorf("%w: %v", ErrUnsupportedMethod, err) + } + + if bytes.Equal(decodedDigestValue, hash.Sum(nil)) { + return nil + } + return fmt.Errorf("reference could not be verified: %w", ErrInvalidDigest) +} + +func (ctx *ValidationContext) DecodeRef(ref *types.Reference) (crypto.Hash, []byte, error) { + + hash_id, ok := digestAlgorithmsByIdentifier[ref.DigestAlgo.Algorithm] + if !ok { + return 0, nil, fmt.Errorf("%w: digest: %s", ErrUnsupportedMethod, ref.DigestAlgo.Algorithm) + } + digest, err := base64.StdEncoding.DecodeString(ref.DigestValue) + if err != nil { + return 0, nil, fmt.Errorf("could not decode reference: %w", ErrInvalidDigest) + } + return hash_id, digest, nil +} diff --git a/xml_constants.go b/xml_constants.go index d2b98e2..e881652 100644 --- a/xml_constants.go +++ b/xml_constants.go @@ -23,13 +23,17 @@ const ( X509DataTag = "X509Data" X509CertificateTag = "X509Certificate" InclusiveNamespacesTag = "InclusiveNamespaces" + ObjectTag = "Object" + ManifestTag = "Manifest" ) const ( AlgorithmAttr = "Algorithm" + TypeAttr = "Type" URIAttr = "URI" DefaultIdAttr = "ID" PrefixListAttr = "PrefixList" + ManifestPrefix = "Package" ) type AlgorithmID string @@ -44,7 +48,11 @@ const ( RSASHA512SignatureMethod = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha512" ) -//Well-known signature algorithms +const ( + ManifestRefType = "http://www.w3.org/2000/09/xmldsig#Manifest" +) + +// Well-known signature algorithms const ( // Supported canonicalization algorithms CanonicalXML10ExclusiveAlgorithmId AlgorithmID = "http://www.w3.org/2001/10/xml-exc-c14n#" From 8b036be280f8d3341b0f588a95a43ff313b513da Mon Sep 17 00:00:00 2001 From: Serge Lysenko Date: Mon, 17 Oct 2022 11:25:03 +0300 Subject: [PATCH 4/6] Update readme examples --- README.md | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/README.md b/README.md index a775887..f6352dc 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,73 @@ func validate(root *x509.Certificate, el *etree.Element) { } ``` +### Working with Manifest + +```go +package main + +import ( + "crypto" + "github.com/beevik/etree" + "github.com/austdev/goxmldsig" + "github.com/austdev/goxmldsig/types" +) + +func main() { + // Generate a key and self-signed certificate for signing + randomKeyStore := dsig.RandomKeyStoreForTest() + ctx := dsig.NewDefaultSigningContext(randomKeyStore) + + digest := []byte{0x45, 0xf1, 0xab, 0xd7, 0x8a, 0x6f, 0x92, 0xe6, 0xa4, 0xb6, 0x8e, 0xba, 0x8f, 0xe7, 0x91, 0x96, 0xe0, 0xb2, 0x16, 0xd6, 0x0b, 0x82, 0x1b, 0x00, 0x45, 0xfa, 0xb8, 0xad, 0xd4, 0xfa, 0xff, 0xf9} + + sig := ctx.CreateSignature("id1234") + + // Get SHA256 hash of "package" data and add it as a reference + err := ctx.AddManifestRef(sig, "package", crypto.SHA256, digest) + if err != nil { + panic(err) + } + + // Sign the signature + signed, err := ctx.Sign(sig) + if err != nil { + panic(err) + } + + // Serialize the signature. + doc := etree.NewDocument() + doc.SetRoot(signed) + str, err := doc.WriteToString() + if err != nil { + panic(err) + } + + println(str) +} + +// Validate a signature against a root certificate +func validate(root *x509.Certificate, sig *etree.Element) { + // Construct a signing context with one or more roots of trust. + ctx := dsig.NewDefaultValidationContext(&dsig.MemoryX509CertificateStore{ + Roots: []*x509.Certificate{root}, + }) + + manifest, err := ctx.ValidateManifest(signed) + if err != nil { + panic(err) + } + + for idx := range manifest.References { + ref := &manifest.References[idx] + // Pass raw data of "package" for validating + err := ctx.VerifyReference(ref, test_data) + if err != nil { + panic(err) + } + } +} +``` + ## Limitations This library was created in order to [implement SAML 2.0](https://github.com/russellhaering/gosaml2) From 845f74f3e02bbef1e91d66e9cd142219e112bb67 Mon Sep 17 00:00:00 2001 From: Serge Lysenko Date: Tue, 11 Oct 2022 21:47:19 +0300 Subject: [PATCH 5/6] Remove path map manipulations into types/signature.go & fix ValidateEx() --- etreeutils/unmarshal.go | 6 ++--- main_test.go | 50 +++++++++++++++++++++++++++++++++++++ types/signature.go | 50 ++++++++++++++++++++++++++++++++++++- types/signature_test.go | 28 +++++++++++++++++++++ validate.go | 55 +++-------------------------------------- validate_test.go | 18 -------------- 6 files changed, 134 insertions(+), 73 deletions(-) create mode 100644 types/signature_test.go diff --git a/etreeutils/unmarshal.go b/etreeutils/unmarshal.go index b1fecf8..3e6e0a0 100644 --- a/etreeutils/unmarshal.go +++ b/etreeutils/unmarshal.go @@ -9,7 +9,7 @@ import ( // NSUnmarshalElement unmarshals the passed etree Element into the value pointed to by // v using encoding/xml in the context of the passed NSContext. If v implements // ElementKeeper, SetUnderlyingElement will be called on v with a reference to el. -func NSUnmarshalElement(ctx NSContext, el *etree.Element, v interface{}) error { +func NSUnmarshalElement(ctx NSContext, root, el *etree.Element, v interface{}) error { detatched, err := NSDetatch(ctx, el) if err != nil { return err @@ -29,7 +29,7 @@ func NSUnmarshalElement(ctx NSContext, el *etree.Element, v interface{}) error { switch v := v.(type) { case ElementKeeper: - v.SetUnderlyingElement(el) + v.SetUnderlyingElement(root, el) } return nil @@ -38,6 +38,6 @@ func NSUnmarshalElement(ctx NSContext, el *etree.Element, v interface{}) error { // ElementKeeper should be implemented by types which will be passed to // UnmarshalElement, but wish to keep a reference type ElementKeeper interface { - SetUnderlyingElement(*etree.Element) + SetUnderlyingElement(root, el *etree.Element) UnderlyingElement() *etree.Element } diff --git a/main_test.go b/main_test.go index 0e709c3..0838a98 100644 --- a/main_test.go +++ b/main_test.go @@ -81,3 +81,53 @@ func TestManifestExample(t *testing.T) { require.NoError(t, err) require.NotEmpty(t, manifest) } + +func TestRecursiveSigning(t *testing.T) { + + // Generate a key and self-signed certificate for signing + randomKeyStore := RandomKeyStoreForTest() + ctx := NewDefaultSigningContext(randomKeyStore) + + test := []byte{0x45, 0xf1, 0xab, 0xd7, 0x8a, 0x6f, 0x92, 0xe6, 0xa4, 0xb6, 0x8e, 0xba, 0x8f, 0xe7, 0x91, 0x96, 0xe0, 0xb2, 0x16, 0xd6, 0x0b, 0x82, 0x1b, 0x00, 0x45, 0xfa, 0xb8, 0xad, 0xd4, 0xfa, 0xff, 0xf9} + + sig := ctx.CreateSignature("id1234") + err := ctx.AddManifestRef(sig, "package", crypto.SHA256, test) + require.NoError(t, err) + + // Sign the signature + signed, err := ctx.Sign(sig) + require.NoError(t, err) + + list := &etree.Element{Tag: "Signatures"} + list.AddChild(signed) + + // create second layer + signed, err = ctx.SignEnveloped(list) + require.NoError(t, err) + + // Validate + _, certData, err := ctx.KeyStore.GetKeyPair() + require.NoError(t, err) + + cert, err := x509.ParseCertificate(certData) + require.NoError(t, err) + + // Construct a signing context with one or more roots of trust. + vctx := NewDefaultValidationContext(&MemoryX509CertificateStore{ + Roots: []*x509.Certificate{cert}, + }) + + // It is important to only use the returned validated element. + // See: https://www.w3.org/TR/xmldsig-bestpractices/#check-what-is-signed + manifest, err := vctx.ValidateManifest(signed) + + require.NoError(t, err) + require.NotEmpty(t, manifest) + require.Equal(t, len(manifest.References), 1) + + hash, digest, err := vctx.DecodeRef(&manifest.References[0]) + + require.NoError(t, err) + require.Equal(t, digest, test) + require.Equal(t, hash, crypto.SHA256) +} diff --git a/types/signature.go b/types/signature.go index d9f73fb..a0cb7a9 100644 --- a/types/signature.go +++ b/types/signature.go @@ -84,13 +84,17 @@ type Signature struct { SignatureValue *SignatureValue `xml:"SignatureValue"` KeyInfo *KeyInfo `xml:"KeyInfo"` el *etree.Element + path []int RootRef *Reference } // SetUnderlyingElement will be called with a reference to the Element this Signature // was unmarshaled from. -func (s *Signature) SetUnderlyingElement(el *etree.Element) { +func (s *Signature) SetUnderlyingElement(root, el *etree.Element) { s.el = el + // map the path to the passed signature relative to the passed root, in order + // to enable removal of the signature by an enveloped signature transform + s.path = mapPathToElement(root, el) } // UnderlyingElement returns a reference to the Element this signature was unmarshaled @@ -98,3 +102,47 @@ func (s *Signature) SetUnderlyingElement(el *etree.Element) { func (s *Signature) UnderlyingElement() *etree.Element { return s.el } + +func (s *Signature) RemoveUnderlyingElement(el *etree.Element) bool { + return removeElementAtPath(el, s.path) +} + +func mapPathToElement(tree, el *etree.Element) []int { + for i, child := range tree.Child { + if child == el { + return []int{i} + } + } + + for i, child := range tree.Child { + if childElement, ok := child.(*etree.Element); ok { + childPath := mapPathToElement(childElement, el) + if childPath != nil { + return append([]int{i}, childPath...) + } + } + } + + return nil +} + +func removeElementAtPath(el *etree.Element, path []int) bool { + + if len(path) == 0 { + return false + } + + if path[0] < len(el.Child) { + + if len(path) == 1 { + el.RemoveChildAt(path[0]) + return true + } + + childElement, ok := el.Child[path[0]].(*etree.Element) + if ok { + return removeElementAtPath(childElement, path[1:]) + } + } + return false +} diff --git a/types/signature_test.go b/types/signature_test.go new file mode 100644 index 0000000..09bd0a8 --- /dev/null +++ b/types/signature_test.go @@ -0,0 +1,28 @@ +package types + +import ( + "testing" + + "github.com/austdev/goxmldsig/etreeutils" + + "github.com/beevik/etree" + "github.com/stretchr/testify/require" +) + +func TestMapPathAndRemove(t *testing.T) { + doc := etree.NewDocument() + err := doc.ReadFromString(``) + require.NoError(t, err) + + el, err := etreeutils.NSFindOne(doc.Root(), "x", "RemoveMe") + require.NoError(t, err) + require.NotNil(t, el) + + path := mapPathToElement(doc.Root(), el) + removed := removeElementAtPath(doc.Root(), path) + require.True(t, removed) + + el, err = etreeutils.NSFindOne(doc.Root(), "x", "RemoveMe") + require.NoError(t, err) + require.Nil(t, el) +} diff --git a/validate.go b/validate.go index 57f5c49..6832f8f 100644 --- a/validate.go +++ b/validate.go @@ -79,46 +79,6 @@ func childPath(space, tag string) string { } } -func mapPathToElement(tree, el *etree.Element) []int { - for i, child := range tree.Child { - if child == el { - return []int{i} - } - } - - for i, child := range tree.Child { - if childElement, ok := child.(*etree.Element); ok { - childPath := mapPathToElement(childElement, el) - if childPath != nil { - return append([]int{i}, childPath...) - } - } - } - - return nil -} - -func removeElementAtPath(el *etree.Element, path []int) bool { - - if len(path) == 0 { - return false - } - - if path[0] < len(el.Child) { - - if len(path) == 1 { - el.RemoveChildAt(path[0]) - return true - } - - childElement, ok := el.Child[path[0]].(*etree.Element) - if ok { - return removeElementAtPath(childElement, path[1:]) - } - } - return false -} - // Transform returns a new element equivalent to the passed root el, but with // the set of transformations described by the ref applied. // @@ -133,11 +93,6 @@ func (ctx *ValidationContext) transform( } transforms := ref.Transforms.Transforms - // map the path to the passed signature relative to the passed root, in - // order to enable removal of the signature by an enveloped signature - // transform - signaturePath := mapPathToElement(el, sig.UnderlyingElement()) - var canonicalizer Canonicalizer for _, transform := range transforms { @@ -146,7 +101,7 @@ func (ctx *ValidationContext) transform( switch AlgorithmID(algo) { case EnvelopedSignatureAltorithmId: el = el.Copy() // make a copy of the passed root - if !removeElementAtPath(el, signaturePath) { + if !sig.RemoveUnderlyingElement(el) { return nil, nil, fmt.Errorf("%w: error applying canonicalization transform: Signature not found", ErrInvalidSignature) } @@ -407,7 +362,7 @@ func (ctx *ValidationContext) findSignature(root *etree.Element) (*types.Signatu // Unmarshal the signature into a structured Signature type _sig := &types.Signature{} - err = etreeutils.NSUnmarshalElement(ctx, signatureEl, _sig) + err = etreeutils.NSUnmarshalElement(ctx, root, signatureEl, _sig) if err != nil { return err } @@ -591,12 +546,10 @@ func (ctx *ValidationContext) Validate(el *etree.Element) (*etree.Element, error // Validate verifies that the passed element contains a valid signatures // matching a currently-valid certificate in the context's CertificateStore. func (ctx *ValidationContext) ValidateManifest(el *etree.Element) (*types.Manifest, error) { - // Make a copy of the element to avoid mutating the one we were passed. - el = el.Copy() for { - - sig, err := ctx.findSignature(el) + // Make a copy of the element to avoid mutating the one we were passed. + sig, err := ctx.findSignature(el.Copy()) if err != nil { return nil, err } diff --git a/validate_test.go b/validate_test.go index 7516376..7de7438 100644 --- a/validate_test.go +++ b/validate_test.go @@ -286,24 +286,6 @@ func TestValidateWithModifiedAndSignatureEdited(t *testing.T) { require.Error(t, err) } -func TestMapPathAndRemove(t *testing.T) { - doc := etree.NewDocument() - err := doc.ReadFromString(``) - require.NoError(t, err) - - el, err := etreeutils.NSFindOne(doc.Root(), "x", "RemoveMe") - require.NoError(t, err) - require.NotNil(t, el) - - path := mapPathToElement(doc.Root(), el) - removed := removeElementAtPath(doc.Root(), path) - require.True(t, removed) - - el, err = etreeutils.NSFindOne(doc.Root(), "x", "RemoveMe") - require.NoError(t, err) - require.Nil(t, el) -} - const ( validExample = `http://www.okta.com/exkrfkzzb7NyB3UeP0h7LwRDkrPmsTcUa++BIS5VJIANUlZN7zzdtjLfxfLAWds=UyjNRj9ZFbhApPhWEuVG26yACVqd25uyRKalSpp6XCdjrqKjI8Fmx7Q/IFkk5M755cxyFCQGttxThR6IPBk4Kp5OG2qGKXNHt7OQ8mumSLqWZpBJbmzNIKyG3nWlFoLVCoWPtBTd2gZM0aHOQp1JKa1birFBp2NofkEXbLeghZQ2YfCc4m8qgpZW5k/Itc0P/TVIkvPInjdSMyjm/ql4FUDO8cMkExJNR/i+GElW8cfnniWGcDPSiOqfIjLEDvZouXC7F1v5Wa0SmIxg7NJUTB+g6yrDN15VDq3KbHHTMlZXOZTXON2mBZOj5cwyyd4uX3aGSmYQiy/CGqBdqxrW2A==MIIDnjCCAoagAwIBAgIGAXHxS90vMA0GCSqGSIb3DQEBCwUAMIGPMQswCQYDVQQGEwJVUzETMBEG A1UECAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNU2FuIEZyYW5jaXNjbzENMAsGA1UECgwET2t0YTEU From 72a93fb0bdca9413d44366c4129f61ec50674e9a Mon Sep 17 00:00:00 2001 From: Serge Lysenko Date: Fri, 28 Oct 2022 16:25:40 +0300 Subject: [PATCH 6/6] Fix missing manifest error processing --- README.md | 2 +- main_test.go | 17 +++++++++++++++-- sign.go | 6 +++--- sign_test.go | 2 +- 4 files changed, 20 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index f6352dc..04fab21 100644 --- a/README.md +++ b/README.md @@ -110,7 +110,7 @@ func main() { } // Sign the signature - signed, err := ctx.Sign(sig) + signed, err := ctx.SignManifest(sig) if err != nil { panic(err) } diff --git a/main_test.go b/main_test.go index 0838a98..5eadf26 100644 --- a/main_test.go +++ b/main_test.go @@ -59,7 +59,7 @@ func TestManifestExample(t *testing.T) { require.NoError(t, err) // Sign the signature - signed, err := ctx.Sign(sig) + signed, err := ctx.SignManifest(sig) require.NoError(t, err) // Validate @@ -82,6 +82,19 @@ func TestManifestExample(t *testing.T) { require.NotEmpty(t, manifest) } +func TestMissingManifest(t *testing.T) { + + // Generate a key and self-signed certificate for signing + randomKeyStore := RandomKeyStoreForTest() + ctx := NewDefaultSigningContext(randomKeyStore) + + sig := ctx.CreateSignature("id1234") + + // Sign the signature + _, err := ctx.SignManifest(sig) + require.Error(t, err) +} + func TestRecursiveSigning(t *testing.T) { // Generate a key and self-signed certificate for signing @@ -95,7 +108,7 @@ func TestRecursiveSigning(t *testing.T) { require.NoError(t, err) // Sign the signature - signed, err := ctx.Sign(sig) + signed, err := ctx.SignManifest(sig) require.NoError(t, err) list := &etree.Element{Tag: "Signatures"} diff --git a/sign.go b/sign.go index f46aeb4..bc57a2f 100644 --- a/sign.go +++ b/sign.go @@ -278,7 +278,7 @@ func (ctx *SigningContext) createNamespacedElement(el *etree.Element, tag string return child } -func (ctx *SigningContext) Sign(sig *etree.Element) (*etree.Element, error) { +func (ctx *SigningContext) SignManifest(sig *etree.Element) (*etree.Element, error) { // First get the default context rootNSCtx := etreeutils.DefaultNSContext @@ -290,8 +290,8 @@ func (ctx *SigningContext) Sign(sig *etree.Element) (*etree.Element, error) { } man := sig.FindElementPath(ctx.manifestPath(sig)) - if err != nil { - return nil, err + if man == nil { + return nil, errors.New("missing manifest element") } manifest, err := etreeutils.NSDetatch(sigNSCtx, man) diff --git a/sign_test.go b/sign_test.go index 595a18f..5e33470 100644 --- a/sign_test.go +++ b/sign_test.go @@ -147,7 +147,7 @@ func TestSignManifest(t *testing.T) { id := man.SelectAttr(ctx.IdAttribute) require.NotEmpty(t, id) - signed, err := ctx.Sign(sig) + signed, err := ctx.SignManifest(sig) require.NoError(t, err) signedInfo := signed.FindElement("//" + SignedInfoTag)