repository_tool: Root pre-verification during metadata writes misses possible verification failures

[awwad]

Summary and Impact

This is a bug that occurs when root metadata is being verified before being written. This pre-write verification is intended to prevent the writing of metadata that would not be verifiable by a client. The code is flawed, and so it is readily possible to produce root metadata that will not be verifiable by a client.

Note that this bug does not result in incorrect client verification behavior, and so is not strictly a security issue. Problems can result from this behavior, however.

Background and Details

When writing new metadata (e.g. using repository_tool.write*), the write stack employs repository_lib._generate_and_write_metadata().

_generate_and_write_metadata:

1. signs the metadata based on what keyids are noted in roledb._roledb_dict[<repo>][<role>]['signing_keyids']
2. checks to make sure that the resulting signed metadata is correct / can be verified
3. writes the metadata file

There is a substantial bug in step 2 that results in this verification check failing to catch root metadata that a client might reject. Note that the way that step 1 works will be altered by #846 (alters internal metadata storage to match the written metadata format) and that the bug in step 2 will also be fixed by #846. It may be worth fixing this bug in parallel, however, and it must in any case be recorded.

Root metadata is a bit special. Any other top-level metadata can be verified by simply looking at the contemporaneous root metadata. Root metadata itself dictates the keys expected for verification of root metadata, however, and that means that (e.g.) clients will use version 5 of root to verify version 6 of root. To generalize: in order to verify version V of root, you must verify that it has good signatures from BOTH:

• at least the threshold and keyids for root from version V-1 of root
• at least the threshold and keyids for root from version V of root (i.e. the version being verified)

We don't want users of repository_tool to write metadata that can't be verified, so we check that; however, we do the verification against expected keyids & threshold from root version V-1 incorrectly. In particular, when we use repository_tool to update root's expectations of root (using repository_tool.add_verification_key(), repository_tool.remove_verification_key(), and repository_tool.threshold()), old values are saved in roledb._roledb_dict[<repo>][<role>]['previous_threshold'] and ...['previous_keyids'], but:

• remove_verification_key doesn't note prior values in those variables
• add_verification_key and threshold overwrite what they last added to those variables, resulting in the wrong values if e.g. you add two verification keys between one root version and the next, or if (much less likely) you switch the threshold value twice before writing.

The Fix:

• If there's already a value for ...['previous_keyids'], don't overwrite it. The same goes for `...['previous_threshold'].
• Empty the values on write.

#846 should also result in a fix to this, with slightly different properties, as those values will be moved.

[lukpueh]

remove_verification_key doesn't note prior values in those variables

Which is what's expected, right?

And shouldn't the same count for add_verification_key and threshold? Shouldn't we rather take the previous keys and thresholds from the factual previous root file, i.e. V-1.root instead of inferring them based on when someone added or removed verification keys or modified the threshold for the current root file, i.e. V.root?

• Empty the values on write.

Same as above goes for write. IIUC not every write increments the version number, hence it should not generally affect the book keeping of current and previous keys. And even if a write always increments the version number, I think it is better to just keep track of the "current version" and retrieve previous and current keys based on that version.

Am I missing something?

[awwad]

It's possible to go that way (referring to the prior trusted root file). If we were talking about the client, that is certainly the direction we'd go (and do go btw; client preserves current and previous metadata).

It's less clear to me that when working with repository_tool, that is the right approach -- but if you/we dig in a bit and find that reasonable, that's fine.

It should be noted that the issue above describes the current scheme (noting changes when they're made) and how it is broken and how to make it work without a paradigm change.

You could rebuild it a bit by developing a sense of which written metadata files are the preceding finished files, but this could be slightly trickier than the straight fixes.

[awwad]

As for your specific question about remove_verification_key: it is important to be able to assess whether a particular set of signatures fulfills vX and vX-1's root constraints. If a key being used to reach threshold is no longer listed in vX, it is important to still know that it counts towards threshold for vX-1's constraints.

If this doesn't clear things up, I'll provide a detailed example, so LMK.

[jku]

Closing this issue as it was filed against (what is now known as) the legacy codebase: issue seems to not be relevant anymore. Please re-open or file a new issue if you feel that the issue is revelant to current python-tuf.

Specifically on validation before writing:

• we'll have a validation feature (Add a "validate" argument option to JSONSerializer  #1775) for metadata serialization: this will prevent writing metadata that cannot be deserialized or metadata that changes during the serialize-deserialize cycle -- situations that should not happen but in a language like Python could happen from a programming error
• we do not have validation that ensures the metadata is signed before writing as that is not a job for Metadata API but the (still to appear) repository tooling. We do have easy to use API that handles the checks though: Metadata.verify_delegate()

More details

Current source code (and upcoming 1.0 release) only contains the modern components

• a low-level Metadata API (tuf.api) and
• tuf.ngclient that implements the client workflow,

Legacy components (e.g. tuf.client, tuf.repository_tool, tuf.repository_lib as well as the repo and client scripts) are no longer included. See announcement and API reference for more details.