fix(verify): Fix a vulnerability in the verification of threshold signatures (due to handling of keys with multiple IDs)

[cedricvanrompay-datadog]

Release Notes: Fixes a vulnerability in the verification of threshold signatures (due to handling of keys with multiple IDs)

Types of changes:

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of the changes being introduced by the pull request:

Go-TUF has a security vulnerability in the way it validates threshold signatures: it relies on the key IDs declared in the metadata file it is currently validating (varibale sig.KeyID) to tell if this new signature should count towards the threshold, assuming that these IDs will be the same as the one it generates (verifier.MarshalPublicKey().IDs()). This assumption can be made false and this can be leveraged to forge valid threshold signatures using a number of keys which, in theory, should be insufficient.

This PR adds a test demonstrating the vulnerability, then it fixes the vulnerability.

My opinion however is that Go-TUF should stop trying to be compatible with old versions of TUF implementations that were assigning several IDs to a same key, as this makes signature validation way too error-prone. Note that all versions of the TUF specification since version 1 are very clear on there being a single correct way to compute the key ID:

KEYID: The identifier of the key signing the ROLE object, which is a hexdigest of the SHA-256 hash of the canonical form of the key. The keyid MUST be unique in the "signatures" array: multiple signatures with the same keyid are not allowed.

source: https://theupdateframework.github.io/specification/v1.0.29/index.html#role-keyid

KEYID: A KEYID, which MUST be correct for the specified KEY. Clients MUST calculate each KEYID to verify this is correct for the associated key. Clients MUST ensure that for any KEYID represented in this key list and in other files, only one unique key has that KEYID.

source: https://theupdateframework.github.io/specification/v1.0.29/index.html#root-keyid

Also, I am aware that TAP 12 (which does not apply to TUF 1.0.x anyway) introduces key ID flexibility. A better solution than the one in this PR would be to apply the recommendation in TAP 12 to "use a standardized representation for keys, like the modulus and exponent for RSA or the point and curve for ECC". That's probably something we should do in Go-TUF without waiting for TUF 1.1.

Please verify and check that the pull request fulfills the following requirements:

• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature

[cedricvanrompay-datadog]

I realise I should probably have shared this privately with the maintainers instead of sending a public PR, apologies.

[trishankatdatadog]

Thanks for reporting this. I had found a similar issue for python-tuf before, but unfortunately we never got around to publishing it, which may explain why no other implementations had noticed.

It looks good on a cursory glance, but let us review and merge ASAP.

[mnm678]

I'll do a more detailed review, but it looks like this needs DCO and conventional commits

I realise I should probably have shared this privately with the maintainers instead of sending a public PR, apologies.

Yes, for future reference, but thanks for sharing this. We'll work to improve our security reporting docs to make the private reporting process more clear in the future.

[mnm678]

LGTM. I'll approve pending the failing checks

[trishankatdatadog]

LGTM, @cedricvanrompay-datadog, @ethan-lowman-dd, and I discussed this over Zoom, thanks!