Add SignatureVerifier::VerifyInitRSAPSS for verifying RSA-PSS signatures.

Add SignatureVerifier::VerifyInitRSAPSS for verifying RSA-PSS signatures.

Change the OpenSSL-based SignatureVerifier to use EVP_DigestVerifyInit
instead of EVP_VerifyInit_ex.

Copy the PSS padding verification code from NSS to the NSS-based
SignatureVerifier because the RSA-PSS code in the NSS softoken isn't
exposed via the NSS PK11_ or VFY_ functions yet.

R=agl@chromium.org,rsleevi@chromium.org
BUG=none
TEST=to be added to net_unittests via testing net::quic::ProofVerifier.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209178

[agl, 2013-06-26 15:21:09]

LGTM.

(I did not check the imported NSS code.)

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier.h
File crypto/signature_verifier.h (right):

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier.h#n...
crypto/signature_verifier.h:75: // An RSA-PSS signature is encoded as a big
integer in the big-endian byte
s/the //

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier.h#n...
crypto/signature_verifier.h:88: int signature_len,
nit: size_t for lengths?

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_ope...
File crypto/signature_verifier_openssl.cc (right):

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_ope...
crypto/signature_verifier_openssl.cc:27: default:
Just a thought: if we omit the default case, would the compiler tell us that a
case was missing in the event that we add another value in the enum? A compile
time error would be nicer than failing/crashing.

[wtc, 2013-06-26 16:13:37]

On second thought, I will work on a unit test today.
But please feel free to review patch set 1 first. Thanks.

Also, the CommonInit and DecodePublicKeyInfo private methods
are intentionally defined in the .cc files near where the
original code was, to facilitate code review. I will move
their definitions to the proper position before I commit this
CL.

[Ryan Sleevi, 2013-06-26 19:48:28]

Unit tests would be good. Otherwise LGTM, mod BUG/LEAK

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_nss.cc
File crypto/signature_verifier_nss.cc (right):

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_nss...
crypto/signature_verifier_nss.cc:99: int public_key_info_len) {
Can you make it an error to call VerifyInit() and VerifyInitRSAPSS() without
having called Final (eg: to make sure no one attempts to re-initialize a
SignatureVerifier)

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_nss...
crypto/signature_verifier_nss.cc:218: signature_.clear();
LEAK? What happens to public_key_ here. Seems like it doesn't end up getting
destroyed.

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_ope...
File crypto/signature_verifier_openssl.cc (right):

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_ope...
crypto/signature_verifier_openssl.cc:27: default:
On 2013/06/26 15:21:10, agl wrote:
> Just a thought: if we omit the default case, would the compiler tell us that a
> case was missing in the event that we add another value in the enum? A compile
> time error would be nicer than failing/crashing.

Yes, it would. So +1 to omitting.

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_ope...
crypto/signature_verifier_openssl.cc:82: ScopedOpenSSL<X509_ALGOR,
X509_ALGOR_free> algorithm(
BUG? No OpenSSLErrStackTracer to wipe errors here

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_ope...
crypto/signature_verifier_openssl.cc:87: DCHECK(digest);
Shouldn't this be a handled case?

It seems like if |digest| is NULL, then during the EVP_DigestVerifyInit() call,
OpenSSL will try to grab the default digest for the key type, which may result
in a hash confusion attack.

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_ope...
crypto/signature_verifier_openssl.cc:100: const EVP_MD* digest =
ToOpenSSLDigest(hash_alg);
BUG? No OpenSSLErrStackTracer to wipe errors here

[wtc, 2013-06-27 02:23:51]

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier.h
File crypto/signature_verifier.h (right):

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier.h#n...
crypto/signature_verifier.h:75: // An RSA-PSS signature is encoded as a big
integer in the big-endian byte

On 2013/06/26 15:21:10, agl wrote:
> s/the //

Done.

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier.h#n...
crypto/signature_verifier.h:88: int signature_len,

On 2013/06/26 15:21:10, agl wrote:
> nit: size_t for lengths?

I will do this in a separate cleanup CL because it will require OWNERS
review of changes outside src/crypto.

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_nss.cc
File crypto/signature_verifier_nss.cc (right):

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_nss...
crypto/signature_verifier_nss.cc:99: int public_key_info_len) {

On 2013/06/26 19:48:28, Ryan Sleevi wrote:
> Can you make it an error to call VerifyInit() and VerifyInitRSAPSS() without
> having called Final (eg: to make sure no one attempts to re-initialize a
> SignatureVerifier)

Done.

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_nss...
crypto/signature_verifier_nss.cc:218: signature_.clear();

On 2013/06/26 19:48:28, Ryan Sleevi wrote:
> LEAK? What happens to public_key_ here. Seems like it doesn't end up getting
> destroyed.

public_key_ is destroyed on lines 214-217 above.

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_ope...
File crypto/signature_verifier_openssl.cc (right):

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_ope...
crypto/signature_verifier_openssl.cc:27: default:

On 2013/06/26 19:48:28, Ryan Sleevi wrote:
>
> Yes, it would. So +1 to omitting.

Done.

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_ope...
crypto/signature_verifier_openssl.cc:82: ScopedOpenSSL<X509_ALGOR,
X509_ALGOR_free> algorithm(

On 2013/06/26 19:48:28, Ryan Sleevi wrote:
> BUG? No OpenSSLErrStackTracer to wipe errors here

Done.

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_ope...
crypto/signature_verifier_openssl.cc:87: DCHECK(digest);

On 2013/06/26 19:48:28, Ryan Sleevi wrote:
> Shouldn't this be a handled case?

Done.

I assume |digest| can only be null if OpenSSL has a bug. But it is fine to
just handle this case.

https://codereview.chromium.org/17776003/diff/1/crypto/signature_verifier_ope...
crypto/signature_verifier_openssl.cc:100: const EVP_MD* digest =
ToOpenSSLDigest(hash_alg);

On 2013/06/26 19:48:28, Ryan Sleevi wrote:
> BUG? No OpenSSLErrStackTracer to wipe errors here

Done.

https://codereview.chromium.org/17776003/diff/25001/crypto/signature_verifier...
File crypto/signature_verifier_openssl.cc (right):

https://codereview.chromium.org/17776003/diff/25001/crypto/signature_verifier...
crypto/signature_verifier_openssl.cc:54: OpenSSLErrStackTracer
err_tracer(FROM_HERE);

rsleevi: I am not familiar with OpenSSLErrStackTracer.
Could you please doublecheck all uses of OpenSSLErrStackTracer
in this file? In particular, do we need to use
OpenSSLErrStackTracer in CommonInit(), whose callers
already use OpenSSLErrStackTracer?

[Ryan Sleevi, 2013-06-27 16:51:08]

lgtm

https://codereview.chromium.org/17776003/diff/25001/crypto/signature_verifier...
File crypto/signature_verifier_openssl.cc (right):

https://codereview.chromium.org/17776003/diff/25001/crypto/signature_verifier...
crypto/signature_verifier_openssl.cc:54: OpenSSLErrStackTracer
err_tracer(FROM_HERE);
On 2013/06/27 02:23:51, wtc wrote:
> 
> rsleevi: I am not familiar with OpenSSLErrStackTracer.
> Could you please doublecheck all uses of OpenSSLErrStackTracer
> in this file? In particular, do we need to use
> OpenSSLErrStackTracer in CommonInit(), whose callers
> already use OpenSSLErrStackTracer?

Ah, as long as both callers have an err tracer, no, it's not needed.

Basically, any time you're calling an OpenSSL function, you want to make sure
that at unwind time, there's at least a tracer on the stack.

[commit-bot: I haz the power, 2013-06-27 18:12:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/17776003/44001

[commit-bot: I haz the power, 2013-06-27 18:29:03]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[wtc, 2013-06-27 19:12:53]

In patch set 5 I changed the switch statements to avoid a warning from
GCC and MSVC:

../../crypto/signature_verifier_nss.cc: In function 'HASH_HashType
crypto::{anonymous}::ToNSSHashType(crypto::SignatureVerifier::HashAlgorithm)':
../../crypto/signature_verifier_nss.cc:29:1: error: control reaches end of
non-void function [-Werror=return-type]

I verified that GCC still warns if the switch statement doesn't handle
the SHA1 or SHA256 case.

[commit-bot: I haz the power, 2013-06-27 19:13:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/17776003/64001

[commit-bot: I haz the power, 2013-06-27 19:36:58]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-28 02:12:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/17776003/53003

[commit-bot: I haz the power, 2013-06-28 17:46:56]

Change committed as 209178