Add some policy controls for VerifySignedData().

Add some policy controls for VerifySignedData().

This allows controlling:
 * What RSA key sizes are accepted
 * What digest algorithms are accepted
 * What ellipitc curves are accepted

This policy is expressed through a separate interface. I expect this interface will morph into something more generic for certificate verifying.

BUG=410574
Committed: https://crrev.com/5d7c3b4c9b4c7e5f40be3b125f37fd8f6b2f9548
Cr-Commit-Position: refs/heads/master@{#342292}

[eroman, 2015-07-28 17:04:30]

eroman@chromium.org changed reviewers:
+ davidben@chromium.org, rsleevi@chromium.org

[Ryan Sleevi, 2015-07-28 21:14:53]

https://codereview.chromium.org/1259313002/diff/1/net/cert/internal/verificat...
File net/cert/internal/verification_policy.h (right):

https://codereview.chromium.org/1259313002/diff/1/net/cert/internal/verificat...
net/cert/internal/verification_policy.h:17: class NET_EXPORT VerificationPolicy
{
Naming wise, this seems like it should be SignaturePolicy? It's what sort of
signatures are accepted?

https://codereview.chromium.org/1259313002/diff/1/net/cert/internal/verificat...
net/cert/internal/verification_policy.h:26: virtual bool
IsAcceptableDigestAlgorithm(DigestAlgorithm algorithm) const = 0;
I'm a little torn on this interface, whether it should take the signature as a
whole. The concerns for MGF-1 are different from that of RSA-SSA, for example.

https://codereview.chromium.org/1259313002/diff/1/net/cert/internal/verificat...
net/cert/internal/verification_policy.h:36: size_t modulus_length_bits) const =
0;
We arguably similarly have an EC policy (nothing less than 256 is acceptable).
I'm wondering if that should generalize into a minimum size policy for an
algorithm.

Similarly, a maximum size may also be desirable to express, since the
implementation may support greater than the CPU allows. Although I'm not sure
how necessary that is.

https://codereview.chromium.org/1259313002/diff/1/net/cert/internal/verificat...
net/cert/internal/verification_policy.h:45: class NET_EXPORT
BaseVerificationPolicy : public VerificationPolicy {
Curious why this split?

That is, why a pure virtual plus a concrete? Why not an overridable concrete
implementation? It seems that most implementations are likely to derive from
this policy, rather than the pure virtual, and then... what, delegate down to
the base class? Or do you see this being something compositional?

[eroman, 2015-07-29 02:44:43]

https://codereview.chromium.org/1259313002/diff/1/net/cert/internal/verificat...
File net/cert/internal/verification_policy.h (right):

https://codereview.chromium.org/1259313002/diff/1/net/cert/internal/verificat...
net/cert/internal/verification_policy.h:17: class NET_EXPORT VerificationPolicy
{
On 2015/07/28 21:14:53, Ryan Sleevi wrote:
> Naming wise, this seems like it should be SignaturePolicy? It's what sort of
> signatures are accepted?

Done.

https://codereview.chromium.org/1259313002/diff/1/net/cert/internal/verificat...
net/cert/internal/verification_policy.h:26: virtual bool
IsAcceptableDigestAlgorithm(DigestAlgorithm algorithm) const = 0;
On 2015/07/28 21:14:53, Ryan Sleevi wrote:
> I'm a little torn on this interface, whether it should take the signature as a
> whole. The concerns for MGF-1 are different from that of RSA-SSA, for example.

Done (Changed to take a signature algorithm instead).

Taking the signature algorithm is more flexible, and probably the right API.
(My original concern was that it would be easy to forget about RSASSA-PSS
parameters)

https://codereview.chromium.org/1259313002/diff/1/net/cert/internal/verificat...
net/cert/internal/verification_policy.h:36: size_t modulus_length_bits) const =
0;
On 2015/07/28 21:14:53, Ryan Sleevi wrote:
> We arguably similarly have an EC policy (nothing less than 256 is acceptable).
> I'm wondering if that should generalize into a minimum size policy for an
> algorithm.
> 

This should already be covered by the IsAcceptableCurveForEcdsa() which lets you
accept/reject based on the named curve.

The key size in this case is implied from the named curve itself, and the
default implementation will enforce a minimum key size of 256 bits by virtue of
accepting curves secp256r1, secp384r1, secp521r1.

I could modify the API to explicitly take the group degree size in bits (i.e.
256, 384, 521 for the currently supported curves) to make this more apparent,
but I suspect it is easy enough to do this given the named curve (just a few
function calls in OpenSSL) that this isn't worthwhile.

https://codereview.chromium.org/1259313002/diff/1/net/cert/internal/verificat...
net/cert/internal/verification_policy.h:45: class NET_EXPORT
BaseVerificationPolicy : public VerificationPolicy {
On 2015/07/28 21:14:53, Ryan Sleevi wrote:
> Curious why this split?
> 
> That is, why a pure virtual plus a concrete? Why not an overridable concrete
> implementation? It seems that most implementations are likely to derive from
> this policy, rather than the pure virtual, and then... what, delegate down to
> the base class? Or do you see this being something compositional?

My reason for the split mainly came because of not wanting the
"min_rsa_modulus_length_bits" member to be in the base class (since if you
implemented a modulus size policy that didn't use a minimum, this is a pointless
member).

I have changed this around a bit so that:
  * The base class includes the default policy, and enforces RSA key sizes >=
2048
  * A helper class provides a canned implemenation for applying a variable
minimum RSA key size

[Ryan Sleevi, 2015-08-01 01:42:50]

LGTM, but David, would you mind spot-checking to make sure I didn't miss
anything?

https://codereview.chromium.org/1259313002/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1259313002/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_signed_data.cc:174: unsigned int modulus_length_bits =
BN_num_bits(rsa.get()->n);
ScopedRSA is just a ScopedOpenSSL aka a scoped_ptr

So shouldn't this just be

BN_num_bits(rsa->n);

[eroman, 2015-08-01 02:20:45]

https://codereview.chromium.org/1259313002/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1259313002/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_signed_data.cc:174: unsigned int modulus_length_bits =
BN_num_bits(rsa.get()->n);
On 2015/08/01 01:42:50, Ryan Sleevi wrote:
> ScopedRSA is just a ScopedOpenSSL aka a scoped_ptr
> 
> So shouldn't this just be
> 
> BN_num_bits(rsa->n);

Done (copy paste sloppyiness)

[davidben, 2015-08-03 18:52:49]

https://codereview.chromium.org/1259313002/diff/60001/net/cert/internal/signa...
File net/cert/internal/signature_policy.cc (right):

https://codereview.chromium.org/1259313002/diff/60001/net/cert/internal/signa...
net/cert/internal/signature_policy.cc:10: #include <openssl/obj_mac.h>
Better to include obj.h I think. obj_mac.h is really weird and lacks an #include
guard.

(It's not clear there's a good reason for this and we can probably just make
that header standalone, but it currently isn't.)

https://codereview.chromium.org/1259313002/diff/60001/net/cert/internal/signa...
File net/cert/internal/signature_policy.h (right):

https://codereview.chromium.org/1259313002/diff/60001/net/cert/internal/signa...
net/cert/internal/signature_policy.h:33: // NID_secp384r1).
This means the interface has a BoringSSL dependency. I'm assuming this is
considered okay. (I certainly don't object. If anything, I think we should
depend on it more and use CBS_get_asn1 to parse DER.)

https://codereview.chromium.org/1259313002/diff/60001/net/cert/internal/signa...
net/cert/internal/signature_policy.h:48: class NET_EXPORT SimpleSignaturePolicy
: public SignaturePolicy {
Do you expect this to be used outside of the test you put it in? Wondering if it
should just be in that one file. (I suppose there's if you want a default
policy. You actually could just instantiate SignaturePolicy as things are now,
though that's kinda odd.)

https://codereview.chromium.org/1259313002/diff/60001/net/cert/internal/verif...
File net/cert/internal/verify_signed_data_unittest.cc (right):

https://codereview.chromium.org/1259313002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_signed_data_unittest.cc:19: #include
<openssl/obj_mac.h>
Ditto about obj_mac.h being weird.

[eroman, 2015-08-06 21:15:38]

https://codereview.chromium.org/1259313002/diff/60001/net/cert/internal/signa...
File net/cert/internal/signature_policy.cc (right):

https://codereview.chromium.org/1259313002/diff/60001/net/cert/internal/signa...
net/cert/internal/signature_policy.cc:10: #include <openssl/obj_mac.h>
On 2015/08/03 18:52:48, David Benjamin wrote:
> Better to include obj.h I think. obj_mac.h is really weird and lacks an
#include
> guard.
> 
> (It's not clear there's a good reason for this and we can probably just make
> that header standalone, but it currently isn't.)

Done.

https://codereview.chromium.org/1259313002/diff/60001/net/cert/internal/signa...
File net/cert/internal/signature_policy.h (right):

https://codereview.chromium.org/1259313002/diff/60001/net/cert/internal/signa...
net/cert/internal/signature_policy.h:33: // NID_secp384r1).
On 2015/08/03 18:52:48, David Benjamin wrote:
> This means the interface has a BoringSSL dependency. I'm assuming this is
> considered okay. (I certainly don't object. If anything, I think we should
> depend on it more and use CBS_get_asn1 to parse DER.)

Acknowledged (left the dependency as-is)

https://codereview.chromium.org/1259313002/diff/60001/net/cert/internal/signa...
net/cert/internal/signature_policy.h:48: class NET_EXPORT SimpleSignaturePolicy
: public SignaturePolicy {
On 2015/08/03 18:52:49, David Benjamin wrote:
> Do you expect this to be used outside of the test you put it in? Wondering if
it
> should just be in that one file. (I suppose there's if you want a default
> policy. You actually could just instantiate SignaturePolicy as things are now,
> though that's kinda odd.)

I *think* it will be used outside of the test, since I suspect a typical
behavior will be to verify using either 1024-bit min RSA keys or 2048-bit min
RSA keys, but not entirely sure how that will shake out.

Left it here for now but added myself a TODO(eroman) to follow-up on this once
we have the callers better fleshed out.

[davidben, 2015-08-06 21:32:04]

lgtm

https://codereview.chromium.org/1259313002/diff/90001/net/cert/internal/verif...
File net/cert/internal/verify_signed_data_unittest.cc (right):

https://codereview.chromium.org/1259313002/diff/90001/net/cert/internal/verif...
net/cert/internal/verify_signed_data_unittest.cc:21: #include
<openssl/obj_mac.h>
(This one also should be obj.h.)

https://codereview.chromium.org/1259313002/diff/90001/net/cert/internal/verif...
net/cert/internal/verify_signed_data_unittest.cc:353: if
(algorithm.ParamsForRsaPss() &&
Would algorithm.algorithm() == SignatureAlgorithmId::RsaPss be better, or is the
intent that calling ParamsForFoo is a suitable proxy for checking the type?

https://codereview.chromium.org/1259313002/diff/90001/net/cert/internal/verif...
net/cert/internal/verify_signed_data_unittest.cc:381: // exclusively SHA256
should pass).
Nit: maybe phrase this as 'the tests using SHA512 should fail', since you don't
really care if it accepts a sha384 or something.

[eroman, 2015-08-06 22:22:30]

https://codereview.chromium.org/1259313002/diff/90001/net/cert/internal/verif...
File net/cert/internal/verify_signed_data_unittest.cc (right):

https://codereview.chromium.org/1259313002/diff/90001/net/cert/internal/verif...
net/cert/internal/verify_signed_data_unittest.cc:21: #include
<openssl/obj_mac.h>
On 2015/08/06 21:32:04, David Benjamin wrote:
> (This one also should be obj.h.)

Done. Sorry about not remembering that!

https://codereview.chromium.org/1259313002/diff/90001/net/cert/internal/verif...
net/cert/internal/verify_signed_data_unittest.cc:353: if
(algorithm.ParamsForRsaPss() &&
On 2015/08/06 21:32:04, David Benjamin wrote:
> Would algorithm.algorithm() == SignatureAlgorithmId::RsaPss be better, or is
the
> intent that calling ParamsForFoo is a suitable proxy for checking the type?

Done.

https://codereview.chromium.org/1259313002/diff/90001/net/cert/internal/verif...
net/cert/internal/verify_signed_data_unittest.cc:381: // exclusively SHA256
should pass).
On 2015/08/06 21:32:04, David Benjamin wrote:
> Nit: maybe phrase this as 'the tests using SHA512 should fail', since you
don't
> really care if it accepts a sha384 or something.

Done.

[eroman, 2015-08-07 00:29:03]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-08-07 00:29:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1259313002/#ps110001
(title: "Address more comments")

[commit-bot: I haz the power, 2015-08-07 00:29:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259313002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259313002/110001

[commit-bot: I haz the power, 2015-08-07 02:06:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 02:06:37]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[eroman, 2015-08-07 03:26:33]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2015-08-07 03:26:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259313002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259313002/110001

[commit-bot: I haz the power, 2015-08-07 05:20:28]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-08-07 05:21:20]

Patchset 7 (id:??) landed as
https://crrev.com/5d7c3b4c9b4c7e5f40be3b125f37fd8f6b2f9548
Cr-Commit-Position: refs/heads/master@{#342292}