Implement VerifySignedData() for ECDSA, RSA PKCS#1 and RSA PSS.

Implement VerifySignedData() for ECDSA, RSA PKCS#1 and RSA PSS.

The implementation is specifically for BoringSSL.

BUG=410574
Committed: https://crrev.com/2b124dbcd72c180b2865dc0448e280d2e3e7cbe4
Cr-Commit-Position: refs/heads/master@{#340633}

[eroman, 2015-07-02 17:49:06]

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

[eroman, 2015-07-02 17:49:07]

I am still in the process of adding more test data (which is tedious), but the
CL itself should be ready for review.

[Ryan Sleevi, 2015-07-07 14:07:31]

The std::string changes make me deeply uncomfortable - it's a big footgun (as
shown when our usages of it weren't safe)

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:103: // the needed checks?
David: Look here

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:137: // the needed checks?
David: And here

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.h (right):

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.h:15: }
nit: }  // namespace der

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.h:29: //   |public_key| - A DER encoded
SubjectPublicKeyInfo.
nit: DER-encoded

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data_unittest.cc (right):

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:26: //                 
algorithm (signatureAlgorithm in x509)
s/x509/X.509/

(and throughout)

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:45: PEMTokenizer
pem_tok(file_data, pem_headers);
nit: s/pem_tok/pem_tokenizer/ - the tok abbreviation is non-obvious

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:46: for (size_t cur_index = 0;
pem_tok.GetNext(); cur_index++) {
|cur_index| is unused. Why not

while (pem_tok.GetNext()) {

}

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:91: if
(!base::ReadFileToString(test_file_path, &file_data)) {
Why not ASSERT_TRUE(base::ReadFileToString(...)) << "Couldn't read file: " <<
test_file_path.value() ?

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:119: RunTestCase(SUCCESS,
"rsa-pkcs1-sha1.pem");
In looking at the enum & such, why not do

EXPECT_TRUE(RunTestCase(...));
EXPECT_FALSE(RunTestCase(...));

?

Is it so you can use the Assert macros?

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

https://codereview.chromium.org/1209283004/diff/120001/net/der/input.h#newcode53
net/der/input.h:53: explicit Input(const std::string& s);
Intentionally removed this API. I'm not keen to reintroduce it.

[eroman, 2015-07-07 17:25:25]

+davidben, since I think Ryan forgot to add after leaving questions for him :)

[davidben, 2015-07-07 18:03:56]

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

[davidben, 2015-07-07 18:03:57]

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:31: #include <openssl/pkcs12.h>
What's this include for?

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:52: pkey->reset(d2i_PUBKEY(nullptr,
&ptr, spki.Length()));
This needs x509.h. I'd rather we not use x509.h, but it's true that pub_encode /
pub_decode hooks on the EVP_PKEY aren't currently exposed anywhere. Not sure yet
what I want to do with those.

We could use this for now, and I could get you folks a fresh API that doesn't
use the legacy ASN.1 stack. But the question there is, like with
AlgorithmIdentifier in signatures, do we want that within BoringSSL or do we
want that in net/cert? net/cert is in a better position to enforce things like:

- RFC 4055 defines an id-RSASSA-PSS for the SPKI to make the key only allow
RSA-PSS? I'm not sure BoringSSL even accepts it at all right now.
https://tools.ietf.org/html/rfc4055#section-3.3

- RFC 3279 says RSA keys are identified by rsaEncryption. d2i_PUBKEY will accept
either rsaEncryption or 2.5.8.1.1 apparently. I dunno whether compatibility
requires we accept both, but it's certainly easier to change a new thing than an
old; the way it's plumbed into BoringSSL is pretty invasive.

- SPKIs for EC keys may use id-ecPublicKey or id-ecDH. The latter is useless for
us, but we shouldn't allow those for ECDSA. BoringSSL currently doesn't parse
id-ecDH at all (and I doubt it ever will), but that's not necessarily in the
contract of a "SPKI -> EVP_PKEY" function. At least right now, an EVP_PKEY
doesn't include any constraints on it. It's a pretty thin abstraction over EC
and RSA keys. (And DSA, but that only half-works.)

- Speaking of DSA, d2i_PUBKEY will happily decode DSA keys right now. They'll
decode, but operations on them won't work. (It's the bare minimum needed thus
far to appease some internal consumers. Hopefully it'll go away again later, but
it does need to be routed up for now.)

Then again, processing SPKIs may be a generally useful thing. Certainly the SSL
stack needs to extract the key even in a post crypto/x509 world. I don't have
strong opinions as to whether that should work by parsing a tiny X.509 subset in
ssl/ or if the "here is a bunch of certificates I got over the wire; do they
look good?" callback should also have the embedder give back the public key. The
latter seems kinda tempting actually. Nice to not have things being parsed
twice.


I currently lean towards having net/cert parse those and instead call
lower-level functions like d2i_RSAPublicKey (soon to no longer use the legacy
ASN.1 stack[*], so I'll have a nicer API for you later). But I'm also not the
person who will be implementing all that.


[*] Note: right now, this does use the legacy ASN.1 stack which really parses
BER, not DER. It's absurdly lenient; we didn't even notice this:
https://boringssl.googlesource.com/boringssl/+/cb41d7702980776e61f46c535cfdef...

However, fairly soon those will be using crypto/bytestring. Then they will be,
as far as I know, completely strict w.r.t. DER. So that's something we'll need
to sniff at for compat once that's in.

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:76: return *out;
I believe MSVC will grump about this and want !!*out or *out != nullptr.

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:103: // the needed checks?
On 2015/07/07 14:07:31, Ryan Sleevi (slow through 7-15 wrote:
> David: Look here

I don't believe BoringSSL is even passed the signature algorithm here, so it
couldn't do it if we wanted to. But, no, an EVP_PKEY has no constraints attached
to it. It's just a thin wrapper over RSA and EC_KEY, nothing more.

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:137: // the needed checks?
On 2015/07/07 14:07:31, Ryan Sleevi (slow through 7-15 wrote:
> David: And here

Ditto.

[eroman, 2015-07-07 18:07:01]

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.h (right):

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.h:15: }
On 2015/07/07 14:07:31, Ryan Sleevi (slow through 7-15 wrote:
> nit: }  // namespace der

Done.

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.h:29: //   |public_key| - A DER encoded
SubjectPublicKeyInfo.
On 2015/07/07 14:07:31, Ryan Sleevi (slow through 7-15 wrote:
> nit: DER-encoded

Done.

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data_unittest.cc (right):

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:26: //                 
algorithm (signatureAlgorithm in x509)
On 2015/07/07 14:07:31, Ryan Sleevi (slow through 7-15 wrote:
> s/x509/X.509/
> 
> (and throughout)

Done.

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:45: PEMTokenizer
pem_tok(file_data, pem_headers);
On 2015/07/07 14:07:31, Ryan Sleevi (slow through 7-15 wrote:
> nit: s/pem_tok/pem_tokenizer/ - the tok abbreviation is non-obvious

Done.

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:46: for (size_t cur_index = 0;
pem_tok.GetNext(); cur_index++) {
On 2015/07/07 14:07:31, Ryan Sleevi (slow through 7-15 wrote:
> |cur_index| is unused. Why not
> 
> while (pem_tok.GetNext()) {
> 
> }

Fixed (was a left-over from older version when I cared about the index).

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:91: if
(!base::ReadFileToString(test_file_path, &file_data)) {
On 2015/07/07 14:07:31, Ryan Sleevi (slow through 7-15 wrote:
> Why not ASSERT_TRUE(base::ReadFileToString(...)) << "Couldn't read file: " <<
> test_file_path.value() ?

Done.

(Left-over from when RunTestCase used to have a return value)

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:119: RunTestCase(SUCCESS,
"rsa-pkcs1-sha1.pem");
On 2015/07/07 14:07:31, Ryan Sleevi (slow through 7-15 wrote:
> In looking at the enum & such, why not do
> 
> EXPECT_TRUE(RunTestCase(...));
> EXPECT_FALSE(RunTestCase(...));
> 
> ?
> 
> Is it so you can use the Assert macros?

I experimented with this earlier, but found it had some downsides:

 (1) It complicates how tests are disabled. Currently this unit-test is compiled
in all configurations, however the tests only pass on builds using BoringSSL.
Right now this is accomplished by putting an early return in RunTestCase(). This
approach no longer works when the return value of RunTestCase() is changed to a
bool that is asserted by the test.

(2) It slightly weakens the error output on failures. The line number shown is
now the EXPECT_TRUE(RunTestCase()) one rather than the more explicit line. This
can be solved by returning an assertion result.

(3) Makes code more verbose by not being able to use ASSERTions.

(4) More bug prone. For instance, if when failing to read the test file
RunTestCase() were to simply return false, a test for failed signature matching
might ASSERT_FALSE() and end up passing. The reason for the failure however is
not the right one (failed running the test, as opposed to verification failed).
This is easily solved by adding an ADD_FAILURE(), but it does suggest that a
boolean return value can be ambiguous.

In the near future I expect we will switch to using error codes rather than
booleans (or at least some interface for outputting the error details). At which
point the tests will need to be adjusted to check the actual error information,
and we can re-visit the approach.

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

https://codereview.chromium.org/1209283004/diff/120001/net/der/input.h#newcode53
net/der/input.h:53: explicit Input(const std::string& s);
On 2015/07/07 14:07:31, Ryan Sleevi (slow through 7-15 wrote:
> Intentionally removed this API. I'm not keen to reintroduce it.

Done.
Didn't realize it had been added and removed already.

I moved it to a test helper function, and also changed the input type to a
non-const reference so it cannot inadvertently be used as in the original bug
(with a temporary std::string value).

[eroman, 2015-07-07 18:34:44]

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:31: #include <openssl/pkcs12.h>
On 2015/07/07 18:03:56, David Benjamin wrote:
> What's this include for?

I was apparently relying on it for its transitive include of x509.h (needed for
d2i_PUBKEY()).

Changed to included x509.h directly instead.

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:32: #include <openssl/rand.h>
This similarly was not needed, and removed.

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:52: pkey->reset(d2i_PUBKEY(nullptr,
&ptr, spki.Length()));
On 2015/07/07 18:03:56, David Benjamin wrote:
> This needs x509.h. I'd rather we not use x509.h, but it's true that pub_encode
/
> pub_decode hooks on the EVP_PKEY aren't currently exposed anywhere. Not sure
yet
> what I want to do with those.
> 
> We could use this for now, and I could get you folks a fresh API that doesn't
> use the legacy ASN.1 stack. But the question there is, like with
> AlgorithmIdentifier in signatures, do we want that within BoringSSL or do we
> want that in net/cert? net/cert is in a better position to enforce things
like:
> 
> - RFC 4055 defines an id-RSASSA-PSS for the SPKI to make the key only allow
> RSA-PSS? I'm not sure BoringSSL even accepts it at all right now.
> https://tools.ietf.org/html/rfc4055#section-3.3
> 
> - RFC 3279 says RSA keys are identified by rsaEncryption. d2i_PUBKEY will
accept
> either rsaEncryption or 2.5.8.1.1 apparently. I dunno whether compatibility
> requires we accept both, but it's certainly easier to change a new thing than
an
> old; the way it's plumbed into BoringSSL is pretty invasive.
> 
> - SPKIs for EC keys may use id-ecPublicKey or id-ecDH. The latter is useless
for
> us, but we shouldn't allow those for ECDSA. BoringSSL currently doesn't parse
> id-ecDH at all (and I doubt it ever will), but that's not necessarily in the
> contract of a "SPKI -> EVP_PKEY" function. At least right now, an EVP_PKEY
> doesn't include any constraints on it. It's a pretty thin abstraction over EC
> and RSA keys. (And DSA, but that only half-works.)
> 
> - Speaking of DSA, d2i_PUBKEY will happily decode DSA keys right now. They'll
> decode, but operations on them won't work. (It's the bare minimum needed thus
> far to appease some internal consumers. Hopefully it'll go away again later,
but
> it does need to be routed up for now.)
> 
> Then again, processing SPKIs may be a generally useful thing. Certainly the
SSL
> stack needs to extract the key even in a post crypto/x509 world. I don't have
> strong opinions as to whether that should work by parsing a tiny X.509 subset
in
> ssl/ or if the "here is a bunch of certificates I got over the wire; do they
> look good?" callback should also have the embedder give back the public key.
The
> latter seems kinda tempting actually. Nice to not have things being parsed
> twice.
> 
> 
> I currently lean towards having net/cert parse those and instead call
> lower-level functions like d2i_RSAPublicKey (soon to no longer use the legacy
> ASN.1 stack[*], so I'll have a nicer API for you later). But I'm also not the
> person who will be implementing all that.
> 
> 
> [*] Note: right now, this does use the legacy ASN.1 stack which really parses
> BER, not DER. It's absurdly lenient; we didn't even notice this:
>
https://boringssl.googlesource.com/boringssl/+/cb41d7702980776e61f46c535cfdef...
> 
> However, fairly soon those will be using crypto/bytestring. Then they will be,
> as far as I know, completely strict w.r.t. DER. So that's something we'll need
> to sniff at for compat once that's in.

Thanks David!

So my take-away is:

  (1) Switch to d2i_RSAPublicKey() / d2i_EC_PUBKEY()
  (2) Longer term we want to either expose different API in boringssl, or move
the parsing into net/cert

The same issue applies to WebCrypto, which to be spec compliant should just
implement the WebCrypto version of parsing and writing.

If we decide against keeping that logic in boringssl, I can implement the needed
parsing here for WebCrypto and net/cert. However my gut feeling is that it
probably should just stay in boringssl which as you say still has a need to
parse SPKI, so seems like it should be the canonical place for that.

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:76: return *out;
On 2015/07/07 18:03:57, David Benjamin wrote:
> I believe MSVC will grump about this and want !!*out or *out != nullptr.

Done.

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:103: // the needed checks?
On 2015/07/07 18:03:56, David Benjamin wrote:
> On 2015/07/07 14:07:31, Ryan Sleevi (slow through 7-15 wrote:
> > David: Look here
> 
> I don't believe BoringSSL is even passed the signature algorithm here, so it
> couldn't do it if we wanted to. But, no, an EVP_PKEY has no constraints
attached
> to it. It's just a thin wrapper over RSA and EC_KEY, nothing more.

Thanks!

Indeed, seems the checks are in fact quite necessary.
My code as written would happily inflate a non RSA-key from SPKI, and then do
the verification using that key type.

I will fix this and update the tests.

Sadly I mistakenly thought I was already testing for this with:
  rsa-using-ec-key.pem
  ecdsa-using-rsa-key.pem

But the test construction for those is such that I swapped out the key of
otherwise valid parameters, whereas swapping the algorithm would have made for a
better test.

[davidben, 2015-07-07 18:53:21]

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/120001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:52: pkey->reset(d2i_PUBKEY(nullptr,
&ptr, spki.Length()));
On 2015/07/07 18:34:43, eroman wrote:
> On 2015/07/07 18:03:56, David Benjamin wrote:
> > This needs x509.h. I'd rather we not use x509.h, but it's true that
pub_encode
> /
> > pub_decode hooks on the EVP_PKEY aren't currently exposed anywhere. Not sure
> yet
> > what I want to do with those.
> > 
> > We could use this for now, and I could get you folks a fresh API that
doesn't
> > use the legacy ASN.1 stack. But the question there is, like with
> > AlgorithmIdentifier in signatures, do we want that within BoringSSL or do we
> > want that in net/cert? net/cert is in a better position to enforce things
> like:
> > 
> > - RFC 4055 defines an id-RSASSA-PSS for the SPKI to make the key only allow
> > RSA-PSS? I'm not sure BoringSSL even accepts it at all right now.
> > https://tools.ietf.org/html/rfc4055#section-3.3
> > 
> > - RFC 3279 says RSA keys are identified by rsaEncryption. d2i_PUBKEY will
> accept
> > either rsaEncryption or 2.5.8.1.1 apparently. I dunno whether compatibility
> > requires we accept both, but it's certainly easier to change a new thing
than
> an
> > old; the way it's plumbed into BoringSSL is pretty invasive.
> > 
> > - SPKIs for EC keys may use id-ecPublicKey or id-ecDH. The latter is useless
> for
> > us, but we shouldn't allow those for ECDSA. BoringSSL currently doesn't
parse
> > id-ecDH at all (and I doubt it ever will), but that's not necessarily in the
> > contract of a "SPKI -> EVP_PKEY" function. At least right now, an EVP_PKEY
> > doesn't include any constraints on it. It's a pretty thin abstraction over
EC
> > and RSA keys. (And DSA, but that only half-works.)
> > 
> > - Speaking of DSA, d2i_PUBKEY will happily decode DSA keys right now.
They'll
> > decode, but operations on them won't work. (It's the bare minimum needed
thus
> > far to appease some internal consumers. Hopefully it'll go away again later,
> but
> > it does need to be routed up for now.)
> > 
> > Then again, processing SPKIs may be a generally useful thing. Certainly the
> SSL
> > stack needs to extract the key even in a post crypto/x509 world. I don't
have
> > strong opinions as to whether that should work by parsing a tiny X.509
subset
> in
> > ssl/ or if the "here is a bunch of certificates I got over the wire; do they
> > look good?" callback should also have the embedder give back the public key.
> The
> > latter seems kinda tempting actually. Nice to not have things being parsed
> > twice.
> > 
> > 
> > I currently lean towards having net/cert parse those and instead call
> > lower-level functions like d2i_RSAPublicKey (soon to no longer use the
legacy
> > ASN.1 stack[*], so I'll have a nicer API for you later). But I'm also not
the
> > person who will be implementing all that.
> > 
> > 
> > [*] Note: right now, this does use the legacy ASN.1 stack which really
parses
> > BER, not DER. It's absurdly lenient; we didn't even notice this:
> >
>
https://boringssl.googlesource.com/boringssl/+/cb41d7702980776e61f46c535cfdef...
> > 
> > However, fairly soon those will be using crypto/bytestring. Then they will
be,
> > as far as I know, completely strict w.r.t. DER. So that's something we'll
need
> > to sniff at for compat once that's in.
> 
> Thanks David!
> 
> So my take-away is:
> 
>   (1) Switch to d2i_RSAPublicKey() / d2i_EC_PUBKEY()
>   (2) Longer term we want to either expose different API in boringssl, or move
> the parsing into net/cert
> 
> The same issue applies to WebCrypto, which to be spec compliant should just
> implement the WebCrypto version of parsing and writing.
> 
> If we decide against keeping that logic in boringssl, I can implement the
needed
> parsing here for WebCrypto and net/cert. However my gut feeling is that it
> probably should just stay in boringssl which as you say still has a need to
> parse SPKI, so seems like it should be the canonical place for that.

Oh, good point. I hadn't thought about WebCrypto.

That does beg the question of what a "parse SPKI" API should look like though.
If BoringSSL internally parses SPKI, we'll need to figure out the right story
for enforcing all the things we want to enforce. Either by pushing things into
EVP_PKEY or having {net/cert,WebCrypto} extract things out of the AlgID in
parallel or a bit of both.

Ryan, do you have opinions here?

[eroman, 2015-07-07 21:19:47]

I have updated to using d2i_EC_PUBKEY() / d2i_RSA_PUBKEY() rather than the more
generic d2i_PUBKEY().

This still leaves some open questions with regards to how we enforce policy
encoded in that SPKI, versus the signatureAlgorithm. Minimally right now the key
types must match.

But because this implementation just parses SPKI as either EC or RSA key, it
might be overly permissive:

  * An SPKI with PSS parameters or OID could be used with PKCS#1 signature
verification.

  * Not sure if there are OIDs or parameters that bind EC SPKI to specific
algorithms. If there are, then those aren't being checked either, and you would
be able to for instance encode the key for ECDSA as an ECDH key.

[davidben, 2015-07-07 21:28:20]

On 2015/07/07 21:19:47, eroman wrote:
> I have updated to using d2i_EC_PUBKEY() / d2i_RSA_PUBKEY() rather than the
more
> generic d2i_PUBKEY().
> 
> This still leaves some open questions with regards to how we enforce policy
> encoded in that SPKI, versus the signatureAlgorithm. Minimally right now the
key
> types must match.
> 
> But because this implementation just parses SPKI as either EC or RSA key, it
> might be overly permissive:
> 
>   * An SPKI with PSS parameters or OID could be used with PKCS#1 signature
> verification.
> 
>   * Not sure if there are OIDs or parameters that bind EC SPKI to specific
> algorithms. If there are, then those aren't being checked either, and you
would
> be able to for instance encode the key for ECDSA as an ECDH key.

[Sorry, I should have been clearer. Skimming RFC 5480, there's three OIDs for
ECC:

id-ecPublicKey
id-ecDH
id-ecMQV

Only the first may be used for ECDSA. It may also a priori be used for ECDH. In
parallel there's also key usage bits in the certificate itself, including
digitalSignature, keyAgreement, keyCertSign, etc. Those *also* control what the
key may be used for.

BoringSSL doesn't understand any but id-ecPublicKey, and I don't think there's
actually any danger of that changing. But it is an example of key usage being
partly determine by the SPKI AlgorithmIdentifier, which means that SPKI ->
EVP_PKEY is a slightly weird API. We'd either want EVP_PKEY to capture key
usages internally (right now it doesn't capture any, so that's changing what
EVP_PKEY is), net/cert to process SPKI's AlgID (in addition to BoringSSL doing
so internally) which is a little odd, or having an SPKI -> EVP_PKEY +
some-representation-of-usages API.


SPKI is an extremely uninteresting structure, so having WebCrypto and net/cert
have their own code to parse it might not be wrong. Both X.509 and WebCrypto
need to specify exactly how usage constraints map onto their respective
operations anyway. So it makes some sense for their implementations to have
dedicated code for this.

"This also saves me having to port d2i_PUBKEY to crypto/bytestring." :-) Right
now it has the same BER/DER caveat as anything else using the legacy OpenSSL
stuff.]

[eroman, 2015-07-07 21:28:37]

In fact I imagine I should also be doing a whitelist for the allowed EC named
curves?

Since otherwise the decision of curve is being left up to BoringSSL based on
what it can parse from the SPKI, which maybe isn't the best idea.

[eroman, 2015-07-15 02:09:47]

I have updated this CL, please take another look.

I am still using BoringSSL's d2i_RSA_PUBKEY(), and d2i_EC_PUBKEY() to parse the
SPKI, however I have:

  * Added some comment blocks explaining what the format should be (based on RFC
5912)
  * Added more tests (2 of which are disabled)

The disabled tests involve the use of rsaPss as the algorithm identifier in
public keys (which BoringSSL does not support). Rather than address that in this
CL I will do so in a follow-up. (Which is to say currently only rsaEncryption
keys are allowed when doing rsaPss verification).

Another compatibility issue I noticed is that according to RFC 5912,
rsaEncryption (pk-rsa) keys should have absent algorithm parameters. In practice
though every SPKI I came across encodes them as NULL, and BoringSSL accepts
either (and outputs them as NULL). So I don't expect we will want to be spec
compliant in that regard. Added a note and marked the tests as passing.

The additional tests I added since the last review are all related to
modifications to the SPKI:

ecdsa-prime256v1-sha512-using-ecmqv-key.pem
ecdsa-prime256v1-sha512-using-ecdh-key.pem
rsa-pkcs1-sha1-key-params-absent.pem
rsa-pss-sha1-salt20-using-pss-key-no-params.pem
rsa-pkcs1-sha1-using-pss-key-no-params.pem
rsa-pss-sha256-salt10-using-pss-key-with-params.pem
rsa-pss-sha256-salt10-using-pss-key-with-wrong-params.pem
rsa-pss-sha1-salt20-using-pss-key-with-null-params.pem

[Ryan Sleevi, 2015-07-16 04:05:24]

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:12: // not worth the effort at this
point.
iOS is still affected by this at present.

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:162: EVP_PKEY_CTX* pctx = NULL;  //
Owned by |ctx|.
nullptr?

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:168: if
(!EVP_DigestVerifyInit(ctx.get(), &pctx, digest, NULL, public_key.get()))
nullptr

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:279: crypto::ScopedEVP_MD_CTX
ctx(EVP_MD_CTX_create());
From here through 292, it seems the same as RsaVerify. Should we align those two
more into a single helper function? Why split - it seems the only split needed
is for key parsing?

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:285: if
(!EVP_DigestVerifyInit(ctx.get(), NULL, digest, NULL, public_key.get()) ||
nullptr

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data_unittest.cc (right):

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:27: der::Input
InputFromString(std::string& s) {
I understand why, but this isn't allowed by the style guide -
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg...

"All parameters passed by reference must be labeled const."
"but we never allow non-const reference parameters except when required by
convention, e.g., swap()"

Seems like a "const std::string* s" would accomplish this, and still require you
to bind to named params (in effect)

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

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:12: // not worth the effort at this
point.
On 2015/07/16 04:05:23, Ryan Sleevi (slow through 7-15 wrote:
> iOS is still affected by this at present.

Would you prefer I designate this file _openssl and add the necessary build
rules?

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:279: crypto::ScopedEVP_MD_CTX
ctx(EVP_MD_CTX_create());
On 2015/07/16 04:05:24, Ryan Sleevi (slow through 7-15 wrote:
> From here through 292, it seems the same as RsaVerify. Should we align those
two
> more into a single helper function? Why split - it seems the only split needed
> is for key parsing?

The RSA version is somewhat different, in that it associates a EVP_PKEY_CTX, and
then sets some parameters on it in the PSS case.

I can merge them if I do a if/else for the key import, and then test for PSS.
I will try it out see how I like the result.

[eroman, 2015-07-18 22:44:23]

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:279: crypto::ScopedEVP_MD_CTX
ctx(EVP_MD_CTX_create());
On 2015/07/16 17:34:07, eroman wrote:
> On 2015/07/16 04:05:24, Ryan Sleevi (slow through 7-15 wrote:
> > From here through 292, it seems the same as RsaVerify. Should we align those
> two
> > more into a single helper function? Why split - it seems the only split
needed
> > is for key parsing?
> 
> The RSA version is somewhat different, in that it associates a EVP_PKEY_CTX,
and
> then sets some parameters on it in the PSS case.
> 
> I can merge them if I do a if/else for the key import, and then test for PSS.
> I will try it out see how I like the result.

Done.

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:285: if
(!EVP_DigestVerifyInit(ctx.get(), NULL, digest, NULL, public_key.get()) ||
On 2015/07/16 04:05:24, Ryan Sleevi wrote:
> nullptr

Done (throughout)

[davidben, 2015-07-20 19:21:59]

Ryan: What is the story with the usage-specific SPKIs like id-RSASSA-PSS,
id-ecDH, and id-ecMQV anyway? EVP_PKEY has no notion of usage. It's a very
transparent discriminated union, not a tight abstraction. Instead, nothing in
BoringSSL or OpenSSL ever recognizes those things.

id-ecMQV and id-ecDH are easy. We can definitely pretend those don't exist. Is
there any need to ever support id-RSASSA-PSS? It can only be used for an
intermediate certificate since TLS doesn't use RSA-PSS signatures today. Does
anything use this? (Note that this is distinct from RSA-PSS in the signature
algorithm. This is specifically about RSA-PSS in the SPKI.)


This determines whether we should be writing our own SPKI handling completely
and if it makes any sense to lean on BoringSSL to parse out SPKIs. As I see it,
our options are:

1. Declare that usage-specific SPKIs are verboten. Now the SPKI -> EVP_PKEY
functions in BoringSSL can actually make sense. Implement a new version in
crypto/ and use those. This is documented to only parse usage-unconstrained
ones, so type signatures of SPKI -> (EVP_PKEY|RSA|EC_KEY) are defensible, and it
works to enforce policy by checking the result of the parse after-the-fact, as
this CL does.

2. Support crazy usage-specific SPKIs. For the PKI stack, SPKI parsing needs to
be external to BoringSSL because BoringSSL doesn't know about usage. BoringSSL
probably still retains an internal SPKI parser that can only handle
usage-unconstrained SPKIs. Those will be used within the ssl/ stack because the
ssl/ stack can't do RSA-PSS anyway.


#2 is sound, but it seems rather unnecessary complexity. What's the point of
those key types? Unless there's an actual need here, I think I now prefer #1.
(I'd previously advocated for #2, but the ramifications for the SSL stack hadn't
occurred to me.)

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:46: *out = EVP_sha1();
#include <openssl/digest.h>

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:59: return *out != nullptr;
This assumes that *out was initialized to nullptr (which it isn't in line 71).

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:78: pctx,
salt_length_bytes_int.ValueOrDie()));
In BoringSSL, these can only return 0 or 1.

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:85: // There are two flavors of RSA
public key that this function recognizes from
Currently, d2i_RSA_PUBKEY also accepts 2.5.8.1.1.

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:99: //     pk-rsaSSA-PSS PUBLIC-KEY ::=
{
d2i_RSA_PUBKEY doesn't actually recognize this one. This goes back again to the
question of what we want to do about usage-specific SPKIs.

Probably simplest to, for the first version, just not support them.

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:123: // algorithm.
This isn't implemented.

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:127: // for these to be present and
NULL.
This one says it MUST be NULL.
https://tools.ietf.org/html/rfc3279#section-2.3.1

I'm not familiar with the ASN.1 syntax of RFC 5912, but if "ARE absent" means
what I assume it means, this seems a bug in RFC 5912. (5912 seems to only exist
because "Some developers would like the IETF to use the latest version of ASN.1
in its standards." and... honestly I like the 1988 ASN.1 better. This new one is
hard to read.)


I should not that, we can fix this but, currently, d2i_RSA_PUBKEY will just
ignore the parameters. So if there's garbage in there, it won't notice.


I can add a crypto/bytestring-based SPKI parser within BoringSSL that avoids a
lot of these stupid bits. But I think we need a ruling on how SPKI
responsibilities are supposed to be divided between net/cert/,
boringssl/src/crypto/, and boringssl/src/ssl/.

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:133: crypto::ScopedRSA rsa(
#include <openssl/rsa.h>

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:141: if (!pkey ||
!EVP_PKEY_set1_RSA(pkey->get(), rsa.get()))
This is slightly odd since d2i_RSA_PUBKEY will actually get you an EVP_PKEY and
then dismantle it. And then this is creating a wrapper anew.

(This is part of the usual "anything based on the legacy crypto/x509 stack is
extremely shoddy in quality and probably should be ditched".)

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:255: if
(!IsAllowedCurveName(EC_GROUP_get_curve_name(EC_KEY_get0_group(ec.get()))))
#include <openssl/ec.h>
#include <openssl/ec_key.h>

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:260: if (!pkey ||
!EVP_PKEY_set1_EC_KEY(pkey->get(), ec.get()))
(Ditto about this dismantling an EVP_PKEY, only to reassemble it.)

[eroman, 2015-07-20 20:03:40]

> Ryan: What is the story with the usage-specific SPKIs like id-RSASSA-PSS,
> id-ecDH, and id-ecMQV anyway? EVP_PKEY has no notion of usage. It's a very
> transparent discriminated union, not a tight abstraction. Instead, nothing in
> BoringSSL or OpenSSL ever recognizes those things.

id-ecDH, id-ecMQV seem fine as is (not needed for ECDSA).

As far as id-RSASSA-PSS, even if we decide against implementing it for
certificate verification, we still need it for WebCrypto compliance:

(see Import Key definition):

https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#rsa-pss-...

I would say do this implementation outside of BoringSSL, and use it both for our
cert verifier, and for WebCrypto, so they match the standards.

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data_unittest.cc (right):

https://codereview.chromium.org/1209283004/diff/320001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:27: der::Input
InputFromString(std::string& s) {
On 2015/07/16 04:05:24, Ryan Sleevi wrote:
> I understand why, but this isn't allowed by the style guide -
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg...
> 
> "All parameters passed by reference must be labeled const."
> "but we never allow non-const reference parameters except when required by
> convention, e.g., swap()"
> 
> Seems like a "const std::string* s" would accomplish this, and still require
you
> to bind to named params (in effect)

Done.

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:46: *out = EVP_sha1();
On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> #include <openssl/digest.h>

Done.

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:59: return *out != nullptr;
On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> This assumes that *out was initialized to nullptr (which it isn't in line 71).

Done. Good catch.

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:78: pctx,
salt_length_bytes_int.ValueOrDie()));
On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> In BoringSSL, these can only return 0 or 1.

Done.

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:85: // There are two flavors of RSA
public key that this function recognizes from
On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> Currently, d2i_RSA_PUBKEY also accepts 2.5.8.1.1.

Acknowledged. Note that this is covered by my tests:

VerifySignedDataTest.DISABLED_RsaPss_Sha1_Salt20_UsingPssKeyNoParams
VerifySignedDataTest.DISABLED_RsaPss_Sha256_Salt10_UsingPssKeyWithParams

Which for obvious reasons is disabled :)

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:99: //     pk-rsaSSA-PSS PUBLIC-KEY ::=
{
On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> d2i_RSA_PUBKEY doesn't actually recognize this one. This goes back again to
the
> question of what we want to do about usage-specific SPKIs.
> 
> Probably simplest to, for the first version, just not support them.

Acknowledged. Same response as above.

I will add a TODO() to make this more obvious.

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:123: // algorithm.
On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> This isn't implemented.

Acknowledged.

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:127: // for these to be present and
NULL.
On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> This one says it MUST be NULL.
> https://tools.ietf.org/html/rfc3279#section-2.3.1
> 
> I'm not familiar with the ASN.1 syntax of RFC 5912, but if "ARE absent" means
> what I assume it means, this seems a bug in RFC 5912. (5912 seems to only
exist
> because "Some developers would like the IETF to use the latest version of
ASN.1
> in its standards." and... honestly I like the 1988 ASN.1 better. This new one
is
> hard to read.)
> 

Correct, "ARE absent" in this case means MUST be absent. RFC 5912 uses
"preferredAbsent" for things that SHOULD be absent:

ParamOptions ::= ENUMERATED {
   required,         -- Parameters MUST be encoded in structure
   preferredPresent, -- Parameters SHOULD be encoded in structure
   preferredAbsent,  -- Parameters SHOULD NOT be encoded in structure
   absent,           -- Parameters MUST NOT be encoded in structure
   inheritable,      -- Parameters are inherited if not present
   optional,         -- Parameters MAY be encoded in the structure
   ...
}

It is frustrating that these RFCs are not in agreement on certain things.

How should I proceed with these checks, which RFCs to give precedence to?

Obviously in this case RFC 5912's definition is not helpful, since I didn't
actually come across any where it wasn't NULL.

I have added a reference to 3279 in the compatibility note, but perhaps it would
be better for me to re-write the documentation without using 5912 as the
authoritative source.

> 
> I should not that, we can fix this but, currently, d2i_RSA_PUBKEY will just
> ignore the parameters. So if there's garbage in there, it won't notice.
> 
> 
> I can add a crypto/bytestring-based SPKI parser within BoringSSL that avoids a
> lot of these stupid bits. But I think we need a ruling on how SPKI
> responsibilities are supposed to be divided between net/cert/,
> boringssl/src/crypto/, and boringssl/src/ssl/.

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:133: crypto::ScopedRSA rsa(
On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> #include <openssl/rsa.h>

Done.

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:141: if (!pkey ||
!EVP_PKEY_set1_RSA(pkey->get(), rsa.get()))
On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> This is slightly odd since d2i_RSA_PUBKEY will actually get you an EVP_PKEY
and
> then dismantle it. And then this is creating a wrapper anew.
> 
> (This is part of the usual "anything based on the legacy crypto/x509 stack is
> extremely shoddy in quality and probably should be ditched".)

Do you want me to go back to using d2i_PUBKEY() as in the earlier patchset? (and
test the key type for ECDSA vs RSA of course).

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:255: if
(!IsAllowedCurveName(EC_GROUP_get_curve_name(EC_KEY_get0_group(ec.get()))))
On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> #include <openssl/ec.h>
> #include <openssl/ec_key.h>

Done.

[davidben, 2015-07-20 21:17:23]

> As far as id-RSASSA-PSS, even if we decide against implementing it for
> certificate verification, we still need it for WebCrypto compliance:

Hrmf. Could WebCrypto not do that? That seems kind of obnoxious. :-/ Also means
our WebCrypto implementation isn't actually complaint today, right? BoringSSL
doesn't support id-RSASSA-PSS, and WebCrypto says:

- Import accepts id-RSASSA-PSS or id-rsaEncryption, former gets some minimal
checks done. (But not all! It doesn't enforce saltLength's constraint; see RFC
4055, section 3.3, scenario 3.)

- Export always writes id-RSASSA-PSS.

What actual problem is being solved by doing that over just using
id-rsaEncryption + RSAPublicKey for all public RSA keys?

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:127: // for these to be present and
NULL.
On 2015/07/20 20:03:40, eroman wrote:
> On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> > This one says it MUST be NULL.
> > https://tools.ietf.org/html/rfc3279#section-2.3.1
> > 
> > I'm not familiar with the ASN.1 syntax of RFC 5912, but if "ARE absent"
means
> > what I assume it means, this seems a bug in RFC 5912. (5912 seems to only
> exist
> > because "Some developers would like the IETF to use the latest version of
> ASN.1
> > in its standards." and... honestly I like the 1988 ASN.1 better. This new
one
> is
> > hard to read.)
> > 
> 
> Correct, "ARE absent" in this case means MUST be absent. RFC 5912 uses
> "preferredAbsent" for things that SHOULD be absent:
> 
> ParamOptions ::= ENUMERATED {
>    required,         -- Parameters MUST be encoded in structure
>    preferredPresent, -- Parameters SHOULD be encoded in structure
>    preferredAbsent,  -- Parameters SHOULD NOT be encoded in structure
>    absent,           -- Parameters MUST NOT be encoded in structure
>    inheritable,      -- Parameters are inherited if not present
>    optional,         -- Parameters MAY be encoded in the structure
>    ...
> }
> 
> It is frustrating that these RFCs are not in agreement on certain things.
> 
> How should I proceed with these checks, which RFCs to give precedence to?
> 
> Obviously in this case RFC 5912's definition is not helpful, since I didn't
> actually come across any where it wasn't NULL.

If RFC 3279 is treated authoritative, it's not even allowed to be omitted. And
mozilla::pkix will reject the SPKI if not NULL. I think RFC 5912 is just wrong
here.

> I have added a reference to 3279 in the compatibility note, but perhaps it
would
> be better for me to re-write the documentation without using 5912 as the
> authoritative source.

Dunno. A priori, I would think RFC 3279 would be more usefully authoritative,
and any discrepancies in RFC 5912 are just bugs, but at this point you're
probably more familiar with the X.509 landscape than me.

> > 
> > I should not that, we can fix this but, currently, d2i_RSA_PUBKEY will just
> > ignore the parameters. So if there's garbage in there, it won't notice.
> > 
> > 
> > I can add a crypto/bytestring-based SPKI parser within BoringSSL that avoids
a
> > lot of these stupid bits. But I think we need a ruling on how SPKI
> > responsibilities are supposed to be divided between net/cert/,
> > boringssl/src/crypto/, and boringssl/src/ssl/.
>

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:141: if (!pkey ||
!EVP_PKEY_set1_RSA(pkey->get(), rsa.get()))
On 2015/07/20 20:03:40, eroman wrote:
> On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> > This is slightly odd since d2i_RSA_PUBKEY will actually get you an EVP_PKEY
> and
> > then dismantle it. And then this is creating a wrapper anew.
> > 
> > (This is part of the usual "anything based on the legacy crypto/x509 stack
is
> > extremely shoddy in quality and probably should be ditched".)
> 
> Do you want me to go back to using d2i_PUBKEY() as in the earlier patchset?
(and
> test the key type for ECDSA vs RSA of course).

*shrug* That's all d2i_RSA_PUBKEY is doing.

But if the plan is to parse SPKIs here and not lean on BoringSSL at all, I
suppose it doesn't really matter since it'll get replaced with der::Input
anyway. (Any reason not to just do it now? You can even use
RSA_public_key_from_bytes which will save you the &ptr thing, and the check that
you consumed everything.)

https://boringssl.googlesource.com/boringssl/+/master/include/openssl/rsa.h#333

On the EC side, it's slightly more effort because the parameters include things,
but you o2i_ECPublicKey (hrm, the documentation is slightly off; it's not
parsing a DER structure) and pass in an EC_KEY that has the group already set.
I'll add a EC_KEY_public_from_bytes(EC_GROUP*, const uint8_t*, size_t) later.

https://boringssl.googlesource.com/boringssl/+/master/include/openssl/ec_key....

[eroman, 2015-07-20 22:28:08]

> Hrmf. Could WebCrypto not do that? That seems kind of obnoxious. :-/ Also
means
> our WebCrypto implementation isn't actually complaint today, right? BoringSSL
> doesn't support id-RSASSA-PSS, and WebCrypto says:

Correct, the current WebCrypto implementation is not compliant with regard to
PKCS8/SPKI import/export:

https://code.google.com/p/chromium/issues/detail?id=389400
https://code.google.com/p/chromium/issues/detail?id=373545

On the spec side, this is the bug discussing the tradeoffs and possible removal
of these constraints:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=27255

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:141: if (!pkey ||
!EVP_PKEY_set1_RSA(pkey->get(), rsa.get()))
On 2015/07/20 21:17:22, David Benjamin slow until 7-21 wrote:
> On 2015/07/20 20:03:40, eroman wrote:
> > On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> > > This is slightly odd since d2i_RSA_PUBKEY will actually get you an
EVP_PKEY
> > and
> > > then dismantle it. And then this is creating a wrapper anew.
> > > 
> > > (This is part of the usual "anything based on the legacy crypto/x509 stack
> is
> > > extremely shoddy in quality and probably should be ditched".)
> > 
> > Do you want me to go back to using d2i_PUBKEY() as in the earlier patchset?
> (and
> > test the key type for ECDSA vs RSA of course).
> 
> *shrug* That's all d2i_RSA_PUBKEY is doing.
> 
> But if the plan is to parse SPKIs here and not lean on BoringSSL at all, I
> suppose it doesn't really matter since it'll get replaced with der::Input
> anyway. (Any reason not to just do it now? You can even use
> RSA_public_key_from_bytes which will save you the &ptr thing, and the check
that
> you consumed everything.)
> 
>
https://boringssl.googlesource.com/boringssl/+/master/include/openssl/rsa.h#333
> 
> On the EC side, it's slightly more effort because the parameters include
things,
> but you o2i_ECPublicKey (hrm, the documentation is slightly off; it's not
> parsing a DER structure) and pass in an EC_KEY that has the group already set.
> I'll add a EC_KEY_public_from_bytes(EC_GROUP*, const uint8_t*, size_t) later.
> 
>
https://boringssl.googlesource.com/boringssl/+/master/include/openssl/ec_key....

Done, changed to use d2i_PUBKEY().

> Any reason not to just do it now?

My preference is to explore any custom parsing of the spki as a separate CL.

The straightforward call to d2i_PUBKEY() gets us pretty far, and in fact likely
supports most if not all of the cases we care about for the initial milestone
(the only unsupported functionality is specifying a *key* algorithm as
RSASSA-PSS).

Custom parsing for RSASSA-PSS spki and validating against the signature
algorithm is enough work that I consider it a lower-priority follow-up. Mozilla
pkix for instance does not even support RSA-PSS, let alone RSA-PSS as the public
key algorithm.

Too many more patchsets on this CL risks review fatigue, but of course I defer
to my reviewers :)

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:260: if (!pkey ||
!EVP_PKEY_set1_EC_KEY(pkey->get(), ec.get()))
On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> (Ditto about this dismantling an EVP_PKEY, only to reassemble it.)

Done.

[davidben, 2015-07-21 15:29:06]

(Looking at current iteration of CL now.)

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/360001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:141: if (!pkey ||
!EVP_PKEY_set1_RSA(pkey->get(), rsa.get()))
On 2015/07/20 22:28:08, eroman wrote:
> On 2015/07/20 21:17:22, David Benjamin slow until 7-21 wrote:
> > On 2015/07/20 20:03:40, eroman wrote:
> > > On 2015/07/20 19:21:59, David Benjamin slow until 7-21 wrote:
> > > > This is slightly odd since d2i_RSA_PUBKEY will actually get you an
> EVP_PKEY
> > > and
> > > > then dismantle it. And then this is creating a wrapper anew.
> > > > 
> > > > (This is part of the usual "anything based on the legacy crypto/x509
stack
> > is
> > > > extremely shoddy in quality and probably should be ditched".)
> > > 
> > > Do you want me to go back to using d2i_PUBKEY() as in the earlier
patchset?
> > (and
> > > test the key type for ECDSA vs RSA of course).
> > 
> > *shrug* That's all d2i_RSA_PUBKEY is doing.
> > 
> > But if the plan is to parse SPKIs here and not lean on BoringSSL at all, I
> > suppose it doesn't really matter since it'll get replaced with der::Input
> > anyway. (Any reason not to just do it now? You can even use
> > RSA_public_key_from_bytes which will save you the &ptr thing, and the check
> that
> > you consumed everything.)
> > 
> >
>
https://boringssl.googlesource.com/boringssl/+/master/include/openssl/rsa.h#333
> > 
> > On the EC side, it's slightly more effort because the parameters include
> things,
> > but you o2i_ECPublicKey (hrm, the documentation is slightly off; it's not
> > parsing a DER structure) and pass in an EC_KEY that has the group already
set.
> > I'll add a EC_KEY_public_from_bytes(EC_GROUP*, const uint8_t*, size_t)
later.
> > 
> >
>
https://boringssl.googlesource.com/boringssl/+/master/include/openssl/ec_key....
> 
> Done, changed to use d2i_PUBKEY().
> 
> > Any reason not to just do it now?
> 
> My preference is to explore any custom parsing of the spki as a separate CL.
> 
> The straightforward call to d2i_PUBKEY() gets us pretty far, and in fact
likely
> supports most if not all of the cases we care about for the initial milestone
> (the only unsupported functionality is specifying a *key* algorithm as
> RSASSA-PSS).
> 
> Custom parsing for RSASSA-PSS spki and validating against the signature
> algorithm is enough work that I consider it a lower-priority follow-up.
Mozilla
> pkix for instance does not even support RSA-PSS, let alone RSA-PSS as the
public
> key algorithm.
> 
> Too many more patchsets on this CL risks review fatigue, but of course I defer
> to my reviewers :)

Fair enough. Though RSASSA-PSS SPKI isn't the only thing that we get out of
doing the SPKI parsing ourselves. We also get strictness (the weird extra OID is
supported, BER is supported, etc). I was actually thinking just implement
id-rsaEncryption manually which shouldn't be much work. But, yeah, let's leave
it as a follow-up.

[davidben, 2015-07-21 16:26:27]

One last set of comments. (Sorry, I should have done the more careful review on
the whole CL last round. Was mostly just thinking about the SPKI thing before.)

I didn't really check the test data and assume you generated it right.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:22: // Not implemented
Nit: NOTIMPLEMENTED()

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:47: WARN_UNUSED_RESULT bool
GetDigest(DigestAlgorithm digest, const EVP_MD** out) {
#include "base/compiler_specific.h"

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:47: WARN_UNUSED_RESULT bool
GetDigest(DigestAlgorithm digest, const EVP_MD** out) {
Optional: Could make this return const EVP_MD* and just nullptr on failure. That
would also resolve the "modifies output parameters on failure" thing.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:81: return
(EVP_PKEY_CTX_set_rsa_padding(pctx, RSA_PKCS1_PSS_PADDING) &&
Nit: Unnecessary parentheses. (Chromium's not very consistent, but without seems
more common.)

https://code.google.com/p/chromium/codesearch#search/&q=return%5C%20%5C(.*&am...
https://code.google.com/p/chromium/codesearch#search/&q=return%5C%20%5B%5E(%5...

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:89: crypto::ScopedEVP_PKEY* pkey) {
Optional: Could return crypto::ScopedEVP_PKEY rather than use an out-param. That
would also resolve the "modifies output parameters on failure" thing.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:93: pkey->reset(d2i_PUBKEY(nullptr,
&ptr, spki.Length()));
This probably needs a TODO to fix the strictness (not in this CL, yeah),
probably by switching to a dedicated parser. It's not just for id-RSASSA-PSS,
but also to actually enforce all the things we're supposed to enforce. BER, not
using the bogus alternative RSA OID, and actually checking id-rsaEncryption
params. (Or would you rather I go fix that in BoringSSL? We'll want to do that
anyway, and if that happens, I suppose we don't need to switch parsers until
id-RSASSA-PSS is added.)

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:151: // RFC 3279 section 2.3.1 which
says it should be NULL:
Nit: should -> MUST

(I.e. it's not that RFC 3279 allows both and RFC 5912 insists on the other.
They're flat-out contradictory.)

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:159: crypto::ScopedEVP_PKEY* pkey) {
Optional: Could return crypto::ScopedEVP_PKEY rather than use an out-param. That
would also resolve the "modifies output parameters on failure" thing.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:168: DCHECK(algorithm.algorithm() ==
SignatureAlgorithmId::RsaPkcs1 ||
#include "base/logging.h"

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:259: crypto::ScopedEVP_PKEY* pkey) {
Optional: Could return crypto::ScopedEVP_PKEY rather than use an out-param.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.h (right):

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.h:28: //        not the BIT STRING's DER
itself.
"byte contents of that bit string" vs "BIT STRING's DER itself" is a little
ambiguous. I could either interpret it as:

- signature_value is the byte string with the 00 phase octet removed, not the
contents of the DER-encoded BIT STRING, phase octet included

- signature_value is the byte string contents of the BIT STRING, including the
phase octet, not the entire DER element.

Going by the code, it seems you mean the former.


I would write this as:

// IMPORTANT: In RFC 5280, signatureValue is a BIT STRING, however all supported
signature algorithms use byte strings encoded as a BIT STRING with a consistent
encoding. |signature_value| is a byte string, so the caller must decode the BIT
STRING by removing the first DER contents octet and checking the value is zero.

The specs are apparently incredibly badly-written on this point. RFC 3279,
section 2.2 just says "This signature value is then ASN.1 encoded as a BIT
STRING" and then goes on to describe three signature formats of type []byte, not
[]bit. RFC 4055, section 3.2 is much more explicit about the encoding.)


The other option is what signature_value is actually the BIT STRING
representation and VerifySignedData internally strips off and checks/strips the
leading phase octet internally. (We can reject anything that's not 00. X.509
using BIT STRING was just plain a mistake.)

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data_unittest.cc (right):

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:70: return
::testing::AssertionSuccess();
This function never fails. (Did you mean for it to return a failure if any of
public_key through signature_value weren't found? Though you then have the usual
"modifies output parameters on failure" thing unless you also use some
temporaries.)

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:120: ASSERT_EQ(
Nit: I think this can just be EXPECT_EQ.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:221:
DISABLED_RsaPss_Sha256_Salt10_UsingPssKeyWithParams) {
Optional: Do you want to add disabled tests for 2.5.8.1.1 and bad
id-rsaEncryption params as well? Maybe also a non-minimally encoded
SubjectPublicKeyInfo.

https://codereview.chromium.org/1209283004/diff/460001/net/data/verify_signed...
File
net/data/verify_signed_data_unittest/rsa-pkcs1-sha1-using-pss-key-no-params.pem
(right):

https://codereview.chromium.org/1209283004/diff/460001/net/data/verify_signed...
net/data/verify_signed_data_unittest/rsa-pkcs1-sha1-using-pss-key-no-params.pem:6:
algorithm for PKCS#1 v1.5
Nit: period at the end.

https://codereview.chromium.org/1209283004/diff/460001/net/data/verify_signed...
File
net/data/verify_signed_data_unittest/rsa-pss-sha1-salt20-using-pss-key-no-params.pem
(right):

https://codereview.chromium.org/1209283004/diff/460001/net/data/verify_signed...
net/data/verify_signed_data_unittest/rsa-pss-sha1-salt20-using-pss-key-no-params.pem:3:
to rsaPss (1.2.840.113549.1.1.10)
Nit: period at the end.

[eroman, 2015-07-21 19:24:29]

> One last set of comments. (Sorry, I should have done the more careful review
on
> the whole CL last round. Was mostly just thinking about the SPKI thing
before.)

Thanks for the careful reviews!

> I didn't really check the test data and assume you generated it right.

Yeah the test data is a bit tricky to verify...

I can think of two ways to improve this:

  (1) Add the parsed ASN.1 beside the data (minimally helps verify algorithm
parameters, but could easily be out of sync with the PEM). I will see if I can
quickly whip together a script to annotate the data with the output from
"openssl asn1parse -i".

  (2) Add finer grained error codes, and assert the specific error in the
unit-tests. I found this approach worked pretty well for WebCrypto testing, and
gives extra confidence that failing inputs are failing for the right reason (and
not because the test is misconstructed). I talk more about error codes in my
other review responses.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:22: // Not implemented
On 2015/07/21 16:26:27, David Benjamin wrote:
> Nit: NOTIMPLEMENTED()

Done.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:47: WARN_UNUSED_RESULT bool
GetDigest(DigestAlgorithm digest, const EVP_MD** out) {
On 2015/07/21 16:26:27, David Benjamin wrote:
> #include "base/compiler_specific.h"

Done.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:47: WARN_UNUSED_RESULT bool
GetDigest(DigestAlgorithm digest, const EVP_MD** out) {
On 2015/07/21 16:26:27, David Benjamin wrote:
> Optional: Could make this return const EVP_MD* and just nullptr on failure.
That
> would also resolve the "modifies output parameters on failure" thing.

My thinking is to consistently return "bool" since I expect to change the
signature to an error result as a follow-up (once I have thought through more
carefully what the errors should be and their type).

I also had a slight paranoia in this specific instance that some BoringSSL
functions might treat a null EVP_MD as having a special meaning, and didn't want
the error case to inadvertently select some default behavior.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:81: return
(EVP_PKEY_CTX_set_rsa_padding(pctx, RSA_PKCS1_PSS_PADDING) &&
On 2015/07/21 16:26:27, David Benjamin wrote:
> Nit: Unnecessary parentheses. (Chromium's not very consistent, but without
seems
> more common.)
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=return%5C%20%5C(.*&am...
>
https://code.google.com/p/chromium/codesearch#search/&q=return%5C%20%5B%5E(%5...

Done.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:89: crypto::ScopedEVP_PKEY* pkey) {
On 2015/07/21 16:26:27, David Benjamin wrote:
> Optional: Could return crypto::ScopedEVP_PKEY rather than use an out-param.
That
> would also resolve the "modifies output parameters on failure" thing.

My preference is for a bool, as it will likely be changed to a result code soon.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:93: pkey->reset(d2i_PUBKEY(nullptr,
&ptr, spki.Length()));
On 2015/07/21 16:26:27, David Benjamin wrote:
> This probably needs a TODO to fix the strictness (not in this CL, yeah),
> probably by switching to a dedicated parser. It's not just for id-RSASSA-PSS,
> but also to actually enforce all the things we're supposed to enforce. BER,
not
> using the bogus alternative RSA OID, and actually checking id-rsaEncryption
> params. (Or would you rather I go fix that in BoringSSL? We'll want to do that
> anyway, and if that happens, I suppose we don't need to switch parsers until
> id-RSASSA-PSS is added.)

Done.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:151: // RFC 3279 section 2.3.1 which
says it should be NULL:
On 2015/07/21 16:26:27, David Benjamin wrote:
> Nit: should -> MUST
> 
> (I.e. it's not that RFC 3279 allows both and RFC 5912 insists on the other.
> They're flat-out contradictory.)

Done.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:159: crypto::ScopedEVP_PKEY* pkey) {
On 2015/07/21 16:26:27, David Benjamin wrote:
> Optional: Could return crypto::ScopedEVP_PKEY rather than use an out-param.
That
> would also resolve the "modifies output parameters on failure" thing.

Same response as earlier.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:168: DCHECK(algorithm.algorithm() ==
SignatureAlgorithmId::RsaPkcs1 ||
On 2015/07/21 16:26:27, David Benjamin wrote:
> #include "base/logging.h"

Done.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:259: crypto::ScopedEVP_PKEY* pkey) {
On 2015/07/21 16:26:27, David Benjamin wrote:
> Optional: Could return crypto::ScopedEVP_PKEY rather than use an out-param.

Same response as earlier.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.h (right):

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.h:28: //        not the BIT STRING's DER
itself.
On 2015/07/21 16:26:27, David Benjamin wrote:
> "byte contents of that bit string" vs "BIT STRING's DER itself" is a little
> ambiguous. I could either interpret it as:
> 
> - signature_value is the byte string with the 00 phase octet removed, not the
> contents of the DER-encoded BIT STRING, phase octet included
> 
> - signature_value is the byte string contents of the BIT STRING, including the
> phase octet, not the entire DER element.
> 
> Going by the code, it seems you mean the former.
> 
> 
> I would write this as:
> 
> // IMPORTANT: In RFC 5280, signatureValue is a BIT STRING, however all
supported
> signature algorithms use byte strings encoded as a BIT STRING with a
consistent
> encoding. |signature_value| is a byte string, so the caller must decode the
BIT
> STRING by removing the first DER contents octet and checking the value is
zero.
> 
> The specs are apparently incredibly badly-written on this point. RFC 3279,
> section 2.2 just says "This signature value is then ASN.1 encoded as a BIT
> STRING" and then goes on to describe three signature formats of type []byte,
not
> []bit. RFC 4055, section 3.2 is much more explicit about the encoding.)
> 
> 
> The other option is what signature_value is actually the BIT STRING
> representation and VerifySignedData internally strips off and checks/strips
the
> leading phase octet internally. (We can reject anything that's not 00. X.509
> using BIT STRING was just plain a mistake.)

OK so thinking about this more, I think it makes the most sense to just change
the input to a BIT STRING.

Initially I was torn on whether this API should take a BIT STRING, or require
the caller to extract the contents. I settled on the latter mainly out of
laziness, because much of my test data was already in that format.

But based on how it will be called, it is going to be more helpul to just take a
BIT STRING and do all the magic of extraction and making sure there non-multiple
of 8 bits etc in VerifySignedData.

Unless you object, I will start converting the test data to the new format and
update once that is complete (and then this comment can be simplified).

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data_unittest.cc (right):

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:70: return
::testing::AssertionSuccess();
On 2015/07/21 16:26:27, David Benjamin wrote:
> This function never fails. (Did you mean for it to return a failure if any of
> public_key through signature_value weren't found? Though you then have the
usual
> "modifies output parameters on failure" thing unless you also use some
> temporaries.)

Done  -- I additionally made it fail if a block is multiply defined, or never
defined.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:120: ASSERT_EQ(
On 2015/07/21 16:26:27, David Benjamin wrote:
> Nit: I think this can just be EXPECT_EQ.

Done.

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:221:
DISABLED_RsaPss_Sha256_Salt10_UsingPssKeyWithParams) {
On 2015/07/21 16:26:27, David Benjamin wrote:
> Optional: Do you want to add disabled tests for 2.5.8.1.1 and bad
> id-rsaEncryption params as well? Maybe also a non-minimally encoded
> SubjectPublicKeyInfo.

Yes, good ideas.

I will post another patchset when this is complete, and plan to add at least:
  * Missing params for rsaEncryption (I believe BoringSSL allows this)
  * Non-NULL params for rsaEncryption (for instance an empty sequence, or
INTEGER)
  * A NULL params for rsaEncryption that specifies a length other than 0 (is
this what you meant by non-minimal, or where you referring to length encodings?)
  * An rsa key with OID 2.5.8.1.1 (id-ea-rsa)

Let me know if you have other ideas for tests.

https://codereview.chromium.org/1209283004/diff/460001/net/data/verify_signed...
File
net/data/verify_signed_data_unittest/rsa-pkcs1-sha1-using-pss-key-no-params.pem
(right):

https://codereview.chromium.org/1209283004/diff/460001/net/data/verify_signed...
net/data/verify_signed_data_unittest/rsa-pkcs1-sha1-using-pss-key-no-params.pem:6:
algorithm for PKCS#1 v1.5
On 2015/07/21 16:26:27, David Benjamin wrote:
> Nit: period at the end.

Done.

https://codereview.chromium.org/1209283004/diff/460001/net/data/verify_signed...
File
net/data/verify_signed_data_unittest/rsa-pss-sha1-salt20-using-pss-key-no-params.pem
(right):

https://codereview.chromium.org/1209283004/diff/460001/net/data/verify_signed...
net/data/verify_signed_data_unittest/rsa-pss-sha1-salt20-using-pss-key-no-params.pem:3:
to rsaPss (1.2.840.113549.1.1.10)
On 2015/07/21 16:26:27, David Benjamin wrote:
> Nit: period at the end.

Done.

[davidben, 2015-07-21 19:41:14]

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.h (right):

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.h:28: //        not the BIT STRING's DER
itself.
On 2015/07/21 19:24:28, eroman wrote:
> On 2015/07/21 16:26:27, David Benjamin wrote:
> > "byte contents of that bit string" vs "BIT STRING's DER itself" is a little
> > ambiguous. I could either interpret it as:
> > 
> > - signature_value is the byte string with the 00 phase octet removed, not
the
> > contents of the DER-encoded BIT STRING, phase octet included
> > 
> > - signature_value is the byte string contents of the BIT STRING, including
the
> > phase octet, not the entire DER element.
> > 
> > Going by the code, it seems you mean the former.
> > 
> > 
> > I would write this as:
> > 
> > // IMPORTANT: In RFC 5280, signatureValue is a BIT STRING, however all
> supported
> > signature algorithms use byte strings encoded as a BIT STRING with a
> consistent
> > encoding. |signature_value| is a byte string, so the caller must decode the
> BIT
> > STRING by removing the first DER contents octet and checking the value is
> zero.
> > 
> > The specs are apparently incredibly badly-written on this point. RFC 3279,
> > section 2.2 just says "This signature value is then ASN.1 encoded as a BIT
> > STRING" and then goes on to describe three signature formats of type []byte,
> not
> > []bit. RFC 4055, section 3.2 is much more explicit about the encoding.)
> > 
> > 
> > The other option is what signature_value is actually the BIT STRING
> > representation and VerifySignedData internally strips off and checks/strips
> the
> > leading phase octet internally. (We can reject anything that's not 00. X.509
> > using BIT STRING was just plain a mistake.)
> 
> OK so thinking about this more, I think it makes the most sense to just change
> the input to a BIT STRING.
> 
> Initially I was torn on whether this API should take a BIT STRING, or require
> the caller to extract the contents. I settled on the latter mainly out of
> laziness, because much of my test data was already in that format.
> 
> But based on how it will be called, it is going to be more helpul to just take
a
> BIT STRING and do all the magic of extraction and making sure there
non-multiple
> of 8 bits etc in VerifySignedData.
> 
> Unless you object, I will start converting the test data to the new format and
> update once that is complete (and then this comment can be simplified).

SGTM. This is a pretty X.509-specific interface, so making it take the X.509
type, even though the X.509 type is completely idiotic, is probably sane. :-)

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data_unittest.cc (right):

https://codereview.chromium.org/1209283004/diff/460001/net/cert/internal/veri...
net/cert/internal/verify_signed_data_unittest.cc:221:
DISABLED_RsaPss_Sha256_Salt10_UsingPssKeyWithParams) {
On 2015/07/21 19:24:28, eroman wrote:
> On 2015/07/21 16:26:27, David Benjamin wrote:
> > Optional: Do you want to add disabled tests for 2.5.8.1.1 and bad
> > id-rsaEncryption params as well? Maybe also a non-minimally encoded
> > SubjectPublicKeyInfo.
> 
> Yes, good ideas.
> 
> I will post another patchset when this is complete, and plan to add at least:
>   * Missing params for rsaEncryption (I believe BoringSSL allows this)
>   * Non-NULL params for rsaEncryption (for instance an empty sequence, or
> INTEGER)
>   * A NULL params for rsaEncryption that specifies a length other than 0 (is
> this what you meant by non-minimal, or where you referring to length
encodings?)

I was thinking length encodings. I *think* crypto/asn1 will happily accept
those, though I haven't checked.

Honestly, most of it could be covered by "did you use a real DER parser?". Minor
nuisance is that we're switching parsers and one of them's not a real one.

(Actually there's three parsers involved now. der::Input for the outer stuff.
Then the SPKI by crypto/asn1. But the RSAPublicKey is crypto/bytestring. With
the SPKI eventually being folded into one of the other.)

>   * An rsa key with OID 2.5.8.1.1 (id-ea-rsa)
> 
> Let me know if you have other ideas for tests.

[eroman, 2015-07-22 02:10:30]

OK I have changed the signature parameter to being a BIT STRING, and updated all
the test data accordingly (dependent on
https://codereview.chromium.org/1248043002/)

I have also pretty-printed the parsed ASN.1 in the test files to simplify test
data verification.

I am still working on the additional tests we discussed...

[eroman, 2015-07-22 03:16:36]

PTAL ready for review now (I uploaded the tests we discussed).

I confirmed that using the BoringSSL’s SPKI parsing we are too permissive in the
following ways:

 * BER-encoded SPKI (specifically I tested non-minimal TLV lengths) —> 
rsa-pkcs1-sha256-key-encoded-ber.pem

 * Absent parameters for rsaEncryption are allowed —>
rsa-pkcs1-sha1-key-params-absent.pem

 * Using old OID for rsa (2.5.8.1.1) is allowed —>
rsa-pkcs1-sha256-using-id-ea-rsa.pem

 * Using bogus parameters for rsaEncryption (an integer in place of NULL) is
allowed —> rsa-pkcs1-sha256-spki-non-null-params.pem

[davidben, 2015-07-22 17:20:47]

https://codereview.chromium.org/1209283004/diff/560001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/560001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:306: if
(!parser.ReadBitStringNoUnusedBits(&signature_value))
Did you forget to include this change? I can't seem to find the definition in
the tree.

https://codereview.chromium.org/1209283004/diff/560001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.h (right):

https://codereview.chromium.org/1209283004/diff/560001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.h:24: //   |signature_value_bit_string| -
The DER-encoded BIT STRING representing the
Maybe s/BIT STRING/BIT STRING (contents|element)/ just so it's clear which one
you intended.

I don't see the ReadBitStringNoUnusedBits definition, but I'm guessing you made
it the element? Contents might be more consistent with the other
parser.h/parse_values.h splits. Otherwise the caller needs to Parser.ReadRawTLV
and we parse the length twice.

(You don't need to re-regenerate all your tests. Just parse out the element
wrapper in the test harness.)

[eroman, 2015-07-22 17:46:10]

https://codereview.chromium.org/1209283004/diff/560001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/560001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:306: if
(!parser.ReadBitStringNoUnusedBits(&signature_value))
On 2015/07/22 17:20:47, David Benjamin wrote:
> Did you forget to include this change? I can't seem to find the definition in
> the tree.

It is part of a dependent changelist -
https://codereview.chromium.org/1248043002/

Note that the Rietveld UI now links to them if you search for "Depends on
Patchset:"

(also can be found from this comment:
https://codereview.chromium.org/1209283004#msg25)

https://codereview.chromium.org/1209283004/diff/560001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.h (right):

https://codereview.chromium.org/1209283004/diff/560001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.h:24: //   |signature_value_bit_string| -
The DER-encoded BIT STRING representing the
On 2015/07/22 17:20:47, David Benjamin wrote:
> Maybe s/BIT STRING/BIT STRING (contents|element)/ just so it's clear which one
> you intended.
> 
> I don't see the ReadBitStringNoUnusedBits definition, but I'm guessing you
made
> it the element? Contents might be more consistent with the other
> parser.h/parse_values.h splits. Otherwise the caller needs to
Parser.ReadRawTLV
> and we parse the length twice.
> 
> (You don't need to re-regenerate all your tests. Just parse out the element
> wrapper in the test harness.)

Regenerating the test data for signature is not a problem, as I already have an
automated way of doing this. So if we decide on that approach I can strip the
bitstring prefix.

That said, I think it is advantageous to have the signature_value_bit_string be
the full TLV (i.e. start with 03 ) rather than its contents, and think it is the
most consistent approach.

Note that other parameters are also taking the outtermost type rather than the
contents. For instance the public_key is expected to be a sequence, and not the
contents of the sequence. Similarly the signature algorithm parsing functions
take a sequence and not the contents of that sequence.

If we wanted to take the contents of bit string I would argue we should do
similarly for these other parameters, since otherwise signature value is a bit
unique. But I would also argue that taking the sequence contents rather than the
sequence encoding for things like parsing of spki and algorithm identifiers is
worse.

Reading the full TLV is already required for the signed data (say
tbsCertificate) so I don't think it makes things any less understandable to be
doing this for signatureValue/algorithm/publicKey

[davidben, 2015-07-22 17:49:41]

lgtm

https://codereview.chromium.org/1209283004/diff/560001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.cc (right):

https://codereview.chromium.org/1209283004/diff/560001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.cc:306: if
(!parser.ReadBitStringNoUnusedBits(&signature_value))
On 2015/07/22 17:46:10, eroman wrote:
> On 2015/07/22 17:20:47, David Benjamin wrote:
> > Did you forget to include this change? I can't seem to find the definition
in
> > the tree.
> 
> It is part of a dependent changelist -
> https://codereview.chromium.org/1248043002/
> 
> Note that the Rietveld UI now links to them if you search for "Depends on
> Patchset:"
> 
> (also can be found from this comment:
> https://codereview.chromium.org/1209283004#msg25)

Oh, sorry! I missed that.

https://codereview.chromium.org/1209283004/diff/560001/net/cert/internal/veri...
File net/cert/internal/verify_signed_data.h (right):

https://codereview.chromium.org/1209283004/diff/560001/net/cert/internal/veri...
net/cert/internal/verify_signed_data.h:24: //   |signature_value_bit_string| -
The DER-encoded BIT STRING representing the
On 2015/07/22 17:46:10, eroman wrote:
> On 2015/07/22 17:20:47, David Benjamin wrote:
> > Maybe s/BIT STRING/BIT STRING (contents|element)/ just so it's clear which
one
> > you intended.
> > 
> > I don't see the ReadBitStringNoUnusedBits definition, but I'm guessing you
> made
> > it the element? Contents might be more consistent with the other
> > parser.h/parse_values.h splits. Otherwise the caller needs to
> Parser.ReadRawTLV
> > and we parse the length twice.
> > 
> > (You don't need to re-regenerate all your tests. Just parse out the element
> > wrapper in the test harness.)
> 
> Regenerating the test data for signature is not a problem, as I already have
an
> automated way of doing this. So if we decide on that approach I can strip the
> bitstring prefix.
> 
> That said, I think it is advantageous to have the signature_value_bit_string
be
> the full TLV (i.e. start with 03 ) rather than its contents, and think it is
the
> most consistent approach.
> 
> Note that other parameters are also taking the outtermost type rather than the
> contents. For instance the public_key is expected to be a sequence, and not
the
> contents of the sequence. Similarly the signature algorithm parsing functions
> take a sequence and not the contents of that sequence.
> 
> If we wanted to take the contents of bit string I would argue we should do
> similarly for these other parameters, since otherwise signature value is a bit
> unique. But I would also argue that taking the sequence contents rather than
the
> sequence encoding for things like parsing of spki and algorithm identifiers is
> worse.
> 
> Reading the full TLV is already required for the signed data (say
> tbsCertificate) so I don't think it makes things any less understandable to be
> doing this for signatureValue/algorithm/publicKey

Oh, good point about all the other inputs. Full element it is then. I guess
there's not much need to clarify the comment either since DER-encoded BIT STRING
and DER-encoded SubjectPublicKeyInfo presumably refer to the same thing.

[eroman, 2015-07-27 23:22:41]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-07-27 23:22:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1209283004/#ps640001
(title: "rebase onto master")

[commit-bot: I haz the power, 2015-07-27 23:23:48]

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

[commit-bot: I haz the power, 2015-07-27 23:52:26]

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

[commit-bot: I haz the power, 2015-07-27 23:52:26]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[eroman, 2015-07-28 00:09:55]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-07-28 00:09:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1209283004/#ps660001
(title: "appease mscvc int --> bool cast")

[commit-bot: I haz the power, 2015-07-28 00:10:38]

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

[commit-bot: I haz the power, 2015-07-28 00:25:58]

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

[commit-bot: I haz the power, 2015-07-28 00:25:59]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[eroman, 2015-07-28 02:35:26]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2015-07-28 02:35:56]

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

[commit-bot: I haz the power, 2015-07-28 02:40:52]

Committed patchset #34 (id:660001)

[commit-bot: I haz the power, 2015-07-28 02:41:51]

Patchset 34 (id:??) landed as
https://crrev.com/2b124dbcd72c180b2865dc0448e280d2e3e7cbe4
Cr-Commit-Position: refs/heads/master@{#340633}