Bug 1215295 - Support RSA-PSS for digital signing

Bug 1215295 - Support RSA-PSS for digital signing

[ekr-rietveld, 2015-11-06 21:31:59]

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
File external_tests/pk11_gtest/manifest.mn (left):

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/manifest.mn:9: # These sources have access to libssl
internals
Rietveld seems to be a bit confused and think that this is a change to the
ssl_gtest file. Any idea what's up there?

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
File external_tests/pk11_gtest/pk11_gtest.cc (right):

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_gtest.cc:19: g_working_dir_path = workdir;
The working directory isn't needed.

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_gtest.cc:28:
NSS_NoDB_Init(g_working_dir_path.c_str());
You can pass NULL here.

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
File external_tests/pk11_gtest/pk11_rsapss_unittest.cc (right):

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_rsapss_unittest.cc:23: unsigned int sig_len;
These lengths should be size_t

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_rsapss_unittest.cc:144: void Init(TestVector* tv)
{
const

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_rsapss_unittest.cc:157: SECItem params = {
siBuffer, (unsigned char*)&rsaPssParams,
Use C++-style casts in C++ code please.

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_rsapss_unittest.cc:158: sizeof(rsaPssParams) };
Fix indent.

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_rsapss_unittest.cc:180:
SECKEY_DestroySubjectPublicKeyInfo(spki);
Instead, use the types in:
https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/ScopedNSS...

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_rsapss_unittest.cc:229: 
Please include a test demonstrating that bogus signatures fail to verify

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_rsapss_unittest.cc:238: kTestVector1Sig,
sizeof(kTestVector1Sig) };
1. Macros might make this easier.
2. Why bother to make a tv here. Just pass in the arguments directly to Init()

https://codereview.appspot.com/271240043/diff/20001/lib/nss/nss.def
File lib/nss/nss.def (right):

https://codereview.appspot.com/271240043/diff/20001/lib/nss/nss.def#newcode215
lib/nss/nss.def:215: PK11_VerifyWithMechanism;
Please get rrelyea's review for this.

https://codereview.appspot.com/271240043/diff/20001/lib/pk11wrap/pk11obj.c
File lib/pk11wrap/pk11obj.c (right):

https://codereview.appspot.com/271240043/diff/20001/lib/pk11wrap/pk11obj.c#ne...
lib/pk11wrap/pk11obj.c:678: SECItem *param, const SECItem *sig,
Shouldn't param and sig be const?

https://codereview.appspot.com/271240043/diff/20001/lib/pk11wrap/pk11obj.c#ne...
lib/pk11wrap/pk11obj.c:765: SECItem *param, SECItem *sig, const SECItem *hash)
Shouldn't param be const?

[ttaubert, 2015-11-09 17:27:18]

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
File external_tests/pk11_gtest/manifest.mn (left):

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/manifest.mn:9: # These sources have access to libssl
internals
On 2015/11/06 21:31:58, ekr-webrtc wrote:
> Rietveld seems to be a bit confused and think that this is a change to the
> ssl_gtest file. Any idea what's up there?

Yeah, that's weird. The local patch adds a new file. I'll see what I can do.

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
File external_tests/pk11_gtest/pk11_rsapss_unittest.cc (right):

https://codereview.appspot.com/271240043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_rsapss_unittest.cc:180:
SECKEY_DestroySubjectPublicKeyInfo(spki);
On 2015/11/06 21:31:59, ekr-webrtc wrote:
> Instead, use the types in:
>
https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/ScopedNSS...

Oh right, I've used them before for WebCrypto but I didn't think about using
those here too. So instead of copying the files I added a gtest_utils.h that
includes a few lines to support scoped pointers.

[mt, 2015-11-09 18:04:14]

LGTM

https://codereview.appspot.com/271240043/diff/40001/external_tests/pk11_gtest...
File external_tests/pk11_gtest/Makefile (right):

https://codereview.appspot.com/271240043/diff/40001/external_tests/pk11_gtest...
external_tests/pk11_gtest/Makefile:54: OS_CFLAGS += -MTd -EHsc -nologo
Check the latest version here.  The -MD/MTd flags are removed.

https://codereview.appspot.com/271240043/diff/40001/external_tests/pk11_gtest...
File external_tests/pk11_gtest/pk11_rsapss_unittest.cc (right):

https://codereview.appspot.com/271240043/diff/40001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_rsapss_unittest.cc:135: size_t data_len, const
uint8_t* sig, size_t sig_len) {
Same comment as I made for the other patch: you don't need these variables as
state on the test, just pass arguments to verify.

https://codereview.appspot.com/271240043/diff/40001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_rsapss_unittest.cc:204: false, false, nullptr));
You should ASSERT_TRUE(!!privKey) and pubKeyRaw here.

https://codereview.appspot.com/271240043/diff/40001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_rsapss_unittest.cc:209: ScopedSECItem
data(SECITEM_AllocItem(nullptr, nullptr, hLen));
Wouldn't it be easier to do this:
uint8_t dataBuf[50];
SECItem data = { siBuffer, dataBuf, sizeof(dataBuf) };

Then you don't have to check the allocation result.

https://codereview.appspot.com/271240043/diff/40001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_rsapss_unittest.cc:215:
PK11_SignatureLen(privKey.get())));
vector<uint8_t> sigBuf(PK11_SignatureLen(privKey.get()));
SECItem sig = { siBuffer, &sigBuf[0], sigBuf.size() };

https://codereview.appspot.com/271240043/diff/40001/lib/nss/nss.def
File lib/nss/nss.def (right):

https://codereview.appspot.com/271240043/diff/40001/lib/nss/nss.def#newcode208
lib/nss/nss.def:208: PK11_SignWithMechanism;
Please put these in a new section at the bottom for the 3.22 release.

[ekr-rietveld, 2015-11-09 18:37:07]

LGTM with changes below

https://codereview.appspot.com/271240043/diff/40001/external_tests/pk11_gtest...
File external_tests/pk11_gtest/gtest_utils.h (right):

https://codereview.appspot.com/271240043/diff/40001/external_tests/pk11_gtest...
external_tests/pk11_gtest/gtest_utils.h:34: }  // namespace nss_test
This is good stuff, but let's put it in a separate file.

https://codereview.appspot.com/271240043/diff/40001/external_tests/pk11_gtest...
File external_tests/pk11_gtest/pk11_gtest.cc (right):

https://codereview.appspot.com/271240043/diff/40001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_gtest.cc:14: NSS_NoDB_Init(NULL);
nullptr

[ttaubert, 2015-11-09 19:06:00]

All feedback addressed, thanks!

[mt, 2015-11-09 19:17:36]

LGTM with one nit

https://codereview.appspot.com/271240043/diff/60001/external_tests/common/sco...
File external_tests/common/scoped_ptrs.h (right):

https://codereview.appspot.com/271240043/diff/60001/external_tests/common/sco...
external_tests/common/scoped_ptrs.h:8: #define gtest_utils_h__
Fix name

[ttaubert, 2015-11-09 19:43:02]

On 2015/11/09 19:17:36, mt wrote:
> LGTM with one nit
> 
>
https://codereview.appspot.com/271240043/diff/60001/external_tests/common/sco...
> File external_tests/common/scoped_ptrs.h (right):
> 
>
https://codereview.appspot.com/271240043/diff/60001/external_tests/common/sco...
> external_tests/common/scoped_ptrs.h:8: #define gtest_utils_h__
> Fix name

Oh, thanks for spotting this.