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

sigstore: stream input into signing #329

Merged
merged 6 commits into from
Dec 7, 2022
Merged

sigstore: stream input into signing #329

merged 6 commits into from
Dec 7, 2022

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented Dec 6, 2022

WIP; needs test updates and equivalent changes to verification.

Closes #158.

Signed-off-by: William Woodruff william@trailofbits.com

Closes #158.

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw added component:signing Core signing functionality component:verification Core verification functionality labels Dec 6, 2022
@woodruffw woodruffw self-assigned this Dec 6, 2022
See: python/typing#659

Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw marked this pull request as ready for review December 6, 2022 16:19
Comment on lines +122 to +125
nbytes = io.readinto(view) # type: ignore
while nbytes:
sha256.update(view[:nbytes])
nbytes = io.readinto(view) # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flagging for thoughts here: it's possible that, once we make the signing and verification APIs public, a user could decide to be clever and decide to do something like this:

signer.sign(socket_io, identity_token)

...where socket_io is a file-like handle for a network connection. If the signer doesn't control the server or the network itself is untrustworthy (i.e., the transport itself is insecure), an attacker could truncate the streaming input and produce a digest other than the one expected, which in turn would be signed by the given identity.

This in turn could result in valid digests for malicious messages, e.g. for an input structure that looks something like this:

S: IMPORTANT PIECE OF INFORMATION THAT REQUIRES CONTEXT TO FOLLOW

<- attacker truncates stream here

S: IMPORTANT CONTEXT THAT MUST ALSO BE SIGNED

I think we have a couple of options here:

  1. Define this to be out-of-scope/outside of our threat model: sigstore-python operates primarily on local files (and that's all the CLI supports), where these kinds of truncation attacks are more complicated (and any sufficiently empowered attacker can probably just pass a malicious input in instead).
  2. stat or ftell the source before streaming from it, and fail if the ultimate number of bytes read doesn't match the result. This will probably prevent usage with raw sockets entirely, since sockets don't have fixed input sizes. It would however allow usage with a sufficiently smart file-like object, like an HTTP client that provides the size via the HTTP headers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @znewman01, since you may have already run into this on cosign (or it may become a similar design consideration in the near future!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bit outside our threat model, but if 2) is relatively easy, we should do that just to be safe. I have doubts on how well this will work cross-platform, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agreed. I'll leave it as-is for now, with a justifying comment.

di
di previously approved these changes Dec 7, 2022
Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. An example in the docs would be nice.

@woodruffw
Copy link
Member Author

An example in the docs would be nice.

What kind of example are you looking for? This doesn't change the CLI at all, only how we plumb its arguments into the API, so I might be missing something 🙂

Signed-off-by: William Woodruff <william@trailofbits.com>
@di
Copy link
Member

di commented Dec 7, 2022

An example in the docs would be nice.

Sorry, I meant: some documentation of our discussion, which you did. Thanks!

@woodruffw woodruffw merged commit bc7d6a4 into main Dec 7, 2022
@woodruffw woodruffw deleted the ww/sign-streaming branch December 7, 2022 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:signing Core signing functionality component:verification Core verification functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid reading input files to memory
2 participants