Checking version after verifying signatures #112
Conversation
erickt
force-pushed
the
version-after-sig
branch
from
2f8ab29 to
b09e018
Compare
There was a problem hiding this comment.
Making the spec more explicit about these steps to mitigate the impact of parser bugs is a solid change. Thanks @erickt!
I'm curious to understand whether you are doing something differently to POUF-1 in rust-tuf/go-tuf that makes (1) less of a problem for them?
|
Thanks @joshuagl! As of right now, rust-tuf can't do much to avoid the parsing issues with POUF-1. We're relying on Rust and |
There was a problem hiding this comment.
LGTM. It looks like the reference implementation already checks the signature before the version in _update_metadata
|
I cut a bunch of patch releases today that conflict with this PR. I have started to rebase this PR on master, resolving conflicts (and squashing commits). But the pending #119, which is ready to be merged, will create further conflicts. I suggest to first land #119 and then this PR. I'm happy to help resolving the new conflicts. |
tuf-spec.md
Outdated
| @@ -1188,35 +1188,40 @@ VERSION_NUMBER is the version number of the snapshot metadata file listed in | |||
| the timestamp metadata file. In either case, the client MUST write the file to | |||
| non-volatile storage as FILENAME.EXT. | |||
|
|
|||
| * **3.1**. **Check against timestamp metadata.** The hashes and version | |||
| number of the new snapshot metadata file MUST match the hashes (if any) and | |||
There was a problem hiding this comment.
I think we should keep the "if any", because snapshot's hashes in timestamp are still optional (see #90).
There was a problem hiding this comment.
Ah I didn't realize the snapshot hash in timestamp was made optional. I'll restore this language after #119 lands.
There was a problem hiding this comment.
Just rebased on top of master (post-#119) and restored the "if any" in 5.3.1.
tuf-spec.md
Outdated
|
|
||
| * **4.2**. **Check for an arbitrary software attack.** The new targets | ||
| metadata file MUST have been signed by a threshold of keys specified in the | ||
| trusted root metadata file. If the new targets metadata file is not signed | ||
| as required, discard it, abort the update cycle, and report the failure. | ||
|
|
||
| * **4.3**. **Check for a freeze attack.** The latest known time should be | ||
| * **4.3**. **Check against snapshot role's targets version.** The version |
There was a problem hiding this comment.
This is to check for a mix-and-match or rollback attack.
There was a problem hiding this comment.
Do you want to add this here again?
There was a problem hiding this comment.
Yeah, I think just to say why we're doing this, because we try to do it for other checks.
lukpueh
force-pushed
the
version-after-sig
branch
from
c2ec667 to
c860dca
Compare
In section 5.3.1 and section 5.4.1 of the spec, the first part of verifying a new snapshot, targets, and delegated targets role requires checking the new role's version number before we've checked that these new roles were signed by the root role. This exposes TUF clients to potential parser bugs which allows an attacker to potentially compromise the system. For example, consider a man-in-the-middle attacker that has a parser bug which allows for executing arbitrary code, such as [CVE-2017-18349]. An attacker could exploit this in a few manners: 1. [POUF-1] inlines the metadata signatures in with the role JSON metadata file. While snapshot, targets, and delegated targets can have their hashes listed in trusted metadata (and thus be validated before parsing), the timestamp role must be parsed in order to verify it was signed by the trusted root role. 2. TUF-1.0.5 Section 5.3.1 of the workflow states the snapshot should be verified by hash and version number before checking the signature. While we could verify the hash before parsing, the spec doesn't state that the hash should be checked first. It's possible then for a conformant TUF client to check the version number before hash, which would expose the client to the parser bug. 3. TUF-1.0.5 Section 4.4 and 5.4.1 make it optional for the snapshot role to contain the hashes of the targets and any delegated targets role. If left out, we need to still parse these files to check the version number before validating the signatures. This patch addresses (2) and (3) by moving the verification of the version number to after we've verified a role was properly signed by the trusted root role. This would enable a future POUF that's addressed (1) to avoid future parser exploits. [POUF-1]: https://github.com/theupdateframework/taps/blob/master/POUFs/reference-POUF/pouf1.md [CVE-2017-18349]: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-18349
lukpueh
force-pushed
the
version-after-sig
branch
from
c860dca to
6dab211
Compare
|
Just rebased on master, fixing conflicts and restoring the optionality of snapshot hashes in timestamp. The diff to @erickt's original patch, including all the meanwhile merged PRs, can be found here. From my perspective this is good to go, but please double-check! |
Looks good to me. |
In section 5.3.1 and section 5.4.1 of the spec, the first part of verifying a new snapshot, targets, and delegated targets role requires checking the new role's version number before we've checked that these new roles were signed by the root role. This exposes TUF clients to potential parser bugs which allows an attacker to potentially compromise the system.
For example, consider a man-in-the-middle attacker that has a parser bug which allows for executing arbitrary code, such as CVE-2017-18349. An attacker could exploit this in a few manners:
This patch addresses (2) and (3) by moving the verification of the version number to after we've verified a role was properly signed by the trusted root role. This would enable a future POUF that's addressed (1) to avoid future parser exploits.