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

Improper verification of number of DSSE signatures #225

Closed
kommendorkapten opened this issue Jul 10, 2024 · 8 comments · Fixed by #226
Closed

Improper verification of number of DSSE signatures #225

kommendorkapten opened this issue Jul 10, 2024 · 8 comments · Fixed by #226
Assignees
Labels
bug Something isn't working

Comments

@kommendorkapten
Copy link
Member

Description

During verification where a DSSE envelope is embedded in the bundle, sigstore-go accepts envelopes a signature count other than 1, which is in violation with the protobuf-bundle spec.

This does not allow for any integrity or authenticity threats, as counter signatures are required, and the verifier must provide a list of accepted verification materials. But this can cause DoS style attacks where a bundle is modified to contain an invalid signature first, as the first signature is returned.

Version

All versions.

@kommendorkapten kommendorkapten added the bug Something isn't working label Jul 10, 2024
@kommendorkapten kommendorkapten self-assigned this Jul 10, 2024
@kommendorkapten
Copy link
Member Author

cc: @bdehamer @woodruffw @loosebazooka

This change (require exact one signature over the envelope) is a recent clarification in protobuf-specs, how are other clients dealing with this? I would like to also make this a conformance test (test with zero and two signatures).

@woodruffw
Copy link
Member

But this can cause DoS style attacks where a bundle is modified to contain an invalid signature first, as the first signature is returned.

FWIW I think the unique risk of this is marginal, since an attacker who can control the DSSE signature set could also make it (or any other part of the surrounding bundle) invalid in any other number of ways 🙂

We need to make a similar change in sigstore-python, since we currently check all signatures (against a single key) and fail if any of them don't match. We'll probably deal with it by rejecting any DSSE envelope with more than one signature outright.

(Ref: https://github.com/sigstore/sigstore-python/blob/4511ddb2143bfbf29d349192c96f90aa4ea4b0de/sigstore/dsse.py#L259-L281)

+1 to adding this to the conformance suite as well!

@bdehamer
Copy link
Collaborator

In sigstore-js, the DSSE envelope signature count is checked as part of the initial bundle validation -- if there is more than one signature present, the bundle will be rejected immediately.

(ref: https://github.com/sigstore/sigstore-js/blob/c1d15d1c4f10e4a6f9f02224819d5d353bf9370a/packages/bundle/src/validate.ts#L116-L117)

@haydentherapper
Copy link
Contributor

haydentherapper commented Jul 10, 2024

Agreed with @woodruffw that I don't think the DoS vector is any different than with a single signature, as an attacker can always invalid the single signature.

I don't assume this change will affect any users, but it might be worth reiterating publicly that DSSE envelopes should contain only one signature (maybe a blog post about DSSE usage within Sigstore?)

@kommendorkapten
Copy link
Member Author

Yes, it's correct that the entire bundle is unauthenticated so an adversary can easily invalidate it. What I referred to is that by adding a single valid signature, a bundle may be valid according to some implementations, but not by others. But I also agree that this is an extremely low risk action.

@woodruffw
Copy link
Member

What I referred to is that by adding a single valid signature, a bundle may be valid according to some implementations, but not by others. But I also agree that this is an extremely low risk action.

Ah, that makes sense! Yeah, I completely agree that there's a distinct (but minor) risk in this kind of differential behavior between implementations 🙂

@woodruffw
Copy link
Member

I've filed sigstore/sigstore-conformance#149 on the conformance suite to track this as well 🙂

@kommendorkapten
Copy link
Member Author

Thanks @woodruffw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants