Add DER parsing for rsaPss signature algorithms.

Add DER parsing for rsaPss signature algorithms.

BUG=410574
Committed: https://crrev.com/528dcd1e4320c6b2bb80154a07def2ab53340278
Cr-Commit-Position: refs/heads/master@{#339400}

[eroman, 2015-07-01 01:11:31]

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

[eroman, 2015-07-01 01:11:31]

Some more unit-tests are still needed for different combinations of missing/null
parameters and invalid inputs, but this should be ready for  initial review.

[eroman, 2015-07-01 17:24:00]

added more unittests

[Ryan Sleevi, 2015-07-06 15:03:18]

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

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:278: // Parses a MaskGenAlgorithm as
defined by RFC 4055. It is expected that the
Worth mentioning (somewhere) that MaskGenAlgorithm ::= AlgorithmIdentifier

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:279: // mask gen is for MGF1, as this
is the only function allowed for in RFC 4055.
Comment wise, this reads weird. RFC 4055 totally allows this to be extensible /
updated by future RFCs, so it 'allows' for more, and defines how to do that
allowance.

Perhaps
// It is expected that the algorithm is MGF1, as this is the only function
defined in RFC 4055.

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:313: // (context-specific) tag number.
// Consumes an optional, explicitly-tagged INTEGER from |parser|, using the
indicated
// context-specific class number. Values greater than 32-bits will be rejected.

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:317: uint8_t tag_base,
naming nit: tag_base originally suggested to me some base-2/base-10/base-16
thing, until I realized it's meant to be the _class number_ (in ASN.1 terms)

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:387: uint32_t salt_length = 20;
pedantry: 20u

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:388: uint32_t trailer_field = 1;
pedantry: 1u

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:394: if
(!params_parser.ReadOptionalTag(der::ContextSpecificConstructed(0), &field,
When Nick and I originally had discussed this, there was a question about
whether or not explicit class-tagging would be needed. Turns out, it is.

I'm wondering if this is the easiest API for it in understanding what was going
on. Thoughts?

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:399: if (!ParseHashAlgorithm(field,
&hash))
simplify

if (has_field && !ParseHashAlgorithm(field, &hash))
  return false;

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:410: return false;
Simplify

if (has_field && !ParseMaskGenAlgorithm(field, &mgf1_hash))
  return false;

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:430: if (params_parser.HasMore())
BUG: This is unnecessarily strict, since it's totally valid for this field to be
extended in the future.

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
File net/cert/internal/signature_algorithm_unittest.cc (right):

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm_unittest.cc:642: //           NULL
This should work, and is valid.

[eroman, 2015-07-07 01:24:23]

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

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:278: // Parses a MaskGenAlgorithm as
defined by RFC 4055. It is expected that the
On 2015/07/06 15:03:18, Ryan Sleevi (slow through 7-15 wrote:
> Worth mentioning (somewhere) that MaskGenAlgorithm ::= AlgorithmIdentifier

Done.

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:279: // mask gen is for MGF1, as this
is the only function allowed for in RFC 4055.
On 2015/07/06 15:03:17, Ryan Sleevi (slow through 7-15 wrote:
> Comment wise, this reads weird. RFC 4055 totally allows this to be extensible
/
> updated by future RFCs, so it 'allows' for more, and defines how to do that
> allowance.
> 
> Perhaps
> // It is expected that the algorithm is MGF1, as this is the only function
> defined in RFC 4055.

How about this:

// Parses a MaskGenAlgorithm as defined by RFC 4055:
//
//     MaskGenAlgorithm  ::=  AlgorithmIdentifier
//
// This function currently only supports MGF1, as that is the only function
// defined in RFC 4055.

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:313: // (context-specific) tag number.
On 2015/07/06 15:03:18, Ryan Sleevi (slow through 7-15 wrote:
> // Consumes an optional, explicitly-tagged INTEGER from |parser|, using the
> indicated
> // context-specific class number. Values greater than 32-bits will be
rejected.

Done.

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:317: uint8_t tag_base,
On 2015/07/06 15:03:17, Ryan Sleevi (slow through 7-15 wrote:
> naming nit: tag_base originally suggested to me some base-2/base-10/base-16
> thing, until I realized it's meant to be the _class number_ (in ASN.1 terms)

Done.

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:387: uint32_t salt_length = 20;
On 2015/07/06 15:03:17, Ryan Sleevi (slow through 7-15 wrote:
> pedantry: 20u

Done.

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:388: uint32_t trailer_field = 1;
On 2015/07/06 15:03:17, Ryan Sleevi (slow through 7-15 wrote:
> pedantry: 1u

Done.

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:394: if
(!params_parser.ReadOptionalTag(der::ContextSpecificConstructed(0), &field,
On 2015/07/06 15:03:17, Ryan Sleevi (slow through 7-15 wrote:
> When Nick and I originally had discussed this, there was a question about
> whether or not explicit class-tagging would be needed. Turns out, it is.
> 
> I'm wondering if this is the easiest API for it in understanding what was
going
> on. Thoughts?

My API suggestion depends on understanding a few things better.

 * tag.h contains method ContextSpecificPrimitive(). I haven't these in the
specs I have read so far, are you aware of when it would be used? Is this for
IMPLICIT tagging of a context-specific primitive?

  * Would we want to accept unrecognized stuff inside the context specific
contructed tag, or require it to contain at most 1 TLV.

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:399: if (!ParseHashAlgorithm(field,
&hash))
On 2015/07/06 15:03:18, Ryan Sleevi (slow through 7-15 wrote:
> simplify
> 
> if (has_field && !ParseHashAlgorithm(field, &hash))
>   return false;

Done.

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:410: return false;
On 2015/07/06 15:03:17, Ryan Sleevi (slow through 7-15 wrote:
> Simplify
> 
> if (has_field && !ParseMaskGenAlgorithm(field, &mgf1_hash))
>   return false;

Done.

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:430: if (params_parser.HasMore())
On 2015/07/06 15:03:17, Ryan Sleevi (slow through 7-15 wrote:
> BUG: This is unnecessarily strict, since it's totally valid for this field to
be
> extended in the future.

Thanks.

I have some concerns with leaving unconsumed data that I would like understand
better before proceeding with that change.

Are the rules for unconsumed data enumerated somewhere I can review?
The closest I could find in RFC 5280 was the "critical" boolean specific to
extensions and not applicable here. And I didn't see anything obvious in RFC
4055. And I am searching through X.690 for answers.

(1) If new parameters are added which actually change the signature algorithm,
wouldn't it be more prudent to simply reject? In the best case scenario, the
subsequent signature verification will fail because we didn't use the right
algorithm parameters. In the worst case, I am concerned whether this opens an
opportunity to exploit a problem that the new (specified) parameter was meant to
solve, but Chrome discard.

The only type of new parameter I could imagine (one which doesn't actually
change the verification result) would be something to tweak the verification
performance.

If signature verification is doomed to fail anyway, I would expect it is
preferable to fail fast, and give an error message that calls out parameter
parsing. Rather than do the signature verification, and then fail it. How have
algorithm parameters historically been extended? Seems like in the model where
parameters are disregarded it would be more prudent to add new OIDs instead.

(2) Would we want to accept malformed DER when skipping unrecognized data?

The simplest implementation would be to simply remove the HasMore() check above.
A possible downside to this approach is we wouldn't do any validation of the
subsequence data. Maybe this is the goal. However it does mean the subsequent
data could be malformed DER (for instance bad tag lengths), or arbitrary bytes
for that matter.

(3) Should I change anything with regard to the interpretation of EXPLICIT tags?
saltLength, trailerField, and the like are also containers which could contain
more than one tag. I currently check for HasMore(), however if we are allowing
it at the end of the parameters sequence, should we similarly allow it within
those constructed tags?

I will see if I can find anything in the ASN.1 / DER spec to clarify things

[Ryan Sleevi, 2015-07-07 15:27:23]

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

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:430: if (params_parser.HasMore())
On 2015/07/07 01:24:23, eroman wrote:
> On 2015/07/06 15:03:17, Ryan Sleevi (slow through 7-15 wrote:
> > BUG: This is unnecessarily strict, since it's totally valid for this field
to
> be
> > extended in the future.
> 
> Thanks.
> 
> I have some concerns with leaving unconsumed data that I would like understand
> better before proceeding with that change.
> 
> Are the rules for unconsumed data enumerated somewhere I can review?
> The closest I could find in RFC 5280 was the "critical" boolean specific to
> extensions and not applicable here. And I didn't see anything obvious in RFC
> 4055. And I am searching through X.690 for answers.

This is all covered by the ASN.1 schema, which is that the schema is necessarily
extensible unless explicitly non-extensible.

For example, SEQUENCE OF (x...y) SOMETHING must always be a SOMETHING and never
have additional data. However, a 
SEQUENCE {
  foo foo;
  bar bar;
};

Can change over time. And if you run into situations where there are
optionals/ambiguity, you can introduce tagging (explicit or implicit)

Conceptually, if it helps, think of this a similar to protobuf schemas, all of
which are by definition extensible. Every field is implicitly required (think
https://developers.google.com/protocol-buffers/docs/proto#specifying-field-types
), unless explicitly optional, but they're all extensible.

Checking HasMore() and rejecting is a bit like prohibiting message types from
ever being extended (
https://developers.google.com/protocol-buffers/docs/proto#updating )

> If signature verification is doomed to fail anyway, I would expect it is
> preferable to fail fast, and give an error message that calls out parameter
> parsing. Rather than do the signature verification, and then fail it. How have
> algorithm parameters historically been extended? Seems like in the model where
> parameters are disregarded it would be more prudent to add new OIDs instead.

I understand these concerns, but this is valid DER. While unlikely to be done
(for just that reason), our parsing of DER should not be inconsistently
incorrect, which is what checking that there are no additional bytes would be.

> (2) Would we want to accept malformed DER when skipping unrecognized data?
> 
> The simplest implementation would be to simply remove the HasMore() check
above.
> A possible downside to this approach is we wouldn't do any validation of the
> subsequence data. Maybe this is the goal. However it does mean the subsequent
> data could be malformed DER (for instance bad tag lengths), or arbitrary bytes
> for that matter.

Yes, that's correct. ASN.1's T/L/Vs generally provide sufficient context to know
whether or not you can continue to drill down into a field (c.f. constructed
types), although that's still not a perfect solution (somewhat might smuggle
data in an OCTET STRING or BITSTRING, which wouldn't necessarily be drillable)

In ASN.1 schema terms, we're talking about the '...' and EXTENSIBILITY IMPLIED.
You can use RFC 5912 for the ASN.1 2008 definition of tbsCertificate, whose
schema definition (truncated) includes

     [[3:               -- If present, version MUST be v3 --
      extensions      [3]  Extensions{{CertExtensions}} OPTIONAL
      ]], ... }

It's the last ... that explicitly states more fields may be added in the future.
You can also see the discussion on
https://tools.ietf.org/html/rfc6025#section-2.4

That said, having looked in 5912, AlgorithmIdentifier is in a module that
doesn't inherit EXTENSIBILITY IMPLIED and doesn't include the '...', so your
HasMore is likely fine here.

> (3) Should I change anything with regard to the interpretation of EXPLICIT
tags?
> saltLength, trailerField, and the like are also containers which could contain
> more than one tag. I currently check for HasMore(), however if we are allowing
> it at the end of the parameters sequence, should we similarly allow it within
> those constructed tags?

No, an explicitly tagged class includes the original tagged type.

> 
> I will see if I can find anything in the ASN.1 / DER spec to clarify things

You're wanting 31.2 of X.680 for implicit tags, 50.3 of X.680 for type
definitions of collections (ish), I.4 for constraint math on 50.3, and perhaps
more directly related to your general concerns, Section 7.

[eroman, 2015-07-08 00:22:39]

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

https://codereview.chromium.org/1217653006/diff/240001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:430: if (params_parser.HasMore())
On 2015/07/07 15:27:23, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/07/07 01:24:23, eroman wrote:
> > On 2015/07/06 15:03:17, Ryan Sleevi (slow through 7-15 wrote:
> > > BUG: This is unnecessarily strict, since it's totally valid for this field
> to
> > be
> > > extended in the future.
> > 
> > Thanks.
> > 
> > I have some concerns with leaving unconsumed data that I would like
understand
> > better before proceeding with that change.
> > 
> > Are the rules for unconsumed data enumerated somewhere I can review?
> > The closest I could find in RFC 5280 was the "critical" boolean specific to
> > extensions and not applicable here. And I didn't see anything obvious in RFC
> > 4055. And I am searching through X.690 for answers.
> 
> This is all covered by the ASN.1 schema, which is that the schema is
necessarily
> extensible unless explicitly non-extensible.
> 
> For example, SEQUENCE OF (x...y) SOMETHING must always be a SOMETHING and
never
> have additional data. However, a 
> SEQUENCE {
>   foo foo;
>   bar bar;
> };
> 
> Can change over time. And if you run into situations where there are
> optionals/ambiguity, you can introduce tagging (explicit or implicit)
> 
> Conceptually, if it helps, think of this a similar to protobuf schemas, all of
> which are by definition extensible. Every field is implicitly required (think
>
https://developers.google.com/protocol-buffers/docs/proto#specifying-field-types
> ), unless explicitly optional, but they're all extensible.
> 
> Checking HasMore() and rejecting is a bit like prohibiting message types from
> ever being extended (
> https://developers.google.com/protocol-buffers/docs/proto#updating )
> 
> > If signature verification is doomed to fail anyway, I would expect it is
> > preferable to fail fast, and give an error message that calls out parameter
> > parsing. Rather than do the signature verification, and then fail it. How
have
> > algorithm parameters historically been extended? Seems like in the model
where
> > parameters are disregarded it would be more prudent to add new OIDs instead.
> 
> I understand these concerns, but this is valid DER. While unlikely to be done
> (for just that reason), our parsing of DER should not be inconsistently
> incorrect, which is what checking that there are no additional bytes would be.
> 
> > (2) Would we want to accept malformed DER when skipping unrecognized data?
> > 
> > The simplest implementation would be to simply remove the HasMore() check
> above.
> > A possible downside to this approach is we wouldn't do any validation of the
> > subsequence data. Maybe this is the goal. However it does mean the
subsequent
> > data could be malformed DER (for instance bad tag lengths), or arbitrary
bytes
> > for that matter.
> 
> Yes, that's correct. ASN.1's T/L/Vs generally provide sufficient context to
know
> whether or not you can continue to drill down into a field (c.f. constructed
> types), although that's still not a perfect solution (somewhat might smuggle
> data in an OCTET STRING or BITSTRING, which wouldn't necessarily be drillable)
> 
> In ASN.1 schema terms, we're talking about the '...' and EXTENSIBILITY
IMPLIED.
> You can use RFC 5912 for the ASN.1 2008 definition of tbsCertificate, whose
> schema definition (truncated) includes
> 
>      [[3:               -- If present, version MUST be v3 --
>       extensions      [3]  Extensions{{CertExtensions}} OPTIONAL
>       ]], ... }
> 
> It's the last ... that explicitly states more fields may be added in the
future.
> You can also see the discussion on
> https://tools.ietf.org/html/rfc6025#section-2.4
> 
> That said, having looked in 5912, AlgorithmIdentifier is in a module that
> doesn't inherit EXTENSIBILITY IMPLIED and doesn't include the '...', so your
> HasMore is likely fine here.
> 
> > (3) Should I change anything with regard to the interpretation of EXPLICIT
> tags?
> > saltLength, trailerField, and the like are also containers which could
contain
> > more than one tag. I currently check for HasMore(), however if we are
allowing
> > it at the end of the parameters sequence, should we similarly allow it
within
> > those constructed tags?
> 
> No, an explicitly tagged class includes the original tagged type.
> 
> > 
> > I will see if I can find anything in the ASN.1 / DER spec to clarify things
> 
> You're wanting 31.2 of X.680 for implicit tags, 50.3 of X.680 for type
> definitions of collections (ish), I.4 for constraint math on 50.3, and perhaps
> more directly related to your general concerns, Section 7.

Thanks for the information, I will try to absorb that.

I am not sure I understand your concluding remark of "so your HasMore is likely
fine here."
Which "HasMore" is that in relation to?

If we don't consume all the data I should also remove
SignatureAlgorithm::Equals(), since it might be dangerous/wrong to conclude that
two algorithm identifiers with different unconsumed data are the same. As far as
just doing a byte-for-byte equality test, I read that there are existing certs
with mismatched parameters encoding (NULL vs missing) for the signature
algorithm, so a pure equality test would fail those.

[Ryan Sleevi, 2015-07-08 11:30:39]

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

https://codereview.chromium.org/1217653006/diff/260001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:400: if (parser.HasMore())
This is fine

https://codereview.chromium.org/1217653006/diff/260001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:445: if (params_parser.HasMore())
The related discussion (re; RFC 5912, RFC 6025, re: X.680) concluded that this
is fine for this specific case, in that it's not explicitly defined as
extensible under the 2008 syntax (even though it is totally valid for a future
specification update to extend this, per X.680 and the 1998 syntax that it was
originally written under)

[eroman, 2015-07-14 20:50:27]

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

https://codereview.chromium.org/1217653006/diff/260001/net/cert/internal/sign...
net/cert/internal/signature_algorithm.cc:445: if (params_parser.HasMore())
On 2015/07/08 11:30:39, Ryan Sleevi (slow through 7-15 wrote:
> The related discussion (re; RFC 5912, RFC 6025, re: X.680) concluded that this
> is fine for this specific case, in that it's not explicitly defined as
> extensible under the 2008 syntax (even though it is totally valid for a future
> specification update to extend this, per X.680 and the 1998 syntax that it was
> originally written under)

I have updated comments to reference RFC 5912, and provide justification for the
various HasMore() checks.

[Ryan Sleevi, 2015-07-16 03:52:31]

lgtm

[eroman, 2015-07-17 01:06:51]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-07-17 01:06:51]

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/1217653006/#ps350001
(title: "rebase and use results of "git cl format" from new clang-format")

[commit-bot: I haz the power, 2015-07-17 01:07:24]

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

[commit-bot: I haz the power, 2015-07-17 03:07:55]

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

[commit-bot: I haz the power, 2015-07-17 03:07:56]

Try jobs failed on following builders:
  linux_arm_compile on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm_compi...)

[eroman, 2015-07-18 21:39:22]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2015-07-18 21:39:27]

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

[commit-bot: I haz the power, 2015-07-18 22:38:07]

Committed patchset #19 (id:350001)

[commit-bot: I haz the power, 2015-07-18 22:39:06]

Patchset 19 (id:??) landed as
https://crrev.com/528dcd1e4320c6b2bb80154a07def2ab53340278
Cr-Commit-Position: refs/heads/master@{#339400}