Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
5 of 6 tasks
haydentherapper opened this issue May 16, 2024 · 4 comments
Open
5 of 6 tasks
Labels
enhancement New feature or request v1.0 items we want to consider for a v1.0 release

Comments

@haydentherapper
Copy link
Contributor

haydentherapper commented May 16, 2024

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:

  • TSA compared against issued signed timestamps
  • TSA compared for each certificate in its chain
  • Rekor compared against issued SETs
  • CA compared against issued leaf certificates
  • CA compared for the root and intermediate CA certificates
  • CT log compares against issued SCTs

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.

@haydentherapper haydentherapper added the enhancement New feature or request label May 16, 2024
@codysoyland
Copy link
Member

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
Copy link
Member

codysoyland commented May 16, 2024

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
Copy link
Contributor Author

haydentherapper commented May 16, 2024

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

  • Let's imagine time exists from 0 to 100
  • The TSA root and intermediate are valid from 10 - 90
  • The TSA leaf is valid from 20 - 80
  • The TSA service is marked as valid from 30 - 40
  • A signed timestamp is issued at time 35
    • We check that the signed timestamp's time is during the service validity currently
    • To verify the CA chain, current time comes from the signed timestamp, which verifies it falls during the each cert's validity window
  • A misconfigured service issues a timestamp is issued at 50
    • This would fail the current check since the signed timestamp was issued outside the service's validity window

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
Copy link
Contributor Author

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

@haydentherapper haydentherapper changed the title Verifying validity windows SCT verification should compare timestamp against CT log validity window May 21, 2024
@haydentherapper haydentherapper added the v1.0 items we want to consider for a v1.0 release label Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v1.0 items we want to consider for a v1.0 release
Projects
None yet
Development

No branches or pull requests

2 participants