Issue 9438043: code review 9438043: rsa: Implementation of RSASSA-PSS signature algorithm a...
|
Created:
11 years, 10 months ago by Nan Deng Modified:
11 years, 10 months ago Reviewers:
CC:
agl, agl1, gobot, golang-dev, r Visibility:
Public. |
Descriptionrsa: Implementation of RSASSA-PSS signature algorithm and its corresponding test cases
Implementation of RSASSA-PSS signature algorithm described in RFC 3447 and its corresponding test cases.
There are two test files:
- emsa_test.go: Test on EMSA-PSS encoding/decoding scheme
- pssvect_test.go: Test on the signature algorithm itself. All cases are generated from the test vectors provided by RSA Lab, which can be downloaded from ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1/pkcs-1v2-1-vec.zip
Patch Set 1 #Patch Set 2 : diff -r 1abed5873071 https://code.google.com/p/go #Patch Set 3 : diff -r 1abed5873071 https://code.google.com/p/go #
Total comments: 43
Patch Set 4 : diff -r 1abed5873071 https://code.google.com/p/go #Patch Set 5 : diff -r 1abed5873071 https://code.google.com/p/go #Patch Set 6 : diff -r 1abed5873071 https://code.google.com/p/go #Patch Set 7 : diff -r 1abed5873071 https://code.google.com/p/go #
Total comments: 2
Patch Set 8 : diff -r 1abed5873071 https://code.google.com/p/go #Patch Set 9 : diff -r 1abed5873071 https://code.google.com/p/go #Patch Set 10 : diff -r 1abed5873071 https://code.google.com/p/go #Patch Set 11 : diff -r 1abed5873071 https://code.google.com/p/go #
MessagesTotal messages: 18
Hello golang-dev@googlegroups.com (cc: agl@golang.org), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
Does this need to be in the main repo? The go.crypt repo seems fine to ignorant me.
Sign in to reply to this message.
In rsa.go, line 8, there is a TODO says " TODO(agl): Add support for PSS padding." So I submit this CL to the main repo. To me, putting these functions to anywhere would be OK. Do I need to submit another CL to go.crypto repo? On 2013/05/15 22:04:02, r wrote: > Does this need to be in the main repo? The go.crypt repo seems fine to ignorant > me.
Sign in to reply to this message.
R=agl (assigned by minux)
Sign in to reply to this message.
Haven't looked at the tests yet. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go File src/pkg/crypto/rsa/pss.go (right): https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:2: Should have a file-level comment describing what the functions in this file do and referencing http://www.rsa.com/rsalabs/pkcs/files/h11300-wp-pkcs-1v2-2-rsa-cryptography-s... https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:11: func emsaPSSEncode(mHash []byte, emBits int, salt []byte, hash hash.Hash) ([]byte, error) { // See [1], section 9.1.1. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:31: if emLen < hLen+sLen+2 { since this check is after all the slicing, above, I suspect that the code will crash first. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:46: prefix := []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00} var prefix [8]byte https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:48: hash.Write(prefix[0:8]) prefix[:] https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:70: // 11. Set the leftmost 8emLen - emBits bits of the leftmost octet in s/8emLen/8*emLen/ https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:82: func emsaPSSVerify(mHash []byte, em []byte, emBits, sLen int, hash hash.Hash) error { drop the first []byte https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:83: // 1. If the length of M is greater than the input limitation for the // See [1], section 9.1.2. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:110: // 6. If the leftmost 8emLen - emBits bits of the leftmost octet in s/8emLen/8*emLen/ https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:113: if em[0]&(0xFF<<uint(8-(8*emLen-emBits))) != 0 { s/8emLen/8*emLen/ https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:122: // 9. Set the leftmost 8emLen - emBits bits of the leftmost octet in DB s/8emLen/8*emLen/ https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:148: prefix := []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00} var prefix [8]byte https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:149: hash.Write(prefix) prefix[:] https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:154: h0 = hash.Sum(h0[:0]) delete the previous line and do h0 := hash.Sum(nil) https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:157: for i, e := range h0 { use bytes.Equal https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:168: func SignPSS(rand io.Reader, priv *PrivateKey, hash crypto.Hash, hashed []byte, salt []byte) (s []byte, err error) { First line in the function should reference [1], section 8.1.1. (Where [1] will be defined in the file level comment and point to the PKCS#1 spec PDF.) https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:168: func SignPSS(rand io.Reader, priv *PrivateKey, hash crypto.Hash, hashed []byte, salt []byte) (s []byte, err error) { Why is the salt passed in rather than generated internally? How are users expected to pick a good value for the salt length? It might be best to fix it in this package. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:169: em, err := emsaPSSEncode(hashed, priv.N.BitLen()-1, salt, hash.New()) BitLen is a somewhat expensive operation. Worth doing nBits := priv.N.BitLen() at the top of this function. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:186: // sLen is number of bytes of the salt used to sign the message. s/sLen/saltLen/ https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:187: func VerifyPSS(pub *PublicKey, hash crypto.Hash, hashed []byte, sig []byte, sLen int) error { // See [1], section 8.1.2. nBits := pub.N.BitLen() if len(sig) != (nBits+7)/8 { return ErrVerification } https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:197: err := emsaPSSVerify(hashed, em, pub.N.BitLen()-1, sLen, hash.New()) merge with next line.
Sign in to reply to this message.
Hello agl@chromium.org, agl@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com, r@golang.org), Please take another look.
Sign in to reply to this message.
Changed the pss.go according to agl's advise. Removed most test cases from pssvect_test.go. Only two of them are reserved and added comments to them correspondingly.
Sign in to reply to this message.
Hello agl@chromium.org, agl@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com, r@golang.org), Please take another look.
Sign in to reply to this message.
Fixed all parts according to agl's advise. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go File src/pkg/crypto/rsa/pss.go (right): https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:2: On 2013/05/17 15:18:55, agl1 wrote: > Should have a file-level comment describing what the functions in this file do > and referencing > http://www.rsa.com/rsalabs/pkcs/files/h11300-wp-pkcs-1v2-2-rsa-cryptography-s... Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:11: func emsaPSSEncode(mHash []byte, emBits int, salt []byte, hash hash.Hash) ([]byte, error) { On 2013/05/17 15:18:55, agl1 wrote: > // See [1], section 9.1.1. Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:31: if emLen < hLen+sLen+2 { On 2013/05/17 15:18:55, agl1 wrote: > since this check is after all the slicing, above, I suspect that the code will > crash first. Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:46: prefix := []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00} On 2013/05/17 15:18:55, agl1 wrote: > var prefix [8]byte Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:48: hash.Write(prefix[0:8]) On 2013/05/17 15:18:55, agl1 wrote: > prefix[:] Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:70: // 11. Set the leftmost 8emLen - emBits bits of the leftmost octet in On 2013/05/17 15:18:55, agl1 wrote: > s/8emLen/8*emLen/ Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:82: func emsaPSSVerify(mHash []byte, em []byte, emBits, sLen int, hash hash.Hash) error { On 2013/05/17 15:18:55, agl1 wrote: > drop the first []byte Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:83: // 1. If the length of M is greater than the input limitation for the On 2013/05/17 15:18:55, agl1 wrote: > // See [1], section 9.1.2. Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:110: // 6. If the leftmost 8emLen - emBits bits of the leftmost octet in On 2013/05/17 15:18:55, agl1 wrote: > s/8emLen/8*emLen/ Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:113: if em[0]&(0xFF<<uint(8-(8*emLen-emBits))) != 0 { On 2013/05/17 15:18:55, agl1 wrote: > s/8emLen/8*emLen/ Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:122: // 9. Set the leftmost 8emLen - emBits bits of the leftmost octet in DB On 2013/05/17 15:18:55, agl1 wrote: > s/8emLen/8*emLen/ Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:148: prefix := []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00} On 2013/05/17 15:18:55, agl1 wrote: > var prefix [8]byte Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:149: hash.Write(prefix) On 2013/05/17 15:18:55, agl1 wrote: > prefix[:] Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:154: h0 = hash.Sum(h0[:0]) On 2013/05/17 15:18:55, agl1 wrote: > delete the previous line and do > > h0 := hash.Sum(nil) Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:157: for i, e := range h0 { On 2013/05/17 15:18:55, agl1 wrote: > use bytes.Equal Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:168: func SignPSS(rand io.Reader, priv *PrivateKey, hash crypto.Hash, hashed []byte, salt []byte) (s []byte, err error) { Since the test vectors provided by RSA contains the salt, I put the salt here. I think it would be better to it an internal function, say signPSSWithSalt() and define SignPSS to call signPSSWithSalt(). In test cases, we test signPSSWithSalt() so that we can keep using test cases from RSA. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:168: func SignPSS(rand io.Reader, priv *PrivateKey, hash crypto.Hash, hashed []byte, salt []byte) (s []byte, err error) { On 2013/05/17 15:18:55, agl1 wrote: > Why is the salt passed in rather than generated internally? How are users > expected to pick a good value for the salt length? It might be best to fix it in > this package. Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:168: func SignPSS(rand io.Reader, priv *PrivateKey, hash crypto.Hash, hashed []byte, salt []byte) (s []byte, err error) { On 2013/05/17 15:18:55, agl1 wrote: > First line in the function should reference [1], section 8.1.1. (Where [1] will > be defined in the file level comment and point to the PKCS#1 spec PDF.) Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:169: em, err := emsaPSSEncode(hashed, priv.N.BitLen()-1, salt, hash.New()) On 2013/05/17 15:18:55, agl1 wrote: > BitLen is a somewhat expensive operation. Worth doing > > nBits := priv.N.BitLen() > > at the top of this function. Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:186: // sLen is number of bytes of the salt used to sign the message. On 2013/05/17 15:18:55, agl1 wrote: > s/sLen/saltLen/ Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:187: func VerifyPSS(pub *PublicKey, hash crypto.Hash, hashed []byte, sig []byte, sLen int) error { On 2013/05/17 15:18:55, agl1 wrote: > // See [1], section 8.1.2. > nBits := pub.N.BitLen() > if len(sig) != (nBits+7)/8 { > return ErrVerification > } Done. https://codereview.appspot.com/9438043/diff/5001/src/pkg/crypto/rsa/pss.go#ne... src/pkg/crypto/rsa/pss.go:197: err := emsaPSSVerify(hashed, em, pub.N.BitLen()-1, sLen, hash.New()) On 2013/05/17 15:18:55, agl1 wrote: > merge with next line. Done.
Sign in to reply to this message.
https://codereview.appspot.com/9438043/diff/20001/src/pkg/crypto/rsa/pss.go File src/pkg/crypto/rsa/pss.go (right): https://codereview.appspot.com/9438043/diff/20001/src/pkg/crypto/rsa/pss.go#n... src/pkg/crypto/rsa/pss.go:190: func SignPSS(rand io.Reader, priv *PrivateKey, hash crypto.Hash, hashed []byte, saltLen int) (s []byte, err error) { Some reorganisation is probably needed before landing, but I can do that before submitting. The only important point is the interface: I don't think the saltLen parameters should be there. OpenSSL allows one to specify an optional structure that can set the salt len but the defaults are to set the salt to be as long as possible when encoding and decoding. I think we should mirror that or, if you feel strongly that the salt length needs to be a parameter, then we could have a optional options struct. But OpenSSL's version includes some pretty obscure options that I'm not sure that we would want to support: typedef struct rsa_pss_params_st »·······{ »·······X509_ALGOR *hashAlgorithm; »·······X509_ALGOR *maskGenAlgorithm; »·······ASN1_INTEGER *saltLength; »·······ASN1_INTEGER *trailerField; »·······} RSA_PSS_PARAMS;
Sign in to reply to this message.
https://codereview.appspot.com/9438043/diff/20001/src/pkg/crypto/rsa/pss.go File src/pkg/crypto/rsa/pss.go (right): https://codereview.appspot.com/9438043/diff/20001/src/pkg/crypto/rsa/pss.go#n... src/pkg/crypto/rsa/pss.go:190: func SignPSS(rand io.Reader, priv *PrivateKey, hash crypto.Hash, hashed []byte, saltLen int) (s []byte, err error) { On 2013/05/20 19:56:23, agl1 wrote: > Some reorganisation is probably needed before landing, but I can do that before > submitting. > > The only important point is the interface: I don't think the saltLen parameters > should be there. I agree. After a reading of more details, I would rather set the saltLen as len(hashed) (or hash.Size()) > > OpenSSL allows one to specify an optional structure that can set the salt len > but the defaults are to set the salt to be as long as possible when encoding and > decoding. I think we should mirror that or, if you feel strongly that the salt > length needs to be a parameter, then we could have a optional options struct. > But OpenSSL's version includes some pretty obscure options that I'm not sure > that we would want to support: > > typedef struct rsa_pss_params_st > »·······{ > »·······X509_ALGOR *hashAlgorithm; > »·······X509_ALGOR *maskGenAlgorithm; > »·······ASN1_INTEGER *saltLength; > »·······ASN1_INTEGER *trailerField; > »·······} RSA_PSS_PARAMS; To me, supporting such many of options may confuse the user which may in turn lead to some bad decisions. I agree that it would be better to provide a less-flexible version of the interface. I will make the change and upload a new version soon.
Sign in to reply to this message.
Hello agl@chromium.org, agl@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com, r@golang.org), Please take another look.
Sign in to reply to this message.
Hello agl@chromium.org, agl@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com, r@golang.org), Please take another look.
Sign in to reply to this message.
Have you signed the CLA? http://golang.org/doc/contribute.html#copyright I altered the code to be compatible with OpenSSL's PSS implementation. This required adding a PSSOptions struct argument to VerifyPSS and SignPSS. Please see http://play.golang.org/p/pxYrCq1VBj. I also altered the tests to check all the values of NIST's test vectors (from a testdata file) and to check OpenSSL compat. Please see http://play.golang.org/p/qEYe_ieyd-
Sign in to reply to this message.
On 2013/05/22 20:21:02, agl1 wrote: > Have you signed the CLA? http://golang.org/doc/contribute.html#copyright > Just signed it. It said that it will be processed shortly. > I altered the code to be compatible with OpenSSL's PSS implementation. This > required adding a PSSOptions struct argument to VerifyPSS and SignPSS. Please > see http://play.golang.org/p/pxYrCq1VBj. > > I also altered the tests to check all the values of NIST's test vectors (from a > testdata file) and to check OpenSSL compat. Please see > http://play.golang.org/p/qEYe_ieyd- Sure. Do I need to copy those code and upload another patch set to this CL?
Sign in to reply to this message.
On Wed, May 22, 2013 at 4:52 PM, <monnand@gmail.com> wrote: > Just signed it. It said that it will be processed shortly. Thanks. I'll do the A+C update for you. > Sure. Do I need to copy those code and upload another patch set to this > CL? Nope, if you're happy with it then you don't need to update the CL. I'll land from my local copy. Cheers AGL
Sign in to reply to this message.
On 2013/05/22 20:56:39, agl wrote: > On Wed, May 22, 2013 at 4:52 PM, <mailto:monnand@gmail.com> wrote: > > Just signed it. It said that it will be processed shortly. > > Thanks. I'll do the A+C update for you. > Thank you! > > Sure. Do I need to copy those code and upload another patch set to this > > CL? > > Nope, if you're happy with it then you don't need to update the CL. > I'll land from my local copy. > Sure. I'm happy with it. It looks better. > > Cheers > > AGL
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=0fb55e40bd0c *** crypto/rsa: implement PSS signatures. This change contains an implementation of the RSASSA-PSS signature algorithm described in RFC 3447. R=agl, agl CC=gobot, golang-dev, r https://codereview.appspot.com/9438043 Committer: Adam Langley <agl@golang.org>
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
