specification icon indicating copy to clipboard operation
specification copied to clipboard

Change data structure for signatures to use key IDs as dictionary keys

Open davidstrauss opened this issue 4 years ago • 4 comments

Because signatures (at least in the preferred ed25519 mode) are deterministic products of the key and payload materials, it doesn't seem like there's a use case for more than one signature from the same key ID.

Meanwhile -- and more concerning -- we've now had two implementations that have mistakenly used vulnerable signature threshold checking that could have been avoided by using a more robust data format that intrinsically prevents this attack.

I'm not sure how to make this change in a way that's both clean and backwards-compatible, but I'd even be satisfied getting this change into the hopper for TUF 2.0, even if that's a ways off.

Old:

 "signatures": [
  {
   "keyid": "1bf1c6e3cdd3d3a8420b19199e27511999850f4b376c4547b2f32fba7e80fca3",
   "sig": "90d2a06c7a6c2a6a93a9f5771eb2e5ce0c93dd580bebc2080d10894623cfd6eaed
           f4df84891d5aa37ace3ae3736a698e082e12c300dfe5aee92ea33a8f461f02"
  }...

New:

 "signatures": [
    "1bf1c6e3cdd3d3a8420b19199e27511999850f4b376c4547b2f32fba7e80fca3":
  {
   "sig": "90d2a06c7a6c2a6a93a9f5771eb2e5ce0c93dd580bebc2080d10894623cfd6eaed
           f4df84891d5aa37ace3ae3736a698e082e12c300dfe5aee92ea33a8f461f02"
  }...

I'm assuming there may be future signatures that aren't contained in a single value and therefore can't simply have this structure:

 "signatures": {
    "1bf1c6e3cdd3d3a8420b19199e27511999850f4b376c4547b2f32fba7e80fca3": 
    "90d2a06c7a6c2a6a93a9f5771eb2e5ce0c93dd580bebc2080d10894623cfd6eaed
     f4df84891d5aa37ace3ae3736a698e082e12c300dfe5aee92ea33a8f461f02"
   ...

davidstrauss avatar Apr 15 '21 21:04 davidstrauss

This is a good idea, it's always nice to make implementation mistakes harder. It might also be worth cross-posting to the sigining-spec project, which is exploring some options for the signature wrapper more generally (cc @adityasaky)

Unfortunately, as you mention it would break backwards compatibility. There are a few TAPs in draft stage right now that would also require a major version change, so maybe we can prioritize some of them for a 2.0.0 release? At the very least I think this warrants some discussion (and maybe a community meeting).

mnm678 avatar Apr 15 '21 22:04 mnm678

Thanks Marina!

Here's how the signing-spec currently defines key IDs.

https://github.com/secure-systems-lab/signing-spec/blob/master/protocol.md#signature-definition

KEYID: Optional, unauthenticated hint indicating what key and algorithm was used to sign the message. As with Sign(), details are agreed upon out-of-band by the signer and verifier. It MUST NOT be used for security decisions; it may only be used to narrow the selection of possible keys to try.

As I understand it, it puts the onus on implementations to handle this correctly. I may be mistaken though.

I'm assuming there may be future signatures that aren't contained in a single value and therefore can't simply have this structure

GPG signatures from securesystemslib currently include an other_headers field.

At the very least I think this warrants some discussion (and maybe a community meeting).

Is there one scheduled? Could we also get some thoughts on the current version of signing-spec from folks there? :smile:

adityasaky avatar Apr 16 '21 01:04 adityasaky

I agree with this (as you can see in python-tuf I'd like to internally handle signatures as a dict for same reasons).

New:

 "signatures": [
    "1bf1c6e3cdd3d3a8420b19199e27511999850f4b376c4547b2f32fba7e80fca3":
  {
   "sig": "90d2a06c7a6c2a6a93a9f5771eb2e5ce0c93dd580bebc2080d10894623cfd6eaed
           f4df84891d5aa37ace3ae3736a698e082e12c300dfe5aee92ea33a8f461f02"
  }...

Just checking: I believe you mean for the outermost object to be a dictionary:

New:

"signatures": {
    "1bf1c6e3cdd3d3a8420b19199e27511999850f4b376c4547b2f32fba7e80fca3": {
        "sig": "90d2a06c7a6c2a6a93a9f5771eb2e5ce0c93dd580bebc2080d10894623cfd6eaedf4df84891d5aa37ace3ae3736a698e082e12c300dfe5aee92ea33a8f461f02"
    }, ...
}

jku avatar Jun 02 '21 12:06 jku

Just checking: I believe you mean for the outermost object to be a dictionary

Correct. I may have made an error in how I edited the structure by hand.

davidstrauss avatar Jun 03 '21 02:06 davidstrauss