sigstore / sigstore-go

Go library for Sigstore signing and verification
Apache License 2.0
46 stars 21 forks source link

SCT verification should compare timestamp against CT log validity window #178

Open haydentherapper opened 5 months ago

haydentherapper commented 5 months ago

Description

As a reminder, the purpose of validity windows is to mitigate two risks:

  1. An old signing key for a service is compromised and produces an artifact after the service is turned down
  2. A service that was turned down is brought back online and issues a cert or proof after it's been marked as rotated

I've been looking over the code and wanted to confirm if we are verifying validity windows for root metadata. Here's what I've found so far:

In terms of how to structure the code, as is currently implemented, each service (CA, CT log, Rekor, TSA) should be responsible for comparing any of its artifacts that contain timestamps against the validity windows. TODOs below:

The metadata I don't know how to verify is if a log proof was created during a log's validity window, since there is no timestamp. Using a certificate's timestamp is not accurate because a certificate may be logged long after a signing event. In the case of BYO PKI, there may also be no timestamps, if you log a signing event with a key. I think there's nothing to compare against in these cases.

codysoyland commented 5 months ago

Thank you for documenting these!

TSA [Potential bug] There is no comparison for the intermediate and root certificates.

I think this should be covered if I understand correctly. If you follow the code paths from tsaverification.VerifyTimestampResponse, it does eventually hit https://github.com/digitorus/pkcs7/blob/master/verify.go#L204, which uses the TSR timestamp as the CurrentTime when verifying the cert chain, which will verify that the TSR time is within the validity period of every cert in the chain.

I'll continue to look at some of these other potential bugs and comment individually if I find anything.

codysoyland commented 5 months ago

Fulcio [Potential bug] There is no comparison for the intermediate and root certificates.

Again, the CurrentTime is passed to cert.Verify, therefore the entire chain is checked. In this case, the leaf cert timestamps are used as the "current time" for the purpose of checking the validity period.

Edit: I would be in favor of changing this comparison to use observerTimestamp instead of the leaf cert times for clarity here. This block was written before observerTimestamp was an input to this func.

haydentherapper commented 5 months ago

Good find. Talking out loud to convince myself there's no issue:

So yea, TSA seems all good.

Edit: I would be in favor of changing this comparison to use observerTimestamp instead of the leaf cert times for clarity here. This block was written before observerTimestamp was an input to this func.

Good call, agreed.

haydentherapper commented 5 months ago

@codysoyland, I believe "CT log compares against issued SCTs" is still missing, so I've updated the title accordingly.