Add DER parsing of AlgorithmId for signatures.

Add DER parsing of AlgorithmId for signatures.

This initial implementation supports:

  * sha-1WithRSAEncryption
  * sha256WithRSAEncryption
  * sha384WithRSAEncryption
  * sha512WithRSAEncryption
  * ecdsa-with-SHA1
  * ecdsa-with-SHA256
  * ecdsa-with-SHA38
  * ecdsa-with-SHA512

BUG=410574
Committed: https://crrev.com/68de6fee236e99393cfd462b0698cebb50c55d9a
Cr-Commit-Position: refs/heads/master@{#339088}

[eroman, 2015-06-29 01:54:03]

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

[Ryan Sleevi, 2015-06-29 14:45:25]

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
File net/cert/internal/signature_algorithm.cc (right):

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
net/cert/internal/signature_algorithm.cc:85: WARN_UNUSED_RESULT bool
AssignFromDerEmptyParams(
end of line, not beginning.

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
File net/cert/internal/signature_algorithm.h (right):

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
net/cert/internal/signature_algorithm.h:36: WARN_UNUSED_RESULT bool
AssignFromDer(const der::Input& signature_algorithm);
WARN_UNUSED_RESULT historically goes on the end of the line, rather than the
beginning. It also makes things more readable :)

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
net/cert/internal/signature_algorithm.h:39: WARN_UNUSED_RESULT bool Equals(const
SignatureAlgorithm& other) const;
Ditto

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
File net/cert/internal/signature_algorithm_unittest.cc (right):

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
net/cert/internal/signature_algorithm_unittest.cc:49: 0x05};
See
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/internal/...
for the suggested way to structure this comment (similar to line 34/35, but
makes these easier)

https://codereview.chromium.org/1218753002/diff/40001/net/der/input.h
File net/der/input.h (right):

https://codereview.chromium.org/1218753002/diff/40001/net/der/input.h#newcode61
net/der/input.h:61: return Equals(Input(data));
So, this isn't ideal because it double-instantiates templates. You could do
Equals(Input(data, size)) to avoid the double instantiation, or simply leave it
to the caller to do Equals(Input(kTestData))

Either of those are more preferable than Equals(Input(Data)) in here.

[eroman, 2015-06-29 15:19:00]

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
File net/cert/internal/signature_algorithm.cc (right):

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
net/cert/internal/signature_algorithm.cc:85: WARN_UNUSED_RESULT bool
AssignFromDerEmptyParams(
On 2015/06/29 14:45:24, Ryan Sleevi (slow through 7-15 wrote:
> end of line, not beginning.

OK, I changed the other instance, but had to leave this one unchanged.

The problem is that the compiler does not accept WARN_UNUSED_RESULT at the end
of function definitions (only declarations), which is why I tend to prefer
putting it at the start for consistency:

../../net/cert/internal/signature_algorithm.cc:89:30: error: GCC does not allow
'warn_unused_result' attribute in this position on a function definition
[-Werror,-Wgcc-compat]
    SignatureAlgorithm* out) WARN_UNUSED_RESULT {

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
File net/cert/internal/signature_algorithm_unittest.cc (right):

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
net/cert/internal/signature_algorithm_unittest.cc:49: 0x05};
On 2015/06/29 14:45:24, Ryan Sleevi (slow through 7-15 wrote:
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/internal/...
> for the suggested way to structure this comment (similar to line 34/35, but
> makes these easier)

I had used clang-format off in the earlier patchset (2), but I ended up not
caring for it and reverted it in the patchset you reviewed.

My slight preference is to just let clang-format do its thing, but I can change
this if you like.

https://codereview.chromium.org/1218753002/diff/40001/net/der/input.h
File net/der/input.h (right):

https://codereview.chromium.org/1218753002/diff/40001/net/der/input.h#newcode61
net/der/input.h:61: return Equals(Input(data));
On 2015/06/29 14:45:24, Ryan Sleevi (slow through 7-15 wrote:
> So, this isn't ideal because it double-instantiates templates. You could do
> Equals(Input(data, size)) to avoid the double instantiation, or simply leave
it
> to the caller to do Equals(Input(kTestData))
> 
> Either of those are more preferable than Equals(Input(Data)) in here.

Removed.

[Ryan Sleevi, 2015-06-29 16:06:57]

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
File net/cert/internal/signature_algorithm.cc (right):

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
net/cert/internal/signature_algorithm.cc:85: WARN_UNUSED_RESULT bool
AssignFromDerEmptyParams(
On 2015/06/29 15:19:00, eroman wrote:
> On 2015/06/29 14:45:24, Ryan Sleevi (slow through 7-15 wrote:
> > end of line, not beginning.
> 
> OK, I changed the other instance, but had to leave this one unchanged.
> 
> The problem is that the compiler does not accept WARN_UNUSED_RESULT at the end
> of function definitions (only declarations), which is why I tend to prefer
> putting it at the start for consistency:
> 
> ../../net/cert/internal/signature_algorithm.cc:89:30: error: GCC does not
allow
> 'warn_unused_result' attribute in this position on a function definition
> [-Werror,-Wgcc-compat]
>     SignatureAlgorithm* out) WARN_UNUSED_RESULT {

Yeah, this is part of why Peter and several others push very hard against W_U_R,
unfortunately.

I can live with this inconsistency - I think it's more important for the headers
to clearly indicate to readers the most salient bits (return values, parameters)
than on the pre/post conditions. Here, I'd be OK w/ dropping it entirely, but I
suspect you'd argue for it, so I'm fine with the inconsistency :)

[eroman, 2015-06-29 16:17:47]

Left the W_U_R in, because I have a thing for them :)

[Ryan Sleevi, 2015-06-29 16:36:29]

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
File net/cert/internal/signature_algorithm_unittest.cc (right):

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
net/cert/internal/signature_algorithm_unittest.cc:49: 0x05};
On 2015/06/29 15:19:00, eroman wrote:
 > My slight preference is to just let clang-format do its thing, but I can
change
> this if you like.

My issue wasn't (just) the clang-format-off (which is more of the nitty bit),
but about how the description of the structure was focused.

If you look at Nick's code, I'm curious if you find that more readable. To me, I
think it is, compared to the line 59/60/61 bit.

Thoughts?

https://codereview.chromium.org/1218753002/diff/80001/net/cert/internal/signa...
File net/cert/internal/signature_algorithm.h (right):

https://codereview.chromium.org/1218753002/diff/80001/net/cert/internal/signa...
net/cert/internal/signature_algorithm.h:31: struct NET_EXPORT SignatureAlgorithm
{
DESIGN: It seems weird to have a structure with enum members that can be created
in an undefined/ambiguous state.

Should this be moved to a proper class-type with explicit constructors &
copying? What if someone just slings a ('null') SignatureAlgorithm around, which
looks like it'd be seen as RSA-PKCS#1 v1.5 with SHA-1

https://codereview.chromium.org/1218753002/diff/80001/net/cert/internal/signa...
net/cert/internal/signature_algorithm.h:44: // TODO(eroman): Add support for
RSASSA-PSS.
Is the idea being that you'll support RSASSA-PSS via explicit sub members? What
about the work for EdDSA (Curve25519), which may have similar sub-parameters
beyond hash? (I can never quite follow it)

I'm just curious how you see handling SignatureAlgorithms with params in a
general case, with PSS being a concrete example.

[eroman, 2015-06-29 17:05:06]

https://codereview.chromium.org/1218753002/diff/80001/net/cert/internal/signa...
File net/cert/internal/signature_algorithm.h (right):

https://codereview.chromium.org/1218753002/diff/80001/net/cert/internal/signa...
net/cert/internal/signature_algorithm.h:31: struct NET_EXPORT SignatureAlgorithm
{
On 2015/06/29 16:36:29, Ryan Sleevi (slow through 7-15 wrote:
> DESIGN: It seems weird to have a structure with enum members that can be
created
> in an undefined/ambiguous state.
> 
> Should this be moved to a proper class-type with explicit constructors &
> copying? What if someone just slings a ('null') SignatureAlgorithm around,
which
> looks like it'd be seen as RSA-PKCS#1 v1.5 with SHA-1

I will switch this to a full blown class.

https://codereview.chromium.org/1218753002/diff/80001/net/cert/internal/signa...
net/cert/internal/signature_algorithm.h:44: // TODO(eroman): Add support for
RSASSA-PSS.
On 2015/06/29 16:36:29, Ryan Sleevi (slow through 7-15 wrote:
> Is the idea being that you'll support RSASSA-PSS via explicit sub members?
What
> about the work for EdDSA (Curve25519), which may have similar sub-parameters
> beyond hash? (I can never quite follow it)
> 
> I'm just curious how you see handling SignatureAlgorithms with params in a
> general case, with PSS being a concrete example.

Let me go ahead and add RSASSA-PSS support to this changelist, and then we can
discuss the design in more concrete terms.

My thinking for RSASSA-PSS was to just inline the salt length parameter into
SignatureAlgorithm, and then add an enum for RsaSsaPss. I figured for the
current set of verification algorithms this approach would be sufficient.

When we out-grow this and need to add more parameters for future algorithms, we
could switch to using something like WebCryptoAlgorithm which allows for a
separate heap-allocated parameters object.

An alternate design would be to keep the DER-encoded AlgorithmIdentifier as the
canonical representation. This solves needing to duplicate the representation
for each algorithm. The downside is this can lead to code that re-parses the DER
in several places (when validating the algorithm vs doing the verification vs
comparing algorithms), which I think makes for a weaker layering.

[eroman, 2015-06-30 15:53:15]

PTAL - I generalized SignatureAlgorithm to support RSASSA-PSS and its
parameters.

I am keeping the actual parsing or PSS out of this changelist, since it is
complicated enough to be reviewed separately (moreover I haven't finished
implementing it :)

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
File net/cert/internal/signature_algorithm_unittest.cc (right):

https://codereview.chromium.org/1218753002/diff/40001/net/cert/internal/signa...
net/cert/internal/signature_algorithm_unittest.cc:49: 0x05};
On 2015/06/29 16:36:28, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/06/29 15:19:00, eroman wrote:
>  > My slight preference is to just let clang-format do its thing, but I can
> change
> > this if you like.
> 
> My issue wasn't (just) the clang-format-off (which is more of the nitty bit),
> but about how the description of the structure was focused.
> 
> If you look at Nick's code, I'm curious if you find that more readable. To me,
I
> think it is, compared to the line 59/60/61 bit.
> 
> Thoughts?

Done

[Ryan Sleevi, 2015-07-07 13:55:13]

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

[Ryan Sleevi, 2015-07-07 13:55:15]

I'm reasonably OK with this, but I wanted to pass through David for a second
degree of sanity check - he's had to deal with enough 'broken APIs' that he may
have an opinion on this.

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
File net/cert/internal/signature_algorithm.cc (right):

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:113: return
!algorithm_identifier_parser.HasMore();
It's perfectly fine for an algorithm to have additional data.

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:169: // regards - it additionally
accepts NULL parameters.
Why?

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
File net/cert/internal/signature_algorithm.h (right):

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:17: }
nit: }  // namespace der

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:19: // Enumeration that describes a
(parameter-less) hashing algorithm.
nit: This comment doesn't seem to really add much. On a semantic level, it
arguably names rather than describes, but there's arguable nuance in that
terminology.

// The digest algorithm used within a signature.

Might be slightly more direct?

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:28: // algorithm are specified
separately).
// The signature scheme used within a signature. Parameters are specified
separately.

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:52: // The trailer is assumed to be 1,
and the mask gen to be MGF1.
Why?

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:73: SignatureAlgorithm();
Why init+Assign vs a factory create? Is the assumption to avoid the heap?

[eroman, 2015-07-08 01:09:35]

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
File net/cert/internal/signature_algorithm.cc (right):

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:113: return
!algorithm_identifier_parser.HasMore();
On 2015/07/07 13:55:15, Ryan Sleevi (slow through 7-15 wrote:
> It's perfectly fine for an algorithm to have additional data.

For anyone following, this is the same conversation with more detail on
https://codereview.chromium.org/1217653006/.

Also FTR Mozilla does not allow unrecognized parameters in their signature
AlgorithmIdentifiers:
 
http://lxr.mozilla.org/mozilla-central/source/security/pkix/lib/pkixcheck.cpp#45

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:169: // regards - it additionally
accepts NULL parameters.
On 2015/07/07 13:55:15, Ryan Sleevi (slow through 7-15 wrote:
> Why?

Isn't this a moot point based on your earlier comment?

If we allow unrecognized data in AlgorithmIdentifier, wouldn't we also then
accept NULL parameters?
Since it wouldn't be distinguishable from having omitted the optional field, and
then added a new unrecognized field with NULL.

I added the NULL check originally to match Firefox, which allows both empty and
NULL for each flavor of RSA PKCS1 and ECDSA.

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
File net/cert/internal/signature_algorithm.h (right):

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:17: }
On 2015/07/07 13:55:15, Ryan Sleevi (slow through 7-15 wrote:
> nit: }  // namespace der

Done.

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:19: // Enumeration that describes a
(parameter-less) hashing algorithm.
On 2015/07/07 13:55:15, Ryan Sleevi (slow through 7-15 wrote:
> nit: This comment doesn't seem to really add much. On a semantic level, it
> arguably names rather than describes, but there's arguable nuance in that
> terminology.
> 
> // The digest algorithm used within a signature.
> 
> Might be slightly more direct?

Done.

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:28: // algorithm are specified
separately).
On 2015/07/07 13:55:15, Ryan Sleevi (slow through 7-15 wrote:
> // The signature scheme used within a signature. Parameters are specified
> separately.

Done.

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:52: // The trailer is assumed to be 1,
and the mask gen to be MGF1.
On 2015/07/07 13:55:15, Ryan Sleevi (slow through 7-15 wrote:
> Why?

Why not?

I don't see any support in BoringSSL for modulating those parameters, nor did I
see where alternate mask gens were enumerated by the specs.

Are you proposing I add a more general structure for mask gen functions, and
have MGF1 be a special case of it?

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:73: SignatureAlgorithm();
On 2015/07/07 13:55:15, Ryan Sleevi (slow through 7-15 wrote:
> Why init+Assign vs a factory create? Is the assumption to avoid the heap?

Using factory methods creates the need to support assignment or copy or move.

Currently the parameters are held by a scoped_ptr<> so assignment or copy are
not trivial.

Maybe move support would be sufficient, but it seemed like more effort than it
was worth, so I just stuck with assignment to keep it simple.

[Ryan Sleevi, 2015-07-08 11:44:01]

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
File net/cert/internal/signature_algorithm.cc (right):

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:113: return
!algorithm_identifier_parser.HasMore();
On 2015/07/08 01:09:34, eroman wrote:
> For anyone following, this is the same conversation with more detail on
> https://codereview.chromium.org/1217653006/.

The result being that as a result if we adopt the 2008 syntax with RFC 5912,
then it's fine to add this check, even though it wasn't strictly required under
the 1988 syntax (as it wasn't expressible positive or negative and the prose
didn't either, although the implication is that because it's an unttaged
optional any, syntax of which was removed in 95, then any extensibility would be
indistinguishable from the ANY and thus ambiguous, thus non-extensible)

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:169: // regards - it additionally
accepts NULL parameters.
On 2015/07/08 01:09:34, eroman wrote:
> I added the NULL check originally to match Firefox, which allows both empty
and
> NULL for each flavor of RSA PKCS1 and ECDSA.

So if we're adopting the 5912 rules (as discussed elsewhere in this and
related), then rejecting NULL params is in line with the MUST of 5758 (quoted
below) and in line with the relevant sa-ECDSA info classes

   sa-ecdsaWithSHA224 SIGNATURE-ALGORITHM ::= {
    IDENTIFIER ecdsa-with-SHA224
    VALUE ECDSA-Sig-Value
    PARAMS TYPE NULL ARE absent
    HASHES { mda-sha224 }
    PUBLIC-KEYS { pk-ec }
    SMIME-CAPS { IDENTIFIED BY ecdsa-with-SHA224 }
   }

note the reference to, pk-ec

   pk-ec PUBLIC-KEY ::= {
    IDENTIFIER id-ecPublicKey
    KEY ECPoint
    PARAMS TYPE ECParameters ARE required
    -- Private key format not in this module --
    CERT-KEY-USAGE { digitalSignature, nonRepudiation, keyAgreement,
                         keyCertSign, cRLSign }
   }

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
File net/cert/internal/signature_algorithm.h (right):

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:52: // The trailer is assumed to be 1,
and the mask gen to be MGF1.
On 2015/07/08 01:09:34, eroman wrote:
> Are you proposing I add a more general structure for mask gen functions, and
> have MGF1 be a special case of it?

Sorry for being opaque, especially with the round-trip-time. I more meant
"Document why" for these assumptions. My hope is that we document the hell out
of things whenever possible, or even (briefly) summarize some of these CL
comments, because of the subtlety.

For example

// The trailer is assumed to be 1 and the mask generation algorithm to be MGF1,
as that
// is all that is implemented, and any other values while parsing the
AlgorithmIdentifier will thus
// be rejected.

So that if/when there's some other trailer or some other MGF1, it's clear the
reason we didn't add the additional parameters is, like you said, because it's
not implemented and it's all that's specified, so there's no need to explicitly
track/expose via the API.

[Ryan Sleevi, 2015-07-08 11:49:05]

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
File net/cert/internal/signature_algorithm.h (right):

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:73: SignatureAlgorithm();
On 2015/07/08 01:09:34, eroman wrote:
> On 2015/07/07 13:55:15, Ryan Sleevi (slow through 7-15 wrote:
> > Why init+Assign vs a factory create? Is the assumption to avoid the heap?
> 
> Using factory methods creates the need to support assignment or copy or move.
> 
> Currently the parameters are held by a scoped_ptr<> so assignment or copy are
> not trivial.
> 
> Maybe move support would be sufficient, but it seemed like more effort than it
> was worth, so I just stuck with assignment to keep it simple.

Sorry, by factory method I meant something like

scoped_ptr<SignatureAlgorithm> CreateRsaPkcs(...);
scoped_ptr<SignatureAlgorithm> CreateEcdsa(...);
scoped_ptr<SignatureAlgorithm> CreateRsaPss(...);
scoped_ptr<SignatureAlgorithm> ParseDer(...);

with nullptr == unable to create. This makes it more explicit than 

SignatureAlgorithm foo;
// foo.ParseDer(...) || foo.AssignRsaPkcs1(...)
if (!foo.IsValid())

The WUR for ParseDer is understandably trying to make sure an uninitialized Foo
isn't used, but it does seem more overhead w/ the need to check IsValid(). It
also makes it weird for situations like ParamsForRsaPss() returning a structure
that could be invalidated by a subsequent Assign* call.

[The assumption here is that SignatureAlgorithm is moved to a protected/private
ctor as appropriate, and only the dtor is exposed]

By the factory scoped_ptr<> method, you can make the SignatureAlgorithm wholly
immutable and singly-owned, but at the cost of going to the heap. I had assumed
you were making a performance tradeoff there in avoiding the heap, but it sounds
like you weren't?

[eroman, 2015-07-14 20:31:39]

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
File net/cert/internal/signature_algorithm.cc (right):

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:113: return
!algorithm_identifier_parser.HasMore();
On 2015/07/08 11:44:01, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/07/08 01:09:34, eroman wrote:
> > For anyone following, this is the same conversation with more detail on
> > https://codereview.chromium.org/1217653006/.
> 
> The result being that as a result if we adopt the 2008 syntax with RFC 5912,
> then it's fine to add this check, even though it wasn't strictly required
under
> the 1988 syntax (as it wasn't expressible positive or negative and the prose
> didn't either, although the implication is that because it's an unttaged
> optional any, syntax of which was removed in 95, then any extensibility would
be
> indistinguishable from the ANY and thus ambiguous, thus non-extensible)

I have added comments to each place in the code that calls HasMore() referencing
why extra input is disallowed. Thanks for pointing me towards RFC 5912, it
helped clear some stuff up.

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:169: // regards - it additionally
accepts NULL parameters.
On 2015/07/08 11:44:01, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/07/08 01:09:34, eroman wrote:
> > I added the NULL check originally to match Firefox, which allows both empty
> and
> > NULL for each flavor of RSA PKCS1 and ECDSA.
> 
> So if we're adopting the 5912 rules (as discussed elsewhere in this and
> related), then rejecting NULL params is in line with the MUST of 5758 (quoted
> below) and in line with the relevant sa-ECDSA info classes
> 
>    sa-ecdsaWithSHA224 SIGNATURE-ALGORITHM ::= {
>     IDENTIFIER ecdsa-with-SHA224
>     VALUE ECDSA-Sig-Value
>     PARAMS TYPE NULL ARE absent
>     HASHES { mda-sha224 }
>     PUBLIC-KEYS { pk-ec }
>     SMIME-CAPS { IDENTIFIED BY ecdsa-with-SHA224 }
>    }
> 
> note the reference to, pk-ec
> 
>    pk-ec PUBLIC-KEY ::= {
>     IDENTIFIER id-ecPublicKey
>     KEY ECPoint
>     PARAMS TYPE ECParameters ARE required
>     -- Private key format not in this module --
>     CERT-KEY-USAGE { digitalSignature, nonRepudiation, keyAgreement,
>                          keyCertSign, cRLSign }
>    }
> 

I have re-written most of the RFC references in terms of RFC 5912, as it
provides clearer definitions.

I have changed the parameters parsing for RSA and ECDSA to strictly match RFC
5912, which is to say:
  * RSA must provide a NULL parameters in all cases
  * ECDSA must omit it in all cases

I have updated the tests, and made sure that each of these variations is being
tested.

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
File net/cert/internal/signature_algorithm.h (right):

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:52: // The trailer is assumed to be 1,
and the mask gen to be MGF1.
On 2015/07/08 11:44:01, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/07/08 01:09:34, eroman wrote:
> > Are you proposing I add a more general structure for mask gen functions, and
> > have MGF1 be a special case of it?
> 
> Sorry for being opaque, especially with the round-trip-time. I more meant
> "Document why" for these assumptions. My hope is that we document the hell out
> of things whenever possible, or even (briefly) summarize some of these CL
> comments, because of the subtlety.
> 
> For example
> 
> // The trailer is assumed to be 1 and the mask generation algorithm to be
MGF1,
> as that
> // is all that is implemented, and any other values while parsing the
> AlgorithmIdentifier will thus
> // be rejected.
> 
> So that if/when there's some other trailer or some other MGF1, it's clear the
> reason we didn't add the additional parameters is, like you said, because it's
> not implemented and it's all that's specified, so there's no need to
explicitly
> track/expose via the API.

Done.

https://codereview.chromium.org/1218753002/diff/200002/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:73: SignatureAlgorithm();
On 2015/07/08 11:49:05, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/07/08 01:09:34, eroman wrote:
> > On 2015/07/07 13:55:15, Ryan Sleevi (slow through 7-15 wrote:
> > > Why init+Assign vs a factory create? Is the assumption to avoid the heap?
> > 
> > Using factory methods creates the need to support assignment or copy or
move.
> > 
> > Currently the parameters are held by a scoped_ptr<> so assignment or copy
are
> > not trivial.
> > 
> > Maybe move support would be sufficient, but it seemed like more effort than
it
> > was worth, so I just stuck with assignment to keep it simple.
> 
> Sorry, by factory method I meant something like
> 
> scoped_ptr<SignatureAlgorithm> CreateRsaPkcs(...);
> scoped_ptr<SignatureAlgorithm> CreateEcdsa(...);
> scoped_ptr<SignatureAlgorithm> CreateRsaPss(...);
> scoped_ptr<SignatureAlgorithm> ParseDer(...);
> 
> with nullptr == unable to create. This makes it more explicit than 
> 
> SignatureAlgorithm foo;
> // foo.ParseDer(...) || foo.AssignRsaPkcs1(...)
> if (!foo.IsValid())
> 
> The WUR for ParseDer is understandably trying to make sure an uninitialized
Foo
> isn't used, but it does seem more overhead w/ the need to check IsValid(). It
> also makes it weird for situations like ParamsForRsaPss() returning a
structure
> that could be invalidated by a subsequent Assign* call.
> 
> [The assumption here is that SignatureAlgorithm is moved to a
protected/private
> ctor as appropriate, and only the dtor is exposed]
> 
> By the factory scoped_ptr<> method, you can make the SignatureAlgorithm wholly
> immutable and singly-owned, but at the cost of going to the heap. I had
assumed
> you were making a performance tradeoff there in avoiding the heap, but it
sounds
> like you weren't?

Done.
(Except renamed ParseDer --> CreateFromDer() for consistency)

[Ryan Sleevi, 2015-07-16 03:46:37]

LGTM % nits & request for analysis

https://codereview.chromium.org/1218753002/diff/450001/net/cert/internal/sign...
File net/cert/internal/signature_algorithm.cc (right):

https://codereview.chromium.org/1218753002/diff/450001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:191: if (!IsNull(params))
I'm still nervous this is going to cause issues. We'd end up being the strictest
here.

I'm sorry if my mention of 5912 gave you the impression it was correct - IETF
produces documents all the time that "don't work" in the real world :) "yay".

A scan of the CT database would show publicly trusted certs, but then we still
have to worry about internal certs.

https://codereview.chromium.org/1218753002/diff/450001/net/cert/internal/sign...
File net/cert/internal/signature_algorithm.h (right):

https://codereview.chromium.org/1218753002/diff/450001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:45: // cast).
Still feels like we're stuck between a footgun ("you must do this" -> no one
will do this) and an RTTI implementation (exposing SignatureAlgorithmId on the
base class)

Is it even necessary to have the virtual Equals method, since anyone who knows
the parameters are identical can cast to the derived class and do the comparison
there (e.g. having RsaPssParameters have a non-virtual Equals)

[eroman, 2015-07-16 05:23:06]

https://codereview.chromium.org/1218753002/diff/450001/net/cert/internal/sign...
File net/cert/internal/signature_algorithm.cc (right):

https://codereview.chromium.org/1218753002/diff/450001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:191: if (!IsNull(params))
On 2015/07/16 03:46:37, Ryan Sleevi (slow through 7-15 wrote:
> I'm still nervous this is going to cause issues. We'd end up being the
strictest
> here.
> 
> I'm sorry if my mention of 5912 gave you the impression it was correct - IETF
> produces documents all the time that "don't work" in the real world :) "yay".
> 
> A scan of the CT database would show publicly trusted certs, but then we still
> have to worry about internal certs.

Understood.

When I get back I will see what it takes to run through the CT database certs
and see if I can find any incompatibilities with this approach.

https://codereview.chromium.org/1218753002/diff/450001/net/cert/internal/sign...
File net/cert/internal/signature_algorithm.h (right):

https://codereview.chromium.org/1218753002/diff/450001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.h:45: // cast).
On 2015/07/16 03:46:37, Ryan Sleevi (slow through 7-15 wrote:
> Still feels like we're stuck between a footgun ("you must do this" -> no one
> will do this) and an RTTI implementation (exposing SignatureAlgorithmId on the
> base class)
> 
> Is it even necessary to have the virtual Equals method, since anyone who knows
> the parameters are identical can cast to the derived class and do the
comparison
> there (e.g. having RsaPssParameters have a non-virtual Equals)

I removed the virtual in favor of a different approach.

[eroman, 2015-07-16 17:34:44]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-07-16 17:34:44]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1218753002/#ps470001
(title: "de-virtualize Equals()")

[commit-bot: I haz the power, 2015-07-16 17:35:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1218753002/470001

[commit-bot: I haz the power, 2015-07-16 18:48:12]

Committed patchset #25 (id:470001)

[commit-bot: I haz the power, 2015-07-16 18:50:30]

Patchset 25 (id:??) landed as
https://crrev.com/68de6fee236e99393cfd462b0698cebb50c55d9a
Cr-Commit-Position: refs/heads/master@{#339088}