Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sign1 messages with a non-minimal protected header length cannot be verified #119

Closed
plietar opened this issue Dec 7, 2022 · 1 comment · Fixed by #135
Closed

Sign1 messages with a non-minimal protected header length cannot be verified #119

plietar opened this issue Dec 7, 2022 · 1 comment · Fixed by #135
Assignees
Milestone

Comments

@plietar
Copy link

plietar commented Dec 7, 2022

The COSE spec mandates that the TBS (ie. the Sig_structure) be canonically encoded, eg. all the lengths must be encoded as short as possible. For example, a 3-byte protected header { alg: ES256 }, within the TBS, will be encoded as follows:

[...]
43       # Byte string of length 3
A1 01 26 # Contents of the protected header
[...]

(The actual contents don't need to be canonically encoded, only the length does. I'm also omitting the rest of the TBS here)

However, there is no such requirement on the overall envelope. This means that when it comes to serializing this message, using a non-optimal encoding for the length is allowed, for example:

[...]
58       # Byte string, with a uint8 length
03       # Length of the byte string
A1 01 26 # Contents of the protected header
[...]

If a message with this non-optimal encoding is received, the verifier should make sure to re-encode the length to be as short as possible.

This is true of all the fields of the TBS, ie. both the payload and the protected header. While go-cose correctly processes the payload, by decoding it into a []byte and discarding the original encoding of the length, the entire encoding of the protected header is preserved, as a cbor.RawMessage. The encoding of the length of the header leaks into the TBS, which may violate the requirement that the TBS use canonical encoding.


The following is a small reproduction of the problem. The decanonicalize function will modify any encoded message to switch to a non-optimal encoding (assuming the length of the protected header would have been short enough to fit in the initial byte).

The call the Verify() at the end should succeed, but returns a "verification error".

import (
  "bytes"
  "crypto/ecdsa"
  "crypto/elliptic"
  "crypto/rand"
  "github.com/fxamacker/cbor/v2"
  "github.com/veraison/go-cose"

  _ "crypto/sha256"
)

func decanonicalize(data []byte) []byte {
  var msg struct {
    _           struct{} `cbor:",toarray"`
    Protected   []byte
    Unprotected interface{}
    Payload     []byte
    Signature   []byte
  }

  err := cbor.Unmarshal(data, &msg)
  if err != nil { panic(err) }

  buf := new(bytes.Buffer)

  // Sign1 Tag + Start array of 4 elements
  buf.Write([]byte{ 0xd2, 0x84 });

  // Assuming `len(msg.Protected) < 24`, this is non-optimal.
  // The most concise encoding is `0x40 + len(msg.Protected)`
  buf.Write([]byte{ 0x58, byte(len(msg.Protected)) });
  buf.Write(msg.Protected)

  // Encode the rest into the buffer
  encoder := cbor.NewEncoder(buf)
  encoder.Encode(msg.Unprotected)
  encoder.Encode(msg.Payload)
  encoder.Encode(msg.Signature)

  return buf.Bytes()
}

func main() {
  alg := cose.AlgorithmES256

  key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
  if err != nil { panic(err) }

  signer, err := cose.NewSigner(alg, key)
  if err != nil { panic(err) }

  verifier, err := cose.NewVerifier(alg, key.Public())
  if err != nil { panic(err) }

  msg := &cose.Sign1Message{
    Headers: cose.Headers{
      Protected: cose.ProtectedHeader {
        cose.HeaderLabelAlgorithm: cose.AlgorithmES256,
      },
    },
    Payload: []byte("hello"),
  }

  err = msg.Sign(rand.Reader, nil, signer)
  if err != nil { panic(err) }

  data, err := msg.MarshalCBOR()
  if err != nil { panic(err) }

  data = decanonicalize(data)

  var decoded cose.Sign1Message
  err = decoded.UnmarshalCBOR(data)
  if err != nil { panic(err) }

  err = decoded.Verify(nil, verifier)
  if err != nil { panic(err) }
}
@SteveLasker SteveLasker added this to the v1.1.0 milestone Feb 10, 2023
@shizhMSFT
Copy link
Contributor

This is an interesting finding that CBOR Encoding Restrictions only applies to the Sig_structure, the Enc_structure, and the MAC_structure. The COSE_Sign and COSE_Sign1 structures have no restrictions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants