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

crypto: no streaming interface for ed25519 signature verification #31727

Closed
zx2c4 opened this issue Apr 28, 2019 · 4 comments
Closed

crypto: no streaming interface for ed25519 signature verification #31727

zx2c4 opened this issue Apr 28, 2019 · 4 comments

Comments

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Apr 28, 2019

I've got some code for verifying signify signatures I'm working on:

const releasePublicKeyBase64 = "RWQGxwD+15iPpnPCEijYJ3CWYFgojWwBJZNg0OnJfICVu/CfyKeQ0vIA"

func verifyFile(signatureInput []byte, fileInput io.Reader) bool {
	publicKeyBytes, err := base64.StdEncoding.DecodeString(releasePublicKeyBase64)
	if err != nil || len(publicKeyBytes) != ed25519.PublicKeySize+10 || publicKeyBytes[0] != 'E' || publicKeyBytes[1] != 'd' {
		return false
	}
	publicKeyBytes = publicKeyBytes[10:]

	lines := bytes.Split(signatureInput, []byte{'\n'})
	if len(lines) < 2 {
		return false
	}
	if !bytes.HasPrefix(lines[0], []byte("untrusted comment: ")) {
		return false
	}
	signatureBytes, err := base64.StdEncoding.DecodeString(string(lines[1]))
	if err != nil {
		return false
	}
	if len(signatureBytes) != ed25519.SignatureSize+10 || signatureBytes[0] != 'E' || signatureBytes[1] != 'd' {
		return false
	}
	signatureBytes = signatureBytes[10:]

	somewhatLargeFileToBufferYuck, err := ioutil.ReadAll(fileInput)
	if err != nil {
		return false
	}

	return ed25519.Verify(publicKeyBytes, somewhatLargeFileToBufferYuck, signatureBytes)
}

Since ed25519.Verify takes only a single buffer, that means I have to potentially buffer the entire file.

@FiloSottile Would you be opposed to adding a streaming interface? Or, better, a function that takes an io.Reader and does this automatically with that?

Or are you entirely opposed to this and think that only small data (like text files containing hash lists or something) should be signed, rather than whole files (even though ed25519 hashes its input).

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Apr 28, 2019

This is intentional. A streaming interface is useful when the message does not fit in memory, but that means that some of the unverified message needs to be flushed somewhere. While possible to do securely, it's a proven footgun.

I would much rather provide a chaining scheme, if a standard one exists (like CHAIN and STREAM for AEADs). At least that is only at risk of truncation, since each chunk is verified before being released.

@zx2c4
Copy link
Contributor Author

@zx2c4 zx2c4 commented Apr 28, 2019

Rather than a scary incremental init/update/final interface, what about something like...

The current function is:

func Verify(publicKey PublicKey, message, sig []byte) bool

Proposal: add something like:

func VerifyFrom(publicKey PublicKey, message *io.Reader, sig []byte) bool

Seems much harder to screw that up, and allows very obvious ways of, say, opening a file and verifying it.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Apr 28, 2019

It would be a pretty idiomatic API, but the core issue is still that you are probably using this API in the first place because the message doesn't fit in memory, so the Reader will probably be chained to some file storage or similar, leading to unverified bytes getting flushed somewhere.

A better approach would be chunking, where each chunk is verified before being flushed, but that requires an explicit API, and ideally a standard. (I would be happy to discuss that in an issue, though.)

@ItalyPaleAle
Copy link

@ItalyPaleAle ItalyPaleAle commented Oct 13, 2019

@FiloSottile could you please explain what you mean with this?

A streaming interface is useful when the message does not fit in memory, but that means that some of the unverified message needs to be flushed somewhere.

Especially the part where you say "the unverified message needs to be flushed somewhere". How would that be a security risk?

@golang golang locked and limited conversation to collaborators Oct 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.