Refuse to accept certificate chains containing any RSA public key smaller

Reject certificate chains containing small RSA and DSA keys.

"Small" means less than 1024 bits.

BUG=102949
TEST=net_unittests, X509CertificateTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114709

[palmer, 2011-11-16 03:30:45]

This is NOT intended to be commitable code yet — for now, I am just getting the
code off my machine and into a place where rsleevi can see how poorly I
interpreted his instructions. (He gets all credit for any non-broken code this
CL might happen to have.)

I still need to actually invoke the new GetPublicKeyInfo function, write the
unit tests, and do functional tests. Hope to do that tomorrow.

Thanks to rsleevi for copious clues!

[Ryan Sleevi, 2011-11-16 03:46:11]

Looking good so far. The nit brigade, as you develop.

I'm also wondering whether the API should be NOTREACHED()ing? If we only call
this on OSCertHandles that have been verified/vetted by the OS, it's fine, but
if the API is meant to allow any OSCertHandle, then perhaps it should be more
forgiving of the possibility that there may be signatures/OIDs that the
underlying API doesn't know about.

If so, we probably don't want those to CHECK, since an attacker may be able to
trivially supply such a certificate to end users.

Like I said, it's not really a concern if it's only cert handles that have been
vetted, since that implies the OS can parse and understand all of the signature
types. Just something to think about.

http://codereview.chromium.org/8568040/diff/1/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/8568040/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:78: NONE,
nit: UNKNOWN?

If it causes a name collision, it might be time to revisit the naming style for
these enums, as per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Enumerator_Names

http://codereview.chromium.org/8568040/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:83: } PublicKeyType;
I believe you mean

enum PublicKeyType { }. Here, you're declaring an unnamed enum that is a member
of X509Certificate.

http://codereview.chromium.org/8568040/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:416: static unsigned int
GetPublicKeyLengthInBits(OSCertHandle cert_handle);
Sig here doesn't match your new functions.

http://codereview.chromium.org/8568040/diff/1/net/base/x509_certificate_mac.cc
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/8568040/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:1444: SecKeyRef key;
base::mac::ScopedCFTypeRef<SecKeyRef> works handy here.

The only thing is you can't pass a pointer to it for the CopyPublicKey, so
something like

SecKeyRef key;
OSStatus status = SecCertificateCopyPublicKey(cert_handle, &key);
if (status != noErr) {
  NOTREACHED() ...
  return 0;
}
base::mac::ScopedCFType<SecKeyRef> scoped_key(key);

http://codereview.chromium.org/8568040/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:1461: status =
CSSM_QueryKeySizeInBits(crypto::GetSharedCSPHandle(), NULL, cssm_key,
CSSM_KEY.Header has a LogicalKeySizeInBits member. I think that may save you the
call (and implicit lock) into CSSM land. I believe it should be said for all of
Apple's SecKeyRefs.

http://codereview.chromium.org/8568040/diff/1/net/base/x509_certificate_win.cc
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/8568040/diff/1/net/base/x509_certificate_win.c...
net/base/x509_certificate_win.cc:1168: CHECK(CRYPT_OID_INFO.dwGroupId ==
CRYPT_SIGN_ALG_OID_GROUP_ID);
nit: oid_info->dwGroupId (and the next two lines)

[Ryan Sleevi, 2011-11-16 03:47:03]

http://codereview.chromium.org/8568040/diff/1/net/base/x509_certificate_mac.cc
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/8568040/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:1461: status =
CSSM_QueryKeySizeInBits(crypto::GetSharedCSPHandle(), NULL, cssm_key,
On 2011/11/16 03:46:11, Ryan Sleevi wrote:
> CSSM_KEY.Header has a LogicalKeySizeInBits member. I think that may save you
the
> call (and implicit lock) into CSSM land. I believe it should be said for all
of
> Apple's SecKeyRefs.

s/said/set/. Makes more sense now.

[agl, 2011-11-16 15:57:55]

http://codereview.chromium.org/8568040/diff/8/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/8568040/diff/8/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:82: DH,
OTHER?

(For example, ECDH. I wouldn't want code to assume that these are the only valid
key types. It's the sort of thing that might change in the future.)


Later: I see that you're using NONE for OTHER. I think NONE is a misleading
name.

http://codereview.chromium.org/8568040/diff/8/net/base/x509_certificate_mac.cc
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/8568040/diff/8/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:1440: //static
space before "static"

http://codereview.chromium.org/8568040/diff/8/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:1449: return 0;
you're returning 0 in a void function (ditto elsewhere).

http://codereview.chromium.org/8568040/diff/8/net/base/x509_certificate_nss.cc
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/8568040/diff/8/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:1048: //static
space before "static"

http://codereview.chromium.org/8568040/diff/8/net/base/x509_certificate_opens...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/8568040/diff/8/net/base/x509_certificate_opens...
net/base/x509_certificate_openssl.cc:622: //static
space before "static"

http://codereview.chromium.org/8568040/diff/8/net/base/x509_certificate_win.cc
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/8568040/diff/8/net/base/x509_certificate_win.c...
net/base/x509_certificate_win.cc:1156: //static
space

http://codereview.chromium.org/8568040/diff/8/net/base/x509_certificate_win.c...
net/base/x509_certificate_win.cc:1183: *type = PublicKeyType::None;
Capitalisation.

[palmer, 2011-11-16 23:02:41]

> I'm also wondering whether the API should be NOTREACHED()ing?

Only _mac.cc does, and other functions in that module invoke NOTREACHED in
similar situations. So if it's the already-accepted behavior, this new code
should follow along. If we want to change that, we should change it throughout,
in a new CL.

> net/base/x509_certificate.h:78: NONE,
> nit: UNKNOWN?

agl didn't like NONE either, and I agree it does not make sense. It is now
Unknown. (Used the kPublicKeyType* naming convention.)

> GetPublicKeyLengthInBits(OSCertHandle cert_handle);
> Sig here doesn't match your new functions.

Fixed.

> net/base/x509_certificate_mac.cc:1444: SecKeyRef key;
> base::mac::ScopedCFTypeRef<SecKeyRef> works handy here.
> 
> The only thing is you can't pass a pointer to it for the CopyPublicKey, so
> something like
> 
> SecKeyRef key;
> OSStatus status = SecCertificateCopyPublicKey(cert_handle, &key);
> if (status != noErr) {
>   NOTREACHED() ...
>   return 0;
> }
> base::mac::ScopedCFType<SecKeyRef> scoped_key(key);

I get the idea of using a smart pointer, but if I can't pass it to
SecCertificateCopyPublicKey, I don't understand your suggested new code. I'd
love to get rid of those explicit CFReleases.

> CSSM_QueryKeySizeInBits(crypto::GetSharedCSPHandle(), NULL, cssm_key,
> CSSM_KEY.Header has a LogicalKeySizeInBits member. I think that may save you
the
> call (and implicit lock) into CSSM land. I believe it should be said for all
of
> Apple's SecKeyRefs.

Ok, trying that.

> net/base/x509_certificate_win.cc:1168: CHECK(CRYPT_OID_INFO.dwGroupId ==
> CRYPT_SIGN_ALG_OID_GROUP_ID);
> nit: oid_info->dwGroupId (and the next two lines)

Fixed.

NOTE: Unit tests added, new certificates to drive the tests, new
CERT_STATUS_WEAK_KEY/CERT_WEAK_KEY. Also, I now actually invoke the code in
X509Certificate::Verify, including iterating over the chain of intermediate
signers.

[palmer, 2011-11-16 23:06:35]

> net/base/x509_certificate.h:82: DH,
> OTHER?

Yep, it's now kPublicKeyTypeUnknown instead of NONE, and added
kPublicKeyTypeECDH and kPublicKeyTypeDH. The platform-specific implementations
do not check for all types right now. I think that is ok for now since the code
in Verify only cares at all if the type is kPublicKeyTypeRSA.

> (For example, ECDH. I wouldn't want code to assume that these are the only
valid
> key types. It's the sort of thing that might change in the future.)

Right.

> space before "static"

Fixed everywhere. Also fixed some other gcl lints.

> you're returning 0 in a void function (ditto elsewhere).

Fixed.

[Ryan Sleevi, 2011-11-16 23:40:53]

I feel like the unit tests may be missing a bit of coverage, which may lead to
unexpected surprises. It certainly was the case when implementing the
md2/md4/md5 code that the variety of certificates helped shake out a number of
edge cases while implementing.

If you want, I can pass along the scripts I used to generate the signer alg
variants, which you should may be able to adapt.

Permutations that I see wanting to test:
  - Variations of chain path building. Possible considerations:
    * Server supplies an EE & (Strong/Weak) intermediate, no root known
    * Server supplies an EE & (Strong/Weak) intermediate, Root is 'faked' via
TestRootCerts (but not in the chain sent to Verify())
    * Server supplies a (Strong/Weak) EE, missing intermediate
  - Variations of key types to make sure the code handles (expected/unexpected)
results correctly
    * DSA / ECC keys (ECC isn't available on XP - but the behaviour should match
'unknown'/'unsupported' then)
    * Some arbitrary OID type (Can provide some examples of openssl.cnf using
ASN1_conf notation if you need)

Further, if the chain verification returns a certificate error, does this added
weak check make sense, for anything but the server cert? Obviously the EE cert
protects the TLS channel, but in the case of an error, interstitialed or not,
every other cert in the chain is effectively meaningless, right? So why check?
What is the risk/threat-model there?

(That is, I think we should only do these checks on the chain for chains that
are valid, otherwise only on the EE cert).

Also, I believe you want to examine and possibly modify the following files:
  - content/browser/ssl/ssl_policy.cc - determines which errors are fatal, which
are interstitialable
  - chrome/browser/ssl/ssl_error_info.cc - Determines how to present localizable
errors to the end user explaining what the errors mean
  - chrome/app/generated_resources.grd - The localized error messages to
present.

http://codereview.chromium.org/8568040/diff/4003/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/8568040/diff/4003/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:607: 
I don't think this is the correct place to do it, for the same reason I didn't
put the MD2/MD4/MD5 checking here.

First reason is that this only iterates the server supplied chain. Except for OS
X, none of the other implementations care that much about the rest of the server
supplied certs being related.

Second reason is that, in the event of weak certs (particularly intermediaries),
it's possible they'll be replaced (or introduced) during AIA fetching. So this
can have both false negatives and false positives.

I think this should be moved to after VerifyInternal, and operate on
|verified_result->verified_cert|

Also, the same concerns raised re: md2/md4/md5 masking more serious errors may
apply here (if this is interstitialed) or may not.

http://codereview.chromium.org/8568040/diff/4003/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/8568040/diff/4003/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:1448: NOTREACHED() <<
"SecCertificateCopyPublicKey failed: " << status;
The equivalent here in the _win code is a CHECK, not a NOTREACHED. Although I
dislike CHECKs on such attacker-supplied blobs, such as what certificates are.

http://codereview.chromium.org/8568040/diff/4003/net/base/x509_certificate_op...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/8568040/diff/4003/net/base/x509_certificate_op...
net/base/x509_certificate_openssl.cc:627: *size_bits = EVP_PKEY_size(key) * 8;
CHECK(key) ?

http://codereview.chromium.org/8568040/diff/4003/net/base/x509_certificate_un...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/8568040/diff/4003/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:7: #endif
This was in the correct place originally. Please move it back.

http://www.chromium.org/developers/coding-style#TOC-Platform-defines

http://codereview.chromium.org/8568040/diff/4005/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/8568040/diff/4005/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:595: if (type == kPublicKeyTypeRSA && size_bits <
1023) {
As AGL noted over on the Mozilla bug, the '1023' is likely an artifact of
OpenSSL reporting, not the actual key size.

We should be able to use 1024 here.

http://codereview.chromium.org/8568040/diff/4005/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:597: return
MapCertStatusToNetError(verify_result->cert_status);
Rather than duplicating this logic with the lines on 603/604, just use a bool
and have a single exit point from this function for week keys?

http://codereview.chromium.org/8568040/diff/4005/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:601: GetPublicKeyInfo(*i, &size_bits, &type);
bug: If GetPublicKeyInfo fails, |type| is not mutated, but |size_bits| is. This
will cause unknown types to be rejected as weak keys. Intentional? Should this
API return a bool?

It's less of a concern if moved to after verification.

http://codereview.chromium.org/8568040/diff/4005/net/base/x509_certificate_wi...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/8568040/diff/4005/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:1169: CHECK(iod_info->ExtraInfo.cbData >=
sizeof(DWORD));
Do we want to crash on unrecognized extensions? oid_info may be NULL if Windows
doesn't recognize the OID.

Should probably short-circuit here (without a check) if not oid_info, and only
check the rest (which in poking around in IDA, CryptoAPI is equally paranoid
about the results being valid)

http://codereview.chromium.org/8568040/diff/4005/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:1182: default:
case CALG_ECDH:
  *type = kPublicKeyTypeECDH;
  break;

[Ryan Sleevi, 2011-11-16 23:44:57]

> > SecKeyRef key;
> > OSStatus status = SecCertificateCopyPublicKey(cert_handle, &key);
> > if (status != noErr) {
> >   NOTREACHED() ...
> >   return 0;
> > }
> > base::mac::ScopedCFType<SecKeyRef> scoped_key(key);
> 
> I get the idea of using a smart pointer, but if I can't pass it to
> SecCertificateCopyPublicKey, I don't understand your suggested new code. I'd
> love to get rid of those explicit CFReleases.
> 

Ack, no inline replies on the CL.

See X509Certificate::CopyCertChain in x509_certificate_mac to see an example of
the wrapping. (Or AddCertificatesFromBytes, or VerifyInternal's
scoped_trust_ref)

[palmer, 2011-11-17 01:18:03]

> If you want, I can pass along the scripts I used to generate the signer alg
> variants, which you should may be able to adapt.

That'd be great, thanks! I'll happily test all the permutations you mention.

> Further, if the chain verification returns a certificate error, does this
added
> weak check make sense, for anything but the server cert?

Possibly, because this is (intended to be) a hard-fail check, so more severe
than some other errors.

> (That is, I think we should only do these checks on the chain for chains that
> are valid, otherwise only on the EE cert).

Maybe only if ! IsSeriousError, you mean? Sure, I could see doing it that way.
What I just uploaded doesn't have that yet.

> Also, I believe you want to examine and possibly modify the following files:
>   - content/browser/ssl/ssl_policy.cc - determines which errors are fatal,
which
> are interstitialable
>   - chrome/browser/ssl/ssl_error_info.cc - Determines how to present
localizable
> errors to the end user explaining what the errors mean
>   - chrome/app/generated_resources.grd - The localized error messages to
> present.

Will do tomorrow — thanks.

> I think this should be moved to after VerifyInternal, and operate on
> |verified_result->verified_cert|

Done.

> Also, the same concerns raised re: md2/md4/md5 masking more serious errors may
> apply here (if this is interstitialed) or may not.

I intend for this to be a fatal error, so I think we're ok on that score.

> net/base/x509_certificate_openssl.cc:627: *size_bits = EVP_PKEY_size(key) * 8;
> CHECK(key) ?

Done.

> net/base/x509_certificate_unittest.cc:7: #endif
> This was in the correct place originally. Please move it back.

gcl lint disagreed, and it compiled and ran when moved to the lint-approved
place, but ok.

> We should be able to use 1024 here.

Done.

> MapCertStatusToNetError(verify_result->cert_status);
> Rather than duplicating this logic with the lines on 603/604, just use a bool
> and have a single exit point from this function for week keys?

Done.

> net/base/x509_certificate.cc:601: GetPublicKeyInfo(*i, &size_bits, &type);
> bug: If GetPublicKeyInfo fails, |type| is not mutated, but |size_bits| is.
This
> will cause unknown types to be rejected as weak keys. Intentional? Should this
> API return a bool?
> 
> It's less of a concern if moved to after verification.

Moved to after verification, and I changed the platform-specific impls to ensure
that they always either CHECKs/NOTREACHEDs or mutates both values.

> net/base/x509_certificate_win.cc:1169: CHECK(iod_info->ExtraInfo.cbData >=
> sizeof(DWORD));
> Do we want to crash on unrecognized extensions? oid_info may be NULL if
Windows
> doesn't recognize the OID.
> 
> Should probably short-circuit here (without a check) if not oid_info, and only
> check the rest (which in poking around in IDA, CryptoAPI is equally paranoid
> about the results being valid)

I added a CHECK(oid_info). That should be maximally solid?

> net/base/x509_certificate_win.cc:1182: default:
> case CALG_ECDH:
>   *type = kPublicKeyTypeECDH;
>   break;

Done.

[Ryan Sleevi, 2011-11-17 01:50:10]

On 2011/11/17 01:18:03, Chris P. wrote:
> > If you want, I can pass along the scripts I used to generate the signer alg
> > variants, which you should may be able to adapt.
> 
> That'd be great, thanks! I'll happily test all the permutations you mention.

Sent via e-mail to your @google

> 
> > net/base/x509_certificate_unittest.cc:7: #endif
> > This was in the correct place originally. Please move it back.
> 
> gcl lint disagreed, and it compiled and ran when moved to the lint-approved
> place, but ok.

Yeah, this is one area where gcl lint can fail. This has been raised on
chromium-dev in the past though, and the consensus has remained "after all other
headers" - gcl lint be damned :-)


> I added a CHECK(oid_info). That should be maximally solid?

Only if you really do want to crash whenever an unknown SPKI is encountered. If
you're only inspecting certs w/ NO errors, this is fine, but if you're getting
into minor/major error territory, you probably want to be more careful. After
all, an unknown SPKI for any cert but the server cert should just be
seen/treated as untrusted (not weak, not crash) - right?

If not, yes, its fine.

[wtc, 2011-11-17 02:52:18]

Patch Set 6 LGTM.

palmer: I am giving you an owner's LGTM in advance because I
am travelling in a different time zone this week.  Please
do not commit this CL until rsleevi and agl are happy with it.

I noticed a lack of error checking in the implementations of
the GetPublicKeyInfo method.

All of my other comments below are about nits and comment
problems.

http://codereview.chromium.org/8568040/diff/13006/net/base/cert_status_flags.cc
File net/base/cert_status_flags.cc (right):

http://codereview.chromium.org/8568040/diff/13006/net/base/cert_status_flags....
net/base/cert_status_flags.cc:45: return CERT_STATUS_WEAK_KEY;

Nit: since the ordering of the cases doesn't matter, please
add this case after the ERR_CERT_NOT_IN_DNS case below to
match the declaration order in cert_status_flags.h.

http://codereview.chromium.org/8568040/diff/13006/net/base/net_error_list.h
File net/base/net_error_list.h (right):

http://codereview.chromium.org/8568040/diff/13006/net/base/net_error_list.h#n...
net/base/net_error_list.h:376: // The server responded with a certificate that
contains a weak key (e.g.

Nit: Shorten this to "The server's certificate contains a
weak key ...".

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate.c...
net/base/x509_certificate.cc:601: weak_key = true;

It seems that a IsWeakPublicKey/ContainsWeakPublicKey function
may be more appropriate than the GetPublicKeyInfo function.
Also, it may be a good idea to create a method for the code
on lines 593-610.

Please define a constant for 1024.  Your CL's commit message
says "less than 1023 bits".  Should we use 1023 instead?

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate.h...
net/base/x509_certificate.h:416: // Returns the length of the public key in
bits.

Document that the public key type is also returned.

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate_m...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate_m...
net/base/x509_certificate_mac.cc:1448: return;

Since this function returns void, it should always store
some values in the output arguments.

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate_m...
net/base/x509_certificate_mac.cc:1475: *type = kPublicKeyTypeUnknown;

Nit: add a break statement.

Make the same change to other implementations of the
X509Certificate::GetPublicKeyInfo method.

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate_o...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate_o...
net/base/x509_certificate_openssl.cc:628: *size_bits = EVP_PKEY_size(key) * 8;

Is there an OpenSSL function that returns the key size in
bits?  Some EC key sizes are not a multiple of 8 (e.g., 521).

http://codereview.chromium.org/8568040/diff/13006/net/data/ssl/certificates/s...
File net/data/ssl/certificates/strong-key-weak-signer.pem (right):

http://codereview.chromium.org/8568040/diff/13006/net/data/ssl/certificates/s...
net/data/ssl/certificates/strong-key-weak-signer.pem:1: -----BEGIN
CERTIFICATE-----

Nit: The name of this file "strong key weak signer" is not
clear to me.  Perhaps adding "with" would make it clearer...

Please document the new test certificates in
net/data/ssl/certificates/README.

[Ryan Sleevi, 2011-11-17 03:20:33]

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate_m...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate_m...
net/base/x509_certificate_mac.cc:1450: ScopedCFTypeRef<SecKeyRef> scoped_key;
ScopedCFTypeRef<SecKeyRef> scoped_key(key);

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate_o...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate_o...
net/base/x509_certificate_openssl.cc:628: *size_bits = EVP_PKEY_size(key) * 8;
On 2011/11/17 02:52:18, wtc wrote:
> 
> Is there an OpenSSL function that returns the key size in
> bits?  Some EC key sizes are not a multiple of 8 (e.g., 521).

That was on me.

EVP_PKEY_bits, but then it gets into the weird 1023/1024 bit size IIRC. For
DSA/EC/RSA/DH, it's just a wrapper for BN_num_bits.

EVP_PKEY_size does the right thing for RSA, but for DSA/ECDH returns
[EC]DSA_size, which is the maximum length of an [EC]DSA signature created with
that key.

So there's no good solution that solves both cases.

FWIW, the OpenSSL code (command-line, etc) in 0.9.8 just uses a series of ifs
switched off pkey->type, so perhaps both *size_bits and *type should be handled
in the switch.

[Ryan Sleevi, 2011-12-13 05:45:34]

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate.c...
net/base/x509_certificate.cc:579: static bool
IsWeakKey(X509Certificate::PublicKeyType type, size_t size_bits) {
nit: Given that this file makes use of an unnamed namespace, should this be
moved up to around line 227 and de-static'd?

This is something I'm not sure of - the C++ Guide has this to say on the matter,
but it's unclear about "mix and match"

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...

"If you must define a nonmember function and it is only needed in its .cc file,
use an unnamed namespace or static linkage (eg static int Foo() {...}) to limit
its scope."

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate.h...
net/base/x509_certificate.h:415: // Returns the length of the public key in
bits.
nit: This comment doesn't really return anything. Perhaps a slight text update
is needed?

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate_u...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:616: static bool is_weak_key_type(const
std::string& key_type) {
nit: Function naming guidelines are
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names

IsWeakKeyType()

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:621: return 0 == size.compare("768") &&
nit: Why use .compare() == 0 instead of the more common 

return size == "768" && (type == "rsa" || type == "dsa")

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:631: //     key_types="768-rsa 1024-rsa
2048-rsa secp256k1-ecdsa"
typo? You refer to secp256k1-ecdsa in the comment, but then in the code refer to
it as prime256v1-ecdsa (line 637)

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:649: signer_type != key_types.end();
++signer_type) {
nit: This sort of iteration within a test, while not forbidden, can be replaced
by parameterized tests, which are consistent with the rest of the file.

One of the benefits for parameterized tests is you can track test
successes/failures a bit more independently (as opposed to one giant test
pass/fail). Another is that you don't have to worry about test timeouts (which
probably aren't much of a concern here).

It's a suggestion - It just struck me because you're wanting to test 16
different test permutations with only a single test.

const char* const kWeakKeyCertTypes[] = {
  "768-rsa",
  "1024-rsa",
  "2048-rsa",
  "prime256v1-ecdsa",
};

class X509CertificateWeakKeyTest :
  public ::testing::TestWithParam<std::tr1::tuple<const char*, const char*> > {
  virtual void SetUp() OVERRIDE {
    const ParamType& param = GetParam();
    end_entity_type_ = std::tr1::get<0>(param);
    signer_type_ = std::tr1::get<1>(param);
  }
  
  std::string end_entity_type_;
  std::string signer_type_;
};

TEST_P(X509CertificateWeakKeyTest, IsRejected) {
   ...
  // Remove the two for loops, replace *ee_type with ee_type_ and
  // *signer_type with signer_type_
  // You could also move more of the X509Certificate initialization into
SetUp(),
  // so that you just have to do cert_chain_->Verify(...) in this test. Up to
you
  // if you do decide to use this approach.
}

INSTANTIATE_TEST_P(, X509CertificateWeakKeyTest,
                   ::testing::Combine(
                       ::testing::ValuesIn(kWeakCertTypes),
                       ::testing::ValuesIn(kWeakCertTypes)))

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate_w...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate_w...
net/base/x509_certificate_win.cc:1187: CRYPT_SIGN_ALG_OID_GROUP_ID);
CRYPT_PUBKEY_ALG_OID_GROUP_ID

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate_w...
net/base/x509_certificate_win.cc:1189: CHECK(oid_info->dwGroupId ==
CRYPT_SIGN_ALG_OID_GROUP_ID);
CHECK_EQ(oid_info->dwGroupId, CRYPT_PUBKEY_ALG_OID_GROUP_ID)

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate_w...
net/base/x509_certificate_win.cc:1190: CHECK(oid_info->ExtraInfo.cbData >=
sizeof(DWORD));
Delete this CHECK (ExtraInfo.cbData may be 0 if no flags)

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate_w...
net/base/x509_certificate_win.cc:1191: DWORD id =
*reinterpret_cast<DWORD*>(oid_info->ExtraInfo.pbData);
Delete this (updated below at line 1197)

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate_w...
net/base/x509_certificate_win.cc:1197: switch (id) {
switch (id) -> switch (oid_info->AlgId) {

http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate_w...
net/base/x509_certificate_win.cc:1198: case CALG_RSA_SIGN:
add
case CALG_RSA_KEYX:

[palmer, 2011-12-13 18:55:44]

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate_o...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/8568040/diff/13006/net/base/x509_certificate_o...
net/base/x509_certificate_openssl.cc:628: *size_bits = EVP_PKEY_size(key) * 8;
On 2011/11/17 03:20:33, Ryan Sleevi wrote:
> On 2011/11/17 02:52:18, wtc wrote:
> > 
> > Is there an OpenSSL function that returns the key size in
> > bits?  Some EC key sizes are not a multiple of 8 (e.g., 521).
> 
> That was on me.
> 
> EVP_PKEY_bits, but then it gets into the weird 1023/1024 bit size IIRC. For
> DSA/EC/RSA/DH, it's just a wrapper for BN_num_bits.
> 
> EVP_PKEY_size does the right thing for RSA, but for DSA/ECDH returns
> [EC]DSA_size, which is the maximum length of an [EC]DSA signature created with
> that key.
> 
> So there's no good solution that solves both cases.
> 
> FWIW, the OpenSSL code (command-line, etc) in 0.9.8 just uses a series of ifs
> switched off pkey->type, so perhaps both *size_bits and *type should be
handled
> in the switch.

Done.

[palmer, 2011-12-13 19:45:38]

> net/base/x509_certificate.cc:579: static bool
> IsWeakKey(X509Certificate::PublicKeyType type, size_t size_bits) {
> nit: Given that this file makes use of an unnamed namespace, should this be
> moved up to around line 227 and de-static'd?

Yeah, I think so. Done. The C++ Guide is ambiguous, but I think it's best to
stick to The C++ Way when doing C++.

> net/base/x509_certificate.h:415: // Returns the length of the public key in
> bits.
> nit: This comment doesn't really return anything. Perhaps a slight text update
> is needed?

Done.

> net/base/x509_certificate_unittest.cc:616: static bool is_weak_key_type(const
> std::string& key_type) {
> nit: Function naming guidelines are
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names
> 
> IsWeakKeyType()

Done.

> net/base/x509_certificate_unittest.cc:621: return 0 == size.compare("768") &&
> nit: Why use .compare() == 0 instead of the more common 
> 
> return size == "768" && (type == "rsa" || type == "dsa")

Done.

> net/base/x509_certificate_unittest.cc:631: //     key_types="768-rsa 1024-rsa
> 2048-rsa secp256k1-ecdsa"
> typo? You refer to secp256k1-ecdsa in the comment, but then in the code refer
to
> it as prime256v1-ecdsa (line 637)

They are different curves. I used to be using secp256k1 but then I found that
prime256v1 actually works, so started using that instead. Updated the comment to
reflect reality.

> net/base/x509_certificate_unittest.cc:649: signer_type != key_types.end();
> ++signer_type) {
> nit: This sort of iteration within a test, while not forbidden, can be
replaced
> by parameterized tests, which are consistent with the rest of the file.

It matches the generate-weak-test-chains.sh script, which is important. When we
add new key types or remove old ones, we can update just to programs that share
the same "generate all permutations with nested loops" structure. So since it's
not forbidden I think we should leave it like this.

> One of the benefits for parameterized tests is you can track test
> successes/failures a bit more independently (as opposed to one giant test
> pass/fail). Another is that you don't have to worry about test timeouts (which
> probably aren't much of a concern here).

To me it's conceptually one big pass/fail — either the new weak keys check works
for all key types and sizes, or it has a bug and thinks that small EC keys are
"weak".

>
http://codereview.chromium.org/8568040/diff/16001/net/base/x509_certificate_w...

Done, as discussed in chat.

[wtc, 2011-12-13 21:56:17]

Patch Set 10 LGTM.  I have some suggested changes below.
Please also make sure rsleevi is happy with the CL before
you check this in.

I didn't inspect the new test certificate files and scripts.

You should update the CL's commit message to mention DSA.

http://codereview.chromium.org/8568040/diff/26005/net/base/cert_status_flags.cc
File net/base/cert_status_flags.cc (right):

http://codereview.chromium.org/8568040/diff/26005/net/base/cert_status_flags....
net/base/cert_status_flags.cc:55: // serious error.

As this comment note, the order of the errors in this function
is important, from more serious to less serious.

So CERT_STATUS_WEAK_KEY should be listed next to
CERT_STATUS_WEAK_SIGNATURE_ALGORITHM because they seem to have
comparable severity to me.

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate.c...
net/base/x509_certificate.cc:230: type == X509Certificate::kPublicKeyTypeDSA);

To allow easy future expansion, please implement this function
with a switch statement:

  switch (type) {
    case X509Certificate::kPublicKeyTypeRSA:
    case X509Certificate::kPublicKeyTypeDSA:
      return size_bits < 1024;
    default:
      // False negative.
      return false;
  }
}

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate.c...
net/base/x509_certificate.cc:621: return
MapCertStatusToNetError(verify_result->cert_status);

IMPORTANT: I think we should fall through to check for blacklisted
public keys, which are a more serious violation.

Alternatively, we should check for blacklisted public keys
first.

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate.h...
net/base/x509_certificate.h:83: kPublicKeyTypeECDH

It's possible that we just need a kPublicKeyTypeEC public key
type, which corresponds to the id-ecPublicKey OID.  Or
perhaps we should rename kPublicKeyTypeECDSA to
kPublicKeyTypeEC and keep kPublicKeyTypeECDH, which
corresponds to the id-ecDH OID.  See RFC 5480 section 2.1.

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate.h...
net/base/x509_certificate.h:436: // |kPublicKeyTypeUnknown|, |*size_bits| will
be set to 0.

It seems a shame to through away the known key size for an
unsupported key type.  Perhaps it is enough to just document
that "A |*size_bits| of 0 means the key size is unknown."

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_m...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_m...
net/base/x509_certificate_mac.cc:1525: return;

Since the function does not return a success/failure status,
it should always set the output arguments before returning.

This can be accomplished by initializing
  *size_bits = 0;
  *type = kPublicKeyUnknown;
at the beginning of the function.

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_m...
net/base/x509_certificate_mac.cc:1553: *size_bits = 0;

Nit: add a "break" statement to the default case.  This is
my style preference.  (It is required in Java.)

Make the same change to the other implementations of this
function.

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_o...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_o...
net/base/x509_certificate_openssl.cc:683: *size_bits = EVP_PKEY_size(key);

It seems strange that EVP_PKEY_size returns a key size in
either bytes or bits depending on the key type.

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_u...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:614: // Currently, only RSA and DSA are
checked for weakness, and our example weak

Nit: add "keys " after "RSA and DSA".

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:621: return size == "768" && (type ==
"rsa" || type == "dsa");
Nit: for easy of future expansion, it may be better to simulate
a switch statement here:
  if (type == "rsa" || type == "dsa")
    return size == "768";
  // For other key types, return a false negative.
  return false;

http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:657: DLOG(WARNING) << "adding " <<
basename << " to the chain";

Fix the indentation of this line.

http://codereview.chromium.org/8568040/diff/26005/net/data/ssl/certificates/1...
File net/data/ssl/certificates/1024-rsa-ee-by-1024-rsa-intermediate.pem (right):

http://codereview.chromium.org/8568040/diff/26005/net/data/ssl/certificates/1...
net/data/ssl/certificates/1024-rsa-ee-by-1024-rsa-intermediate.pem:27: IP
Address:127.0.0.1

Please document these new test certificates in the
net/data/ssl/certificates/README file.

Ideally these test certificates should have a dNSName of
www.example.com instead of an iPAddress of 127.0.0.1 in
the subject alternative name.

Since you have already generated these test certificates,
you can ignore this suggestion.

[palmer, 2011-12-13 23:23:20]

> You should update the CL's commit message to mention DSA.

Done.

>
http://codereview.chromium.org/8568040/diff/26005/net/base/cert_status_flags....
> net/base/cert_status_flags.cc:55: // serious error.
> 
> As this comment note, the order of the errors in this function
> is important, from more serious to less serious.
> 
> So CERT_STATUS_WEAK_KEY should be listed next to
> CERT_STATUS_WEAK_SIGNATURE_ALGORITHM because they seem to have
> comparable severity to me.

Done.

>
http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate.c...
> net/base/x509_certificate.cc:230: type == X509Certificate::kPublicKeyTypeDSA);
> 
> To allow easy future expansion, please implement this function
> with a switch statement:
> 
>   switch (type) {
>     case X509Certificate::kPublicKeyTypeRSA:
>     case X509Certificate::kPublicKeyTypeDSA:
>       return size_bits < 1024;
>     default:
>       // False negative.
>       return false;
>   }
> }

Done.

> net/base/x509_certificate.cc:621: return
> MapCertStatusToNetError(verify_result->cert_status);
> 
> IMPORTANT: I think we should fall through to check for blacklisted
> public keys, which are a more serious violation.
> 
> Alternatively, we should check for blacklisted public keys
> first.

I moved the blacklisted key check first.

>
http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate.h...
> net/base/x509_certificate.h:83: kPublicKeyTypeECDH
> 
> It's possible that we just need a kPublicKeyTypeEC public key
> type, which corresponds to the id-ecPublicKey OID.  Or
> perhaps we should rename kPublicKeyTypeECDSA to
> kPublicKeyTypeEC and keep kPublicKeyTypeECDH, which
> corresponds to the id-ecDH OID.  See RFC 5480 section 2.1.

Well, I was modelling the names after what the various platform libraries make
available to us. net/base/x509_certificate_win.cc has CALG_ECDSA and CALG_ECDH;
_nss.cc has no DH EC type but does have ECDSA, OpenSSL is the same as NSS, Mac
is the same as NSS. I think it's pretty confusing either way; but also I don't
imagine id-ecMQV will start taking the world by storm any time soon (as good as
it might very well be...).

>
http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate.h...
> net/base/x509_certificate.h:436: // |kPublicKeyTypeUnknown|, |*size_bits| will
> be set to 0.
> 
> It seems a shame to through away the known key size for an
> unsupported key type.  Perhaps it is enough to just document
> that "A |*size_bits| of 0 means the key size is unknown."

Well, for an unsupported key type, we can't possibly continue anyway, and I
wanted to be able to set the type and size_bits to well-defined values in all
cases.

>
http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_m...
> File net/base/x509_certificate_mac.cc (right):
> 
>
http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_m...
> net/base/x509_certificate_mac.cc:1525: return;
> 
> Since the function does not return a success/failure status,
> it should always set the output arguments before returning.
> 
> This can be accomplished by initializing
>   *size_bits = 0;
>   *type = kPublicKeyUnknown;
> at the beginning of the function.

Done.

>
http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_m...
> net/base/x509_certificate_mac.cc:1553: *size_bits = 0;
> 
> Nit: add a "break" statement to the default case.  This is
> my style preference.  (It is required in Java.)
> 
> Make the same change to the other implementations of this
> function.

Done.

>
http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_o...
> net/base/x509_certificate_openssl.cc:683: *size_bits = EVP_PKEY_size(key);
> 
> It seems strange that EVP_PKEY_size returns a key size in
> either bytes or bits depending on the key type.

Yes, rsleevi alerted me to this weirdness.

>
http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_u...
> net/base/x509_certificate_unittest.cc:614: // Currently, only RSA and DSA are
> checked for weakness, and our example weak
> 
> Nit: add "keys " after "RSA and DSA".

Done.

> 
>
http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_u...
> net/base/x509_certificate_unittest.cc:621: return size == "768" && (type ==
> "rsa" || type == "dsa");
> Nit: for easy of future expansion, it may be better to simulate
> a switch statement here:
>   if (type == "rsa" || type == "dsa")
>     return size == "768";
>   // For other key types, return a false negative.
>   return false;

Done.

>
http://codereview.chromium.org/8568040/diff/26005/net/base/x509_certificate_u...
> net/base/x509_certificate_unittest.cc:657: DLOG(WARNING) << "adding " <<
> basename << " to the chain";
> 
> Fix the indentation of this line.

Actually, this and the other DLOG line above it didn't need to be there in the
final code, so I removed them.

>
http://codereview.chromium.org/8568040/diff/26005/net/data/ssl/certificates/1...
> net/data/ssl/certificates/1024-rsa-ee-by-1024-rsa-intermediate.pem:27: IP
> Address:127.0.0.1
> 
> Please document these new test certificates in the
> net/data/ssl/certificates/README file.

Done.

> Ideally these test certificates should have a dNSName of
> http://www.example.com instead of an iPAddress of 127.0.0.1 in
> the subject alternative name.
> 
> Since you have already generated these test certificates,
> you can ignore this suggestion.

Yeah, I was following the example of some other certificates. We can easily
regenerate the certs and modify the test, but I don't think it matters for the
correctness of the tests.

[Ryan Sleevi, 2011-12-13 23:54:16]

LGTM.

A few notes below. If the trybots are happy, commit!

http://codereview.chromium.org/8568040/diff/26008/net/base/cert_status_flags.cc
File net/base/cert_status_flags.cc (right):

http://codereview.chromium.org/8568040/diff/26008/net/base/cert_status_flags....
net/base/cert_status_flags.cc:71: return ERR_CERT_WEAK_KEY;
Just a reminder that you will want to update the localized error pages in a
subsequent CL, especially if you wish an interstitial to show to allow the user
to recover (Line 63)

http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate.c...
net/base/x509_certificate.cc:234: }
nit: whitespace

http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate_n...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:1149: SECKEYPublicKey* key =
CERT_ExtractPublicKey(cert_handle);
Can this fail? Do we care?

http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate_u...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:645:
TestRootCerts::GetInstance()->Add(root_cert.get());
Possible flakiness BUG: Make sure to call TestRootCerts::GetInstance()->Clear()
at the end of this test. Leaving a spare root around can cause unexpected issues
with path building.

I suppose I should make a scoped helper here to automagically
TestRootCerts::Clear() [or have the test fixture do it in TearDown)

http://codereview.chromium.org/8568040/diff/26008/net/data/ssl/scripts/ee.cnf
File net/data/ssl/scripts/ee.cnf (right):

http://codereview.chromium.org/8568040/diff/26008/net/data/ssl/scripts/ee.cnf...
net/data/ssl/scripts/ee.cnf:16: # OpenSSL. See http://crbug.com/62973 and
http://crbug.com/20276
Heh, this has long been added. I think this was a carry-over from the original
script I e-mailed you?

You can remove this TODO, for sure, and optionally update the CN like wtc
requested. Re-generating the certs doesn't matter as much as making sure to
remove the TODO.

[wtc, 2011-12-14 00:18:03]

Patch Set 11 LGTM.  Just some nits, and one important comment
that you may handle as a TODO.

http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate.c...
net/base/x509_certificate.cc:233: return false;

Nit: I think it's important to document here or before this
function that this function may return false negatives.

http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate.c...
net/base/x509_certificate.cc:632: return
MapCertStatusToNetError(verify_result->cert_status);

IMPORTANT: this needs the same kind of care that rsleevi used
in his treat-MD5-signature-as error CL, that we should not
overwrite a more serious error in 'rv'.

I believe the IsPublicKeyBlacklisted code above also needs
to do this.

You can do this in a separate CL since we're still very
early in the M18 development cycle.

http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate_m...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate_m...
net/base/x509_certificate_mac.cc:1521: // Since we might fail, set the output
parameters to known values first.

Nit: known => default?

"known values" is a little confusing because kPublicKeyTypeUnknown
says "Unknown" :-)

http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate_u...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:624: return false;

Nit: same here -- I think a comment to document this function
may return false negatives is useful.

[palmer, 2011-12-14 01:04:23]

> Just a reminder that you will want to update the localized error pages in a
> subsequent CL, especially if you wish an interstitial to show to allow the
user
> to recover (Line 63)

Filed 102949.

>
http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate.c...
> net/base/x509_certificate.cc:234: }
> nit: whitespace

Done.

>
http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate_n...
> net/base/x509_certificate_nss.cc:1149: SECKEYPublicKey* key =
> CERT_ExtractPublicKey(cert_handle);
> Can this fail? Do we care?

Yes and yes, so I applied a policy similar to that in _mac.cc.

>
http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate_u...
> net/base/x509_certificate_unittest.cc:645:
> TestRootCerts::GetInstance()->Add(root_cert.get());
> Possible flakiness BUG: Make sure to call
TestRootCerts::GetInstance()->Clear()
> at the end of this test. Leaving a spare root around can cause unexpected
issues
> with path building.

Good point. Done.

>
http://codereview.chromium.org/8568040/diff/26008/net/data/ssl/scripts/ee.cnf...
> net/data/ssl/scripts/ee.cnf:16: # OpenSSL. See http://crbug.com/62973 and
> http://crbug.com/20276
> Heh, this has long been added. I think this was a carry-over from the original
> script I e-mailed you?

Yep! Removed.

[palmer, 2011-12-14 02:22:44]

>
http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate.c...
> net/base/x509_certificate.cc:233: return false;
> 
> Nit: I think it's important to document here or before this
> function that this function may return false negatives.

Done.

> net/base/x509_certificate.cc:632: return
> MapCertStatusToNetError(verify_result->cert_status);
> 
> IMPORTANT: this needs the same kind of care that rsleevi used
> in his treat-MD5-signature-as error CL, that we should not
> overwrite a more serious error in 'rv'.
> 
> I believe the IsPublicKeyBlacklisted code above also needs
> to do this.
> 
> You can do this in a separate CL since we're still very
> early in the M18 development cycle.

Filed as 107479.

> net/base/x509_certificate_mac.cc:1521: // Since we might fail, set the output
> parameters to known values first.
> 
> Nit: known => default?
> 
> "known values" is a little confusing because kPublicKeyTypeUnknown
> says "Unknown" :-)

Done (and for _nss.cc too).

>
http://codereview.chromium.org/8568040/diff/26008/net/base/x509_certificate_u...
> net/base/x509_certificate_unittest.cc:624: return false;
> 
> Nit: same here -- I think a comment to document this function
> may return false negatives is useful.

Done.

[Ryan Sleevi, 2011-12-14 03:11:01]

http://codereview.chromium.org/8568040/diff/37013/net/base/x509_certificate_u...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/8568040/diff/37013/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:642:
key_types.push_back("prime256v1-ecdsa");
Quick follow-up:

This ("prime256v1-ecdsa") will probably fail the XP bots, AFAIK. Unfortunately,
no try servers.

Just be prepared to be around if/when you land it, or try to run it through hive
first.

[wtc, 2011-12-15 02:10:36]

Patch Set 16 LGTM.

http://codereview.chromium.org/8568040/diff/37013/net/base/x509_certificate_u...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/8568040/diff/37013/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:642:
key_types.push_back("prime256v1-ecdsa");

On 2011/12/14 03:11:01, Ryan Sleevi wrote:
>
> This ("prime256v1-ecdsa") will probably fail the XP bots, AFAIK.

Will "prime256v1-ecdsa" also fail on Mac OS X 10.5?  I
remember Mac OS X 10.5 doesn't support ECC either.

[Ryan Sleevi, 2011-12-15 02:20:18]

On 2011/12/15 02:10:36, wtc wrote:
> Patch Set 16 LGTM.
> 
>
http://codereview.chromium.org/8568040/diff/37013/net/base/x509_certificate_u...
> File net/base/x509_certificate_unittest.cc (right):
> 
>
http://codereview.chromium.org/8568040/diff/37013/net/base/x509_certificate_u...
> net/base/x509_certificate_unittest.cc:642:
> key_types.push_back("prime256v1-ecdsa");
> 
> On 2011/12/14 03:11:01, Ryan Sleevi wrote:
> >
> > This ("prime256v1-ecdsa") will probably fail the XP bots, AFAIK.
> 
> Will "prime256v1-ecdsa" also fail on Mac OS X 10.5?  I
> remember Mac OS X 10.5 doesn't support ECC either.

Ah, goot catch. Yes, it will also fail.

Palmer: See base::mac::IsSnowLeopardOrLater(), similar to the
base::win::GetVersion() check suggested

[palmer, 2011-12-15 02:25:42]

> Palmer: See base::mac::IsSnowLeopardOrLater(), similar to the
> base::win::GetVersion() check suggested

Just added that while you were typing this. Building locally now, and will
upload and try momentarily.

[commit-bot: I haz the power, 2011-12-15 21:22:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/8568040/59001

[commit-bot: I haz the power, 2011-12-15 21:23:41]

Presubmit check for 8568040-59001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
  net/data/ssl/scripts/generate-weak-test-chains.sh

Presubmit checks took 5.0s to calculate.

[commit-bot: I haz the power, 2011-12-15 21:28:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/8568040/61005

[commit-bot: I haz the power, 2011-12-15 22:40:02]

Change committed as 114709

[Timur Iskhodzhanov, 2011-12-16 08:35:39]

This has introduced a leak,
http://code.google.com/p/chromium/issues/detail?id=107841

http://codereview.chromium.org/8568040/diff/61005/net/base/x509_certificate_n...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/8568040/diff/61005/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:1176: }
forgotten to SECKEY_DestroyPublicKey(key) ?