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

Discussion on providing digest/alg for artifact verification #444

Open
kommendorkapten opened this issue Dec 13, 2024 · 3 comments
Open

Comments

@kommendorkapten
Copy link
Member

Originally the verification options supported two options:

  1. Verify a DSSE envelope
  2. Verify a message message signature + an artifact

This was changed in #406 to be able to support some cases in the conformance tests when verifying against a message digest.

Having this as part of the standard verification path have me wondering if this is really a good idea. The first is that this does not seem to be compatible with Sigstore-js and node.js verify implementation which requires the complete message to verify. Pulling in a dependency for crypto verification is something we would like to avoid I think.

Also, this is not compliant with certain algorithms such as Ed25519 (this will fail today with Sigstore-go if an artifact digest is provided), LMS, ML-DSA and SLH-DSA to name a few. We haven't started to talk about PQC yet, but at some point we need too. All of the listed algorithms have pre-hash version, but I can't guess how popular/common those will be.

My third concern is about API security. Crypto APIs should make it hard to do the wrong thing, and having the Sigstore verification APIs accept a message digest is not optimal. Providing an artefact is more secure as we can guarantee in the Sigstore client that we are verifying against the expected digest. Low level APIs such as Go's ecdsa.VerifyASN1 is an example where providing the digest makes sense, but for higher level APIs it can be a risk.

When providing a bundle with a message signature, we explicitly forbid against using the (untrusted) digest for signature verification: https://github.com/sigstore/protobuf-specs/blob/main/protos/sigstore_common.proto#L119

But as we are allowing digests for verification, I can see how this can be misused. Although not the same vulnerability but similar is CVE-2022-36056 in Cosign, where the content of a Rekor entry wasn't verified to actually match the artifact in question.

Proposal

I can understand that it's desirable from the conformance test suite to have this feature, but for regular verification I would like to see an comment about that this is not recommended and an optional feature (it breaks Ed25519 today).
Possibly even rename the policy option https://github.com/sigstore/sigstore-go/blob/main/pkg/verify/signed_entity.go#L444 to WithArtifactDigestUnsafe similar to when no signer identity is provided.

If we also make this an optional feature, how could this be expressed in the conformance test suite?

cc @woodruffw @haydentherapper @bdehamer

@woodruffw
Copy link
Member

Thanks for opening this @kommendorkapten!

I think the situation here is pretty (unfortunately, IMO) nuanced, since Sigstore clients really have two (or more) separate verification flows:

  1. There's a flow that roughly resembles "file verification," which is typically done with hashedrekord entries + "bare" signatures over a file input. In this flow the lack of domain separation between a file's contents and its digest is non-ideal but also not particularly dangerous, since the result is a boolean that says whether or not the signature is correct for the input.
  2. There's an "attestation" flow, which is typically done with dsse/intoto entries and a signature over some encoded representation of the input (e.g. an in-toto statement). In this flow the lack of domain separation is more concerning, since the result is a policy document that needs to be subsequently evaluated for correctness/consistency/validity. In particular, because in-toto statements encode their subject, passing a digest means that the verifier can't check that the subject matches.

I think the divergence between these flows is difficult to explain to users, and has made it hard to create maximally-misuse-resistant client APIs -- sigstore-python keeps the two flows separate at the API level, but conflates them slightly (incorrectly, IMO!) at the sigstore verify CLI level.

TL;DR: I think it's a good idea to put this behind some kind of insecure/"I know what I'm doing" option/flag, but I also think we need to revisit the concept of "verification" more fundamentally 🙂 -- right now we really have two (or more) flows with subtly different outputs/semantics that aren't well-communicated.

@kommendorkapten
Copy link
Member Author

Thank for a good reply, I will only give a short reply now as its late, but I couldn't resist :)

In sigstore-go, we do allow for the "attestation flow" to actually be verified against an artifact: https://github.com/sigstore/sigstore-go/blob/378b249542ef54cb12720de86189af3ebeee67a7/pkg/verify/signature.go#L68

So my primary concern is around mapping the digest to the intended artifact. And offering a policy model where the artifact can be directly provided to the verifier minimizes the risk of a confusion. But in-general I agree, there is a disconnect from the bundle to the artifact, which is very much related to the CVE I linked in the first post.

@haydentherapper
Copy link
Collaborator

Speaking to the impact on verification for signing algorithms that require the pre-image: That's a fair point, and I agree that a comment would be valuable.

The Sigstore client specification is underspecified when it comes to DSSE verification and we should flesh this out, what actually is required to be verified vs what's the responsibility of a verifier for a given predicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants