Export verified_cert and public_key_hashes on Android.

Export verified_cert and public_key_hashes on Android.

On API level 17 and up, X509TrustManager can export the verified chain. Use it
to populate some of the fields in CertVerifyResult. Also correctly populate
is_issued_by_known_root and enable intranet host checking.

Add a test to make sure non-standard roots get flagged as such. If the APIs
are not available, is_issued_by_known_root is always false.

BUG=116838, 147945
TEST=CertVerifyProcTest.PublicKeyHashes
     CertVerifyProcTest.VerifyReturnChainBasic
     CertVerifyProcTest.VerifyReturnChainFiltersUnrelatedCerts
     CertVerifyProcTest.VerifyReturnChainProperlyOrdered
     CertVerifyProcTest.IntranetHostsRejected
     CertVerifyProcTest.IsIssuedByKnownRootIgnoresTestRoots
     CertVerifyProcTest.ExtraneousMD5RootCert
     CertVerifyProcTest.NameConstraintsFailure

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245649

[davidben, 2013-12-18 02:03:20]

Here's an initial version. I haven't actually ran the tests that I've enabled;
if the try bots don't do it, I'll figure out how to run them tomorrow. So far
I've tested by copying over the key pinning code from the NSS socket
implementation and verified that pinning works.

The version doesn't populate is_issued_by_known_root. I implemented the
AndroidCAStore/"system:"-prefix thing, but it doesn't seem to work. Will play
with it more tomorrow.

Not entirely happy with the CertVerifyResult/CertVerifyStatus thing. Maybe I'll
wrestle with GYP to try to get both @CallByNative and java_cpp_template working
on the same file.

[davidben, 2013-12-18 23:49:08]

Alright, it should populate is_issued_by_known_root correctly now. I've added a
test to cover this case now; hopefully it passes on other platforms too. It does
build a giant hash table of all the system trust roots. Dunno if we'll want to
make that cleverer memory-wise.

When running on a platform version which doesn't expose
X509TrustManagerExtensions, verified_certs should continue to contain the
server's chain, as before. is_issued_by_known_root should always be false, which
seems a cleaner default? I can also make it true and add a runtime version check
to the intranet check.

[Ryan Sleevi, 2013-12-19 00:06:58]

I've copied Kenny on here, because of a question below.

The Android bits should be reviewed by an Android OWNER to make sure they're
good, but my naive reading is that it all looks good. Thanks for the quick
turnaround!

https://codereview.chromium.org/108653013/diff/160001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/X509Util.java (right):

https://codereview.chromium.org/108653013/diff/160001/net/android/java/src/or...
net/android/java/src/org/chromium/net/X509Util.java:218: Certificate cert =
systemKeyStore.getCertificate(alias);
I defer to Kenny on this. I'm curious why you wouldn't use
systemKeyStore.entryInstanceOf(alias, KeyStore.TrustedCertificateEntry) and then
use getEntry to get it.

https://codereview.chromium.org/108653013/diff/160001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/108653013/diff/160001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:627: #endif
Can you move each of these conditionals into a common method in an unnamed
namespace, and then check at the beginning of each test?

Also, as a cleanup while you're here, can you nuke line 596?

(note: I realize that lines 620/621 will work on Android < 17, but I'd say we
shouldn't be running the test at all < 17)

eg:
if (!SupportsDetectingKnownRoots())
  return;

https://codereview.chromium.org/108653013/diff/160001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:977: TEST_F(CertVerifyProcTest,
NonStandardRoot) {
s/NonStandardRoot/IsIssuedByKnownRootIgnoresTestRoots/

Also, please add a comment here (consistent with the other tests) that explains
what exactly is being tested.

[davidben, 2013-12-19 00:42:51]

https://codereview.chromium.org/108653013/diff/160001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/X509Util.java (right):

https://codereview.chromium.org/108653013/diff/160001/net/android/java/src/or...
net/android/java/src/org/chromium/net/X509Util.java:218: Certificate cert =
systemKeyStore.getCertificate(alias);
On 2013/12/19 00:06:59, Ryan Sleevi wrote:
> I defer to Kenny on this. I'm curious why you wouldn't use
> systemKeyStore.entryInstanceOf(alias, KeyStore.TrustedCertificateEntry) and
then
> use getEntry to get it.

Eh, this seemed a little cleaner. I'd assumed they're intended to be equivalent,
even though it says getCertificate and not getTrustedCertificate. Also there's a
isCertificateEntry function documented as "Indicates whether the specified alias
is associated with a KeyStore.TrustedCertificateEntry." And there isn't some
other entry type that looks certificate-related. If they're actually intended to
be different, then yeah probably should use entries.

https://codereview.chromium.org/108653013/diff/160001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/108653013/diff/160001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:627: #endif
On 2013/12/19 00:06:59, Ryan Sleevi wrote:
> Can you move each of these conditionals into a common method in an unnamed
> namespace, and then check at the beginning of each test?
> 
> Also, as a cleanup while you're here, can you nuke line 596?
> 
> (note: I realize that lines 620/621 will work on Android < 17, but I'd say we
> shouldn't be running the test at all < 17)
> 
> eg:
> if (!SupportsDetectingKnownRoots())
>   return;

Done.

https://codereview.chromium.org/108653013/diff/160001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:977: TEST_F(CertVerifyProcTest,
NonStandardRoot) {
On 2013/12/19 00:06:59, Ryan Sleevi wrote:
> s/NonStandardRoot/IsIssuedByKnownRootIgnoresTestRoots/
> 
> Also, please add a comment here (consistent with the other tests) that
explains
> what exactly is being tested.

Done.

[davidben, 2013-12-20 17:37:09]

Updated to enable a couple more tests that should work now.

[Ryan Sleevi, 2014-01-11 01:48:55]

On 2013/12/20 17:37:09, David Benjamin wrote:
> Updated to enable a couple more tests that should work now.

Friendly ping, Kenny :)

[Kenny Root, 2014-01-13 17:35:49]

Sorry, I didn't click publish draft comments apparently.

https://codereview.chromium.org/108653013/diff/160001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/X509Util.java (right):

https://codereview.chromium.org/108653013/diff/160001/net/android/java/src/or...
net/android/java/src/org/chromium/net/X509Util.java:218: Certificate cert =
systemKeyStore.getCertificate(alias);
On 2013/12/19 00:42:52, David Benjamin wrote:
> Eh, this seemed a little cleaner. I'd assumed they're intended to be
equivalent,
> even though it says getCertificate and not getTrustedCertificate. Also there's
a
> isCertificateEntry function documented as "Indicates whether the specified
alias
> is associated with a KeyStore.TrustedCertificateEntry." And there isn't some
> other entry type that looks certificate-related. If they're actually intended
to
> be different, then yeah probably should use entries.

Yes, getCertificate() is for TrustedCertificateEntry and getCertificateChain()
is for KeyEntry types.

[Ryan Sleevi, 2014-01-16 23:29:41]

LGTM, mod one pre-existing bug.

You'll need one of the Android guys to stamp the Java files. Adding Yaron, since
src/net/android/OWNERS looks a little out of date...

https://codereview.chromium.org/108653013/diff/210001/net/android/network_lib...
File net/android/network_library.cc (right):

https://codereview.chromium.org/108653013/diff/210001/net/android/network_lib...
net/android/network_library.cc:49: 
Can you check |env| to ensure no exception happened here? Otherwise, I fear
we're possibly using an invalid |result|

[davidben, 2014-01-17 17:52:58]

+benm for android_webview/ as well. (Just buildsystem stuff.)

https://codereview.chromium.org/108653013/diff/210001/net/android/network_lib...
File net/android/network_library.cc (right):

https://codereview.chromium.org/108653013/diff/210001/net/android/network_lib...
net/android/network_library.cc:49: 
On 2014/01/16 23:29:42, Ryan Sleevi wrote:
> Can you check |env| to ensure no exception happened here? Otherwise, I fear
> we're possibly using an invalid |result|

Done.

[benm (inactive), 2014-01-17 17:58:24]

On 2014/01/17 17:52:58, David Benjamin wrote:
> +benm for android_webview/ as well. (Just buildsystem stuff.)
> 
>
https://codereview.chromium.org/108653013/diff/210001/net/android/network_lib...
> File net/android/network_library.cc (right):
> 
>
https://codereview.chromium.org/108653013/diff/210001/net/android/network_lib...
> net/android/network_library.cc:49: 
> On 2014/01/16 23:29:42, Ryan Sleevi wrote:
> > Can you check |env| to ensure no exception happened here? Otherwise, I fear
> > we're possibly using an invalid |result|
> 
> Done.

android_webview lgtm

[Yaron, 2014-01-17 19:31:56]

lgtm

[commit-bot: I haz the power, 2014-01-17 20:17:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/108653013/530001

[commit-bot: I haz the power, 2014-01-17 22:52:18]

Change committed as 245649

[joth, 2014-01-18 00:23:13]

Minor drive-by comments. Great to see this hooked up!

https://codereview.chromium.org/108653013/diff/530001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/X509Util.java (right):

https://codereview.chromium.org/108653013/diff/530001/net/android/java/src/or...
net/android/java/src/org/chromium/net/X509Util.java:204: KeyStore systemKeyStore
= KeyStore.getInstance("AndroidCAStore");
Note "AndroidCAStore" is really an internal detail of the platform, and any
given device might legitimately not have such a key store (i.e., no CTS test
would break if it was removed) (see http://b/5826113#ISSUE_HistoryHeader55)

Hence I'd suggest getting a KeyStoreException here should not be treated like
other key store exceptions (which result in the entire cert check to fail), and
instead just cause the 'is known root' to always be true (as it was prior to
this CL, I think?).

https://codereview.chromium.org/108653013/diff/530001/net/android/java/src/or...
net/android/java/src/org/chromium/net/X509Util.java:243: if
(Build.VERSION.SDK_INT >= 17) {
nit: you can use Build.VERSION_CODES.JELLY_BEAN instead of hard code 17 (aids
readability but no runtime difference: it can never change).

https://codereview.chromium.org/108653013/diff/530001/net/android/network_lib...
File net/android/network_library.cc (right):

https://codereview.chromium.org/108653013/diff/530001/net/android/network_lib...
net/android/network_library.cc:50: *status = android::VERIFY_FAILED;
IIRC this shouldn't be needed: AndroidNetworkLibrary.verifyServerCertificates is
marked as @CalledByNative, not @CalledByNativeUnchecked, hence the jni generator
will already have asserted that no excpetion was thrown before
Java_AndroidNetworkLibrary_verifyServerCertificates returns.

https://codereview.chromium.org/108653013/diff/530001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_android.cc (right):

https://codereview.chromium.org/108653013/diff/530001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_android.cc:147: // TODO(ppi): Implement missing
functionality: yielding the constructed trust
nit: I think this TODO is mostly obsolete now?

[davidben, 2014-01-21 17:28:56]

Uploaded https://codereview.chromium.org/144153002 for those comments.

https://codereview.chromium.org/108653013/diff/530001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/X509Util.java (right):

https://codereview.chromium.org/108653013/diff/530001/net/android/java/src/or...
net/android/java/src/org/chromium/net/X509Util.java:204: KeyStore systemKeyStore
= KeyStore.getInstance("AndroidCAStore");
On 2014/01/18 00:23:13, joth (inactive) wrote:
> Note "AndroidCAStore" is really an internal detail of the platform, and any
> given device might legitimately not have such a key store (i.e., no CTS test
> would break if it was removed) (see http://b/5826113#ISSUE_HistoryHeader55)
> 
> Hence I'd suggest getting a KeyStoreException here should not be treated like
> other key store exceptions (which result in the entire cert check to fail),
and
> instead just cause the 'is known root' to always be true (as it was prior to
> this CL, I think?).

Oh hrm. I'll defer to Ryan, but defaulting to false is probably better? That's
what this CL currently does if the APIs are missing. It used to default true,
but also disabled every check that depends on this. False would probably break
fewer things. Though yeah, either way it should probably be treating
KeyStoreException differently here.

[joth, 2014-01-21 19:15:20]

On 21 January 2014 09:28, <davidben@chromium.org> wrote:

> Uploaded https://codereview.chromium.org/144153002 for those comments.
>
>
>
> https://codereview.chromium.org/108653013/diff/530001/net/
> android/java/src/org/chromium/net/X509Util.java
> File net/android/java/src/org/chromium/net/X509Util.java (right):
>
> https://codereview.chromium.org/108653013/diff/530001/net/
> android/java/src/org/chromium/net/X509Util.java#newcode204
> net/android/java/src/org/chromium/net/X509Util.java:204: KeyStore
> systemKeyStore = KeyStore.getInstance("AndroidCAStore");
> On 2014/01/18 00:23:13, joth (inactive) wrote:
>
>> Note "AndroidCAStore" is really an internal detail of the platform,
>>
> and any
>
>> given device might legitimately not have such a key store (i.e., no
>>
> CTS test
>
>> would break if it was removed) (see
>>
> http://b/5826113#ISSUE_HistoryHeader55)
>
>  Hence I'd suggest getting a KeyStoreException here should not be
>>
> treated like
>
>> other key store exceptions (which result in the entire cert check to
>>
> fail), and
>
>> instead just cause the 'is known root' to always be true (as it was
>>
> prior to
>
>> this CL, I think?).
>>
>
> Oh hrm. I'll defer to Ryan, but defaulting to false is probably better?
> That's what this CL currently does if the APIs are missing. It used to
> default true, but also disabled every check that depends on this. False
> would probably break fewer things. Though yeah, either way it should
> probably be treating KeyStoreException differently here.
>
>
Ah just saw this after the other CL. You're right, I was misremembering.
false is (at least, was when I looked at it) safest from the "not break
things" sense.  Over to Ryan on which is correct here. (It seems running on
pre-JB device and failing to find this 'hidden' API should be treated
equivalently, anyway)

Definite +1 for adding the histogram, very good idea.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.