veraison / go-cose

go library for CBOR Object Signing and Encryption (COSE)
Mozilla Public License 2.0
50 stars 26 forks source link

Cannot sign/verify digests anymore (think Merkle tree, ledger, detached payload) #120

Closed ivarprudnikov closed 1 year ago

ivarprudnikov commented 1 year ago

The implementation which allowed to sign the digests was removed in #100 in favor of passing raw content.

There are some cases when the user does not have content:

Is there a chance to extend the interfaces Signer and Verifier to allow digests again, similar to ecdsa.Sign and ecdsa.VerifyASN1?

qmuntal commented 1 year ago

Hi @ivarprudnikov. The current Signer and Verifier interfaces (as defined in #100) do allow to sign/verify pre-digested content. It's up to the implementors of those interfaces to define which hash function to use, if any.

On the other hand, the built-in signers and verifiers (provided by NewSigner and NewVerifier do use a hardcoded hash function to avoid possible misusages, i.e. using AlgorithmPS256 with crypto.SHA512 or with crypto.Hash(0).

As a matter of fact, the following a sample implementation of a Signer that uses a ecdsa.PrivateKey without hashing the content.

type ecdsaKeySigner struct {
    alg Algorithm
    key *ecdsa.PrivateKey
}

func (es *ecdsaKeySigner) Algorithm() Algorithm {
    return es.alg
}

func (es *ecdsaKeySigner) Sign(rand io.Reader, content []byte) ([]byte, error) {
    r, s, err := ecdsa.Sign(rand, es.key, digest)
    if err != nil {
        return nil, err
    }
    return encodeECDSASignature(es.key.Curve, r, s)
}

func encodeECDSASignature(curve elliptic.Curve, r, s *big.Int) ([]byte, error) {
    n := (curve.Params().N.BitLen() + 7) / 8
    sig := make([]byte, n*2)
    if err := I2OSP(r, sig[:n]); err != nil {
        return nil, err
    }
    if err := I2OSP(s, sig[n:]); err != nil {
        return nil, err
    }
    return sig, nil
}

And the following a sample implementation of a Verifier that uses a ecdsa.PublicKey without hashing the content.

type ecdsaVerifier struct {
    alg Algorithm
    key *ecdsa.PublicKey
}

func (ev *ecdsaVerifier) Algorithm() Algorithm {
    return ev.alg
}

func (ev *ecdsaVerifier) Verify(content []byte, signature []byte) error {
    r, s, err := decodeECDSASignature(ev.key.Curve, signature)
    if err != nil {
        return ErrVerification
    }
    if verified := ecdsa.Verify(ev.key, digest, r, s); !verified {
        return ErrVerification
    }
    return nil
}

func decodeECDSASignature(curve elliptic.Curve, sig []byte) (r, s *big.Int, err error) {
    n := (curve.Params().N.BitLen() + 7) / 8
    if len(sig) != n*2 {
        return nil, nil, fmt.Errorf("invalid signature length: %d", len(sig))
    }
    return OS2IP(sig[:n]), OS2IP(sig[n:]), nil
}
ivarprudnikov commented 1 year ago

do use a hardcoded hash function to avoid possible misusage

Yet the user needs to implement similar:

func decodeECDSASignature(curve elliptic.Curve, sig []byte) (r, s *big.Int, err error) {
    n := (curve.Params().N.BitLen() + 7) / 8
    if len(sig) != n*2 {
        return nil, nil, fmt.Errorf("invalid signature length: %d", len(sig))
    }
    return OS2IP(sig[:n]), OS2IP(sig[n:]), nil
}

This is what I want to avoid as a user of the library and to make it more user friendly for other use cases.

qmuntal commented 1 year ago

Yet the user needs to implement similar:

I see that those ECDSA encode/decode functions are a little bit tricky to implement right.

This is what I want to avoid as a user of the library and to make it more user friendly for other use cases.

Could you clarify which other use cases?

We could expose a new function, e.g. NewSignerUnhashed, that generates built-in signers/verifiers that don't hash the message content. I'm afraid that people will get confused with NewSigner and choose the wrong one, so I want to understand if this is a niche request or not.

ivarprudnikov commented 1 year ago

Could you clarify which other use cases?

It was a reference to what I wrote in the main issue at the top.

Is there a reason not to expose encodeECDSASignature and decodeECDSASignature?

qmuntal commented 1 year ago

Is there a reason not to expose encodeECDSASignature and decodeECDSASignature?

Those functions are an implementation detail, not part of the cose spec. IMO if we finally support sign/verify digested payloads it should by via NewSignerUnhashed, not exposing more internals.

SteveLasker commented 1 year ago

@ivarprudnikov, @shizhMSFT, what do you think about NewSignerUnhashed ^ from @qmuntal?

ivarprudnikov commented 1 year ago

I think the approach works but the name sounds odd and slightly confusing. It is a difficult one to nail it though, maybe NewSignerWithOptions might be a better way to open up configuration and make it a bit future proof? Or do something like NewNonHashingSigner

shizhMSFT commented 1 year ago

I'm actually confused with the goal of this issue.

According to RFC 9052 section 4.4, the canonical CBOR encoded Sig_structure is the only message passed to the Signer or the Verifier.

ivarprudnikov commented 1 year ago

@shizhMSFT if the idea is to stick to the letter then there is another thing not strictly related to pure cose but to other ecosystem components like receipts where a cose signature is being countersigned. In that case the toBeSigned structure is created outside of this library but only the signer is being used to create a signature which is of the same encoding. The value being in having a consistent way to sign other related structures and not only a cose one. So if the developer is creating a library that interops with cose she could rely on one consistent signer/verifier interface.

shizhMSFT commented 1 year ago

Thank @ivarprudnikov for the elaboration.

The scenario you've mentioned is not a typical COSE signing or verification process. Thus, proposing something like NewSignerWithOptions or NewNonHashingSigner will confuse existing COSE users.

Instead of having a new constructor, how about having the following interfaces:

type DigestSigner interface {
    // Algorithm returns the signing algorithm associated with the private key.
    Algorithm() Algorithm

    // SignDigest signs message digest with the private key, possibly using
    // entropy from rand.
    // The resulting signature should follow RFC 8152 section 8.
    SignDigest(rand io.Reader, digest []byte) ([]byte, error)
}

type DigestVerifier interface {
    // Algorithm returns the signing algorithm associated with the public key.
    Algorithm() Algorithm

    // VerifyDigest verifies message digest with the public key, returning nil
    // for success.
    // Otherwise, it returns ErrVerification.
    VerifyDigest(digest, signature []byte) error
}

Then let ecdsaKeySigner implement DigestSigner and let ecdsaVerifier implement DigestVerifier.

The example code can be

signer, err := cose.NewSigner(cose.AlgorithmES256, privateKey)
if err != nil {
    return nil, err
}
digestSigner, ok := signer.(DigestSigner)
if ok {
    sig, err := digestSigner.SignDigest(rand.Reader, digest)
    // further process
} else {
    // digest signing is not supported. e.g. ed25519
}
ivarprudnikov commented 1 year ago

@shizhMSFT agree, looks good. But is the suggestion to implement SignDigest and VerifyDigest in each rsaSigner, ecdsaCryptoSigner, ed25519Signer?

shizhMSFT commented 1 year ago

@ivarprudnikov Yes for rsaSigner and ecdsaCryptoSigner but no for ed25519Signer since ed25519 cannot sign a digest.