[go] crypto/rsa: fixes the salt length calculation when PSSSaltLengthAuto option is set in rsa sign request.
Himanshu Kishna Srivastava (Gerrit)
Himanshu Kishna Srivastava has uploaded this change for review.
crypto/rsa: fixes the salt length calculation when PSSSaltLengthAuto option is set in rsa sign request.
The existing implementation wrongly calculates the salt length when PSSSaltLengthAuto is set.
The maximum salt length should equals modulus_key_size/8 - hash_length - 2.
e.g. with a 4096 bit modulus key and SHA-1 hash, the maximum salt length becomes (4096/8) - 20 - 2 = 490.
Fixes #42741
Change-Id: I18bb82c41c511d564b3f4c443f4b3a38ab010ac5
---
M src/crypto/rsa/pss.go
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/crypto/rsa/pss.go b/src/crypto/rsa/pss.go
index b2adbed..bd984bd 100644
--- a/src/crypto/rsa/pss.go
+++ b/src/crypto/rsa/pss.go
@@ -269,7 +269,7 @@
saltLength := opts.saltLength()
switch saltLength {
case PSSSaltLengthAuto:
- saltLength = priv.Size() - 2 - hash.Size()
+ saltLength = (priv.N.BitLen() / 8) - 2 - hash.Size()
case PSSSaltLengthEqualsHash:
saltLength = hash.Size()
}
To view, visit change 302230. To unsubscribe, or for help writing mail filters, visit settings.
Emmanuel Odeke (Gerrit)
Attention is currently required from: Himanshu Kishna Srivastava, Filippo Valsorda.
Patch set 1:Run-TryBot +1Trust +1
3 comments:
Commit Message:
Patch Set #1, Line 7:
crypto/rsa: fixes the salt length calculation when PSSSaltLengthAuto option is set in rsa sign request.crypto/rsa: fix salt length calculation with PSSSaltLengthAuto
The existing implementation wrongly calculates the salt length when PSSSaltLengthAuto is set.
The maximum salt length should equals modulus_key_size/8 - hash_length - 2.
e.g. with a 4096 bit modulus key and SHA-1 hash, the maximum salt length becomes (4096/8) - 20 - 2 = 490.
When PSSSaltLength is set, the maximum salt length must equal:
modulus_key_size/8 - hash_length - 2
and for example, with a 4096 bit modulus key, and a SHA-1 hash,
it should be:
4096/8 - 20 - 2 = 490Previously we'd encounter this error:
crypto/rsa: key size too small for PSS signature
Patchset:
Thank you for this change Himanshu, and congratulations on your first change as a Go contributor.
Delighted to have you, and thank you for the sharp eyes!Could you please also add a test. Let's adapt Filippo's suggestion to make this test that can add at the bottom of crypto/rsa/pss_test.go
// Ensure that we don't encounter an error when
// signing with PSS signatures.
// See issue https://golang.org/org/issues/42741.
func TestSignWithPSSSaltLengthAuto(t *testing.T) {
key, err := GenerateKey(rand.Reader, 2049)
if err != nil {
t.Fatal(err)
}
digest := sha256.Sum256([]byte("message"))
signature, err := key.Sign(rand.Reader, digest[:], &PSSOptions{
SaltLength: PSSSaltLengthAuto,
Hash: crypto.SHA256,
})
if err != nil {
t.Fatal(err)
}if len(signature) == 0 {
t.Fatal("empty signature returned")
}
}
To view, visit change 302230. To unsubscribe, or for help writing mail filters, visit settings.
Himanshu Kishna Srivastava (Gerrit)
Attention is currently required from: Himanshu Kishna Srivastava, Filippo Valsorda.
Himanshu Kishna Srivastava uploaded patch set #2 to this change.
crypto/rsa: fix salt length calculation with PSSSaltLengthAuto
When PSSSaltLength is set, the maximum salt length must equal:
modulus_key_size/8 - hash_length - 2
and for example, with a 4096 bit modulus key, and a SHA-1 hash,
it should be:
4096/8 - 20 - 2 = 490
Previously we'd encounter this error:
crypto/rsa: key size too small for PSS signature
Fixes #42741
Change-Id: I18bb82c41c511d564b3f4c443f4b3a38ab010ac5
---
M src/crypto/rsa/pss.go
M src/crypto/rsa/pss_test.go
2 files changed, 20 insertions(+), 2 deletions(-)
To view, visit change 302230. To unsubscribe, or for help writing mail filters, visit settings.
Himanshu Kishna Srivastava (Gerrit)
Attention is currently required from: Emmanuel Odeke, Filippo Valsorda.
3 comments:
Commit Message:
Patch Set #1, Line 7:
crypto/rsa: fixes the salt length calculation when PSSSaltLengthAuto option is set in rsa sign request.
crypto/rsa: fix salt length calculation with PSSSaltLengthAuto
Done
The existing implementation wrongly calculates the salt length when PSSSaltLengthAuto is set.
The maximum salt length should equals modulus_key_size/8 - hash_length - 2.
e.g. with a 4096 bit modulus key and SHA-1 hash, the maximum salt length becomes (4096/8) - 20 - 2 = 490.
When PSSSaltLength is set, the maximum salt length must equal: […]
Done
Patchset:
I have also added the unit test case as per suggestion.
Please review.
To view, visit change 302230. To unsubscribe, or for help writing mail filters, visit settings.
Emmanuel Odeke (Gerrit)
Attention is currently required from: Katie Hockman, Roland Shoemaker, Himanshu Kishna Srivastava, Filippo Valsorda.
Patch set 2:Run-TryBot +1Code-Review +1Trust +1
1 comment:
Patchset:
Thanks!
To view, visit change 302230. To unsubscribe, or for help writing mail filters, visit settings.
Emmanuel Odeke (Gerrit)
Attention is currently required from: Katie Hockman, Roland Shoemaker, Himanshu Kishna Srivastava, Filippo Valsorda.
Emmanuel Odeke removed Adam Langley from this change.
To view, visit change 302230. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Attention is currently required from: Katie Hockman, Roland Shoemaker, Himanshu Kishna Srivastava.
Patch set 2:Code-Review +2
2 comments:
File src/crypto/rsa/pss.go:
Patch Set #2, Line 272:
saltLength = (priv.N.BitLen() / 8) - 2 - hash.Size()Technically I think this is supposed to be
(priv.N.BitLen()-1+7)/8 - 2 - hash.Size()
File src/crypto/rsa/pss_test.go:
Patch Set #2, Line 237:
key, err := GenerateKey(rand.Reader, 2049)Running GenerateKey at every test run is pretty expensive. Instead, hardcode a 2049 bit key, and add a comment saying it's intentionally 2049 bits as a test for Issue 42741. Thank you!
To view, visit change 302230. To unsubscribe, or for help writing mail filters, visit settings.
Himanshu Kishna Srivastava (Gerrit)
Attention is currently required from: Katie Hockman, Roland Shoemaker, Himanshu Kishna Srivastava.
Himanshu Kishna Srivastava uploaded patch set #3 to this change.
crypto/rsa: fix salt length calculation with PSSSaltLengthAuto
When PSSSaltLength is set, the maximum salt length must equal:
(modulus_key_size - 1 + 7)/8 - hash_length - 2
and for example, with a 4096 bit modulus key, and a SHA-1 hash,
it should be:
(4096 -1 + 7)/8 - 20 - 2 = 490
Previously we'd encounter this error:
crypto/rsa: key size too small for PSS signature
Fixes #42741
Change-Id: I18bb82c41c511d564b3f4c443f4b3a38ab010ac5
---
M src/crypto/rsa/pss.go
M src/crypto/rsa/pss_test.go
2 files changed, 20 insertions(+), 2 deletions(-)
To view, visit change 302230. To unsubscribe, or for help writing mail filters, visit settings.
Himanshu Kishna Srivastava (Gerrit)
Attention is currently required from: Katie Hockman, Roland Shoemaker, Filippo Valsorda.
3 comments:
Patchset:
I have made the changes as per your review comment in file pss.go.
But for pss_test.go I have used a key of length 513.
The issue is also reproducible with 513 length also. I am avoiding hard code rsa key to make the code simple and clean.I also find that rsa keygenerate is used instead of hardcoding of rsa keys.
Also, I feel it saves the break failure with GenerateKey in future.Please share your opinion on the same.
File src/crypto/rsa/pss.go:
Patch Set #2, Line 272:
saltLength = (priv.N.BitLen() / 8) - 2 - hash.Size()
Technically I think this is supposed to be […]
Agreed. This is the better fix. Thanks.
I have done the changes.
Please review.
File src/crypto/rsa/pss_test.go:
Patch Set #2, Line 237:
key, err := GenerateKey(rand.Reader, 2049)
Running GenerateKey at every test run is pretty expensive. […]
This issue is also reproducible when the size of the key is 513 etc.
Will it be fine if I use key length of 513 here as this will be very less expensive
task in comparison to generate key of length 2049.I am avoiding hard-coding of key to make code simple and avoid any functionality break with GenerateKey in future.
To view, visit change 302230. To unsubscribe, or for help writing mail filters, visit settings.
Emmanuel Odeke (Gerrit)
Attention is currently required from: Katie Hockman, Roland Shoemaker, Filippo Valsorda.
Patch set 3:Run-TryBot +1Code-Review +2Trust +1
1 comment:
Patchset:
LGTM, thank you Himanshu. Please take another look Filippo.
RELNOTE=yes
To view, visit change 302230. To unsubscribe, or for help writing mail filters, visit settings.
Himanshu Kishna Srivastava (Gerrit)
Attention is currently required from: Katie Hockman, Roland Shoemaker, Filippo Valsorda.
Hello All,
Please review the changes. Please let me know if you have any review comment.
Regards
Himanshu
Himanshu Srivastava
Attention is currently required from: Katie Hockman, Roland Shoemaker, Filippo Valsorda.
TryBots are happy.
Patch set 3:TryBot-Result +1
To view, visit change 302230. To unsubscribe, or for help writing mail filters, visit settings.
Himanshu Srivastava
Himanshu Kishna Srivastava (Gerrit)
Attention is currently required from: Katie Hockman, Roland Shoemaker, Filippo Valsorda.
Hello All,
A gentle reminder. Any review comment.
Regards
Himanshu
Filippo Valsorda (Gerrit)
Filippo Valsorda submitted this change.
crypto/rsa: fix salt length calculation with PSSSaltLengthAuto
When PSSSaltLength is set, the maximum salt length must equal:
(modulus_key_size - 1 + 7)/8 - hash_length - 2
and for example, with a 4096 bit modulus key, and a SHA-1 hash,
it should be:
(4096 -1 + 7)/8 - 20 - 2 = 490
Previously we'd encounter this error:
crypto/rsa: key size too small for PSS signature
Fixes #42741
Change-Id: I18bb82c41c511d564b3f4c443f4b3a38ab010ac5
Reviewed-on: https://go-review.googlesource.com/c/go/+/302230
Reviewed-by: Emmanuel Odeke <emma...@orijtech.com>
Reviewed-by: Filippo Valsorda <fil...@golang.org>
Trust: Emmanuel Odeke <emma...@orijtech.com>
Run-TryBot: Emmanuel Odeke <emma...@orijtech.com>
TryBot-Result: Go Bot <go...@golang.org>
---
M src/crypto/rsa/pss.go
M src/crypto/rsa/pss_test.go
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/src/crypto/rsa/pss.go b/src/crypto/rsa/pss.go
index b2adbed..814522d 100644
--- a/src/crypto/rsa/pss.go
+++ b/src/crypto/rsa/pss.go
@@ -269,7 +269,7 @@
saltLength := opts.saltLength()
switch saltLength {
case PSSSaltLengthAuto:
- saltLength = priv.Size() - 2 - hash.Size()
+ saltLength = (priv.N.BitLen()-1+7)/8 - 2 - hash.Size()
case PSSSaltLengthEqualsHash:
saltLength = hash.Size()
}
diff --git a/src/crypto/rsa/pss_test.go b/src/crypto/rsa/pss_test.go
index dfa8d8b..c3a6d46 100644
--- a/src/crypto/rsa/pss_test.go
+++ b/src/crypto/rsa/pss_test.go
@@ -12,7 +12,7 @@
_ "crypto/md5"
"crypto/rand"
"crypto/sha1"
- _ "crypto/sha256"
+ "crypto/sha256"
"encoding/hex"
"math/big"
"os"
@@ -233,6 +233,24 @@
}
}
+func TestSignWithPSSSaltLengthAuto(t *testing.T) {
+ key, err := GenerateKey(rand.Reader, 513)
+ if err != nil {
+ t.Fatal(err)
+ }
+ digest := sha256.Sum256([]byte("message"))
+ signature, err := key.Sign(rand.Reader, digest[:], &PSSOptions{
+ SaltLength: PSSSaltLengthAuto,
+ Hash: crypto.SHA256,
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(signature) == 0 {
+ t.Fatal("empty signature returned")
+ }
+}
+
func bigFromHex(hex string) *big.Int {
n, ok := new(big.Int).SetString(hex, 16)
if !ok {
To view, visit change 302230. To unsubscribe, or for help writing mail filters, visit settings.