Skip to content

x/crypto/argon2: fix panic when calling IDKey or Key with keyLen == 0#95

Open
psampaz wants to merge 2 commits into
golang:masterfrom
psampaz:fix_issue_33583
Open

x/crypto/argon2: fix panic when calling IDKey or Key with keyLen == 0#95
psampaz wants to merge 2 commits into
golang:masterfrom
psampaz:fix_issue_33583

Conversation

@psampaz

@psampaz psampaz commented Aug 11, 2019

Copy link
Copy Markdown

Calling IDKey or Key functions using 0 as KeyLen panics. This change fixes this error by returning an empty byte slice.

Fixes golang/go#33583

@gopherbot

Copy link
Copy Markdown
Contributor

This PR (HEAD: 7e2d075) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/189878 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

Comment thread argon2/argon2.go Outdated
func deriveKey(mode int, password, salt, secret, data []byte, time, memory uint32, threads uint8, keyLen uint32) []byte {
if keyLen == 0 {
return []byte{}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think it's slightly more technically correct to put this after the time/threads checks directly below

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It seems you are right. Done

@gopherbot

Copy link
Copy Markdown
Contributor

This PR (HEAD: 205e825) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/189878 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot

Copy link
Copy Markdown
Contributor

This PR (HEAD: 096cba9) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/189878 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot

Copy link
Copy Markdown
Contributor

Message from Zach Jones:

Patch Set 4:

(2 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/189878.
After addressing review feedback, remember to publish your drafts!

@psampaz psampaz changed the title x/crypto/argon2: fix panic when keyLen == 0 x/crypto/argon2: fix panic when calling IDKey or Key with keyLen == 0 Sep 26, 2019
@gopherbot

Copy link
Copy Markdown
Contributor

Message from Akhil Indurti:

Patch Set 8:

Relaying my comments from the issue here:

Doesn't argon2 expect a minimum keyLen of 4 as per https://tools.ietf.org/html/draft-irtf-cfrg-argon2-03#section-3.1?

In that case, how about intentionally panicking on a keyLen < 4? Something like,

if keyLen < 4 {
    panic("argon2: tag length is too small")
}

The panic is currently happening because blake2b expects a hash size of at least 1, so maybe we should error out instead.


Please don’t reply on this GitHub thread. Visit golang.org/cl/189878.
After addressing review feedback, remember to publish your drafts!

@gopherbot

Copy link
Copy Markdown
Contributor

Message from Zach Jones:

Patch Set 4:

(2 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/189878.
After addressing review feedback, remember to publish your drafts!

@gopherbot

Copy link
Copy Markdown
Contributor

Message from Akhil Indurti:

Patch Set 8:

Relaying my comments from the issue here:

Doesn't argon2 expect a minimum keyLen of 4 as per https://tools.ietf.org/html/draft-irtf-cfrg-argon2-03#section-3.1?

In that case, how about intentionally panicking on a keyLen < 4? Something like,

if keyLen < 4 {
    panic("argon2: tag length is too small")
}

The panic is currently happening because blake2b expects a hash size of at least 1, so maybe we should error out instead.


Please don’t reply on this GitHub thread. Visit golang.org/cl/189878.
After addressing review feedback, remember to publish your drafts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

x/crypto/argon2: panic if keyLen == 0

4 participants