Follow-up changes to Android certificate verification logic.

Follow-up changes to Android certificate verification logic.

- Handle devices without an "AndroidCAStore" KeyStore. Add a histogram to
  record when this happens.
- Remove resolved TODO.
- Unnecessary exception check (relevant function already CalledByNative, not
  CalledByNativeUnchecked)

BUG=116838
TEST=none

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

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

Uploaded as follow-up to comments in https://codereview.chromium.org/108653013/.

I made it treat missing root CA store as setting is_issued_by_known_root to
false everywhere, to match the pre-JB behavior. I think that's probably the
safer option? Though it does mean we'd lose most of our more interesting cert
policies for the sake of not breaking random non-standard roots. Also added a
histogram so we can see if this ever happens.

rsleevi: General review. Is the is_issued_by_known_root behavior/histogram
reasonable?
yfriedman: Java bits
asvitkine: histograms.xml OWNERS
joth: FYI and/or review. Dunno if you're still reviewing and stuff since it says
"joth (inactive)".

[joth, 2014-01-21 19:04:39]

On 2014/01/21 17:33:28, David Benjamin wrote:
> Uploaded as follow-up to comments in
https://codereview.chromium.org/108653013/.
> 
> I made it treat missing root CA store as setting is_issued_by_known_root to
> false everywhere, to match the pre-JB behavior.

No comment if this is what you want now, but FWIW the behavior prior to
crrev.com/108653013 was to always set it true:-
See line 117 on LHS of 
https://codereview.chromium.org/108653013/diff/530001/net/cert/cert_verify_pr...

> I think that's probably the
> safer option? 

When the mechanism was first introduced, agl erred on the side of
not-breaking-users by defaulting to true where we weren't sure.
Historic references:
http://src.chromium.org/viewvc/chrome?revision=80778&view=revision
https://codereview.chromium.org/6816020/




> Though it does mean we'd lose most of our more interesting cert
> policies for the sake of not breaking random non-standard roots. Also added a
> histogram so we can see if this ever happens.
> 
> rsleevi: General review. Is the is_issued_by_known_root behavior/histogram
> reasonable?
> yfriedman: Java bits
> asvitkine: histograms.xml OWNERS
> joth: FYI and/or review. Dunno if you're still reviewing and stuff since it
says
> "joth (inactive)".

I'm not actively taking reviews on new code, but as this was related to
something I'd been involved in for quite a while I was keen to get closure on it
:-)

Cheers!

[Ryan Sleevi, 2014-01-21 19:31:58]

On 2014/01/21 19:04:39, joth (inactive) wrote:
> On 2014/01/21 17:33:28, David Benjamin wrote:
> > Uploaded as follow-up to comments in
> https://codereview.chromium.org/108653013/.
> > 
> > I made it treat missing root CA store as setting is_issued_by_known_root to
> > false everywhere, to match the pre-JB behavior.
> 
> No comment if this is what you want now, but FWIW the behavior prior to
> crrev.com/108653013 was to always set it true:-
> See line 117 on LHS of 
>
https://codereview.chromium.org/108653013/diff/530001/net/cert/cert_verify_pr...

Sure.

Setting to false avoids added security checks, but avoids breaking internal CAs.
Setting to true adds additional security checks (good), but may also break
internal CAs.

> 
> > I think that's probably the
> > safer option? 
> 
> When the mechanism was first introduced, agl erred on the side of
> not-breaking-users by defaulting to true where we weren't sure.
> Historic references:
> http://src.chromium.org/viewvc/chrome?revision=80778&view=revision
> https://codereview.chromium.org/6816020/
> 

not-breaking-users = default=false

> 
> 
> 
> > Though it does mean we'd lose most of our more interesting cert
> > policies for the sake of not breaking random non-standard roots. Also added
a
> > histogram so we can see if this ever happens.
> > 
> > rsleevi: General review. Is the is_issued_by_known_root behavior/histogram
> > reasonable?
> > yfriedman: Java bits
> > asvitkine: histograms.xml OWNERS
> > joth: FYI and/or review. Dunno if you're still reviewing and stuff since it
> says
> > "joth (inactive)".
> 
> I'm not actively taking reviews on new code, but as this was related to
> something I'd been involved in for quite a while I was keen to get closure on
it
> :-)
> 
> Cheers!

[Ryan Sleevi, 2014-01-21 19:33:49]

https://codereview.chromium.org/144153002/diff/30001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/X509Util.java (right):

https://codereview.chromium.org/144153002/diff/30001/net/android/java/src/org...
net/android/java/src/org/chromium/net/X509Util.java:212: // Only record
histograms the first time successfully through.
I'm not sure I follow your logic for this.

If you're trying to avoid noise and get absolute user counts, you're still going
to find noise from situations where the browser restarts (eg: due to memory
pressure).

You're better off using the histogram filters to approximate counts, rather than
filter here. Unless there's some hidden JNI cost, in which case it seems like
returning a boolean/int in the result would be a more efficient way to avoid
that cost?

[davidben, 2014-01-21 20:29:54]

https://codereview.chromium.org/144153002/diff/30001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/X509Util.java (right):

https://codereview.chromium.org/144153002/diff/30001/net/android/java/src/org...
net/android/java/src/org/chromium/net/X509Util.java:212: // Only record
histograms the first time successfully through.
On 2014/01/21 19:33:49, Ryan Sleevi wrote:
> I'm not sure I follow your logic for this.
> 
> If you're trying to avoid noise and get absolute user counts, you're still
going
> to find noise from situations where the browser restarts (eg: due to memory
> pressure).
> 
> You're better off using the histogram filters to approximate counts, rather
than
> filter here. Unless there's some hidden JNI cost, in which case it seems like
> returning a boolean/int in the result would be a more efficient way to avoid
> that cost?

Yeah, I was thinking user counts, but you're right that that seems kind of off.
Switched it to record a sample per verify. I'm not aware of any hidden JNI
costs.

[Ryan Sleevi, 2014-01-21 20:35:07]

LGTM, assuming Yaron is happy

https://codereview.chromium.org/144153002/diff/90002/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/144153002/diff/90002/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:8730: +    Whether the system trust
roots were found on Android 4.2 and up.
Better to be a little more verbose.

Whether or not system installed trust anchors could be distinguished from user
installed trust anchors. Only recorded on Android 4.2 and later.

That is, IF some other image/vendor uses some other naming scheme, and we added
support for it, I'd still like to see the histogram record the same. At least,
that's my thought?

[davidben, 2014-01-21 20:39:57]

https://codereview.chromium.org/144153002/diff/90002/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/144153002/diff/90002/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:8730: +    Whether the system trust
roots were found on Android 4.2 and up.
On 2014/01/21 20:35:07, Ryan Sleevi wrote:
> Better to be a little more verbose.
> 
> Whether or not system installed trust anchors could be distinguished from user
> installed trust anchors. Only recorded on Android 4.2 and later.
> 
> That is, IF some other image/vendor uses some other naming scheme, and we
added
> support for it, I'd still like to see the histogram record the same. At least,
> that's my thought?

Done.

[Yaron, 2014-01-21 20:57:13]

lgtm

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

https://codereview.chromium.org/144153002/diff/170001/net/android/java/src/or...
net/android/java/src/org/chromium/net/X509Util.java:185: Log.w(TAG, "Could not
load system trust root set", e);
Just want to double check that this error doesn't include anything sensitive or
private.  The logs get output to the shared logcat which any app (with requested
permission) can read.

[Alexei Svitkine (slow), 2014-01-22 16:22:55]

lgtm with a nit

https://codereview.chromium.org/144153002/diff/170001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/144153002/diff/170001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:8731: +    user installed trust anchors.
Only recorded on Android 4.2 and later.
Nit: Mention when this is recorded? (on startup? when a certain event happens?
etc)

[davidben, 2014-01-22 17:51:14]

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

https://codereview.chromium.org/144153002/diff/170001/net/android/java/src/or...
net/android/java/src/org/chromium/net/X509Util.java:185: Log.w(TAG, "Could not
load system trust root set", e);
On 2014/01/21 20:57:14, Yaron wrote:
> Just want to double check that this error doesn't include anything sensitive
or
> private.  The logs get output to the shared logcat which any app (with
requested
> permission) can read.

It shouldn't. On my device, if I change the string to one that isn't there, it
says:

W/X509Util( 1447): Could not load system trust root set
W/X509Util( 1447): java.security.KeyStoreException:
java.security.NoSuchAlgorithmException: KeyStore AndroidCAStorez implementation
not found
W/X509Util( 1447): 	at java.security.KeyStore.getInstance(KeyStore.java:119)
W/X509Util( 1447): 	at
org.chromium.net.X509Util.buildSystemTrustRootSet(X509Util.java:211)
W/X509Util( 1447): 	at
org.chromium.net.X509Util.ensureInitialized(X509Util.java:180)
W/X509Util( 1447): 	at
org.chromium.net.X509Util.createCertificateFromBytes(X509Util.java:287)
W/X509Util( 1447): 	at
org.chromium.net.X509Util.verifyServerCertificates(X509Util.java:370)
W/X509Util( 1447): 	at
org.chromium.net.AndroidNetworkLibrary.verifyServerCertificates(AndroidNetworkLibrary.java:214)
W/X509Util( 1447): 	at dalvik.system.NativeStart.run(Native Method)
W/X509Util( 1447): Caused by: java.security.NoSuchAlgorithmException: KeyStore
AndroidCAStorez implementation not found
W/X509Util( 1447): 	at
org.apache.harmony.security.fortress.Engine.notFound(Engine.java:177)
W/X509Util( 1447): 	at
org.apache.harmony.security.fortress.Engine.getInstance(Engine.java:151)
W/X509Util( 1447): 	at java.security.KeyStore.getInstance(KeyStore.java:116)
W/X509Util( 1447): 	... 6 more

so that's not especially interesting.

Though, because it's emitted at every certificate verify now, it does leak
whether the browser did an SSL handshake (but not with what site). Is that
considered a problem for logcat? I can suppress all but the first error. Or
perhaps add a boolean so, if it can't load sSystemTrustRoots, it doesn't try
again until reloadDefaultTrustManager is called. Possibly not a bad plan anyway.

https://codereview.chromium.org/144153002/diff/170001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/144153002/diff/170001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:8731: +    user installed trust anchors.
Only recorded on Android 4.2 and later.
On 2014/01/22 16:22:55, Alexei Svitkine wrote:
> Nit: Mention when this is recorded? (on startup? when a certain event happens?
> etc)

Done.

[davidben, 2014-01-22 18:26:46]

Tweaked it to only try calling buildSystemTrustRoot once, even if it fails to
load the key store. Dunno if this is preferable. Yaron tells me out-of-band that
the logcat issue is probably fine. But it'll avoid spamming the log and looking
up the non-existent KeyStore each time. Does bring back the record on "startup"
thing though.

[Ryan Sleevi, 2014-01-31 20:25:56]

lgtm

[davidben, 2014-01-31 20:31:45]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-01-31 20:32:19]

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

[commit-bot: I haz the power, 2014-01-31 21:20:25]

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

[commit-bot: I haz the power, 2014-02-01 04:34:38]

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

[commit-bot: I haz the power, 2014-02-01 04:34:38]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-02-01 04:34:43]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-01 04:34:45]

CQ bit was unchecked on CL. Ignoring.

[davidben, 2014-02-03 19:39:59]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-02-03 19:40:17]

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

[commit-bot: I haz the power, 2014-02-03 22:08:09]

Change committed as 248587

[commit-bot: I haz the power, 2014-02-03 22:08:12]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 22:08:17]

CQ bit was unchecked on CL. Ignoring.