Fixing BoringSSL on iOS

Fixing BoringSSL on iOS

This adds the final changes to cert_verify_proc_ios and various tests
in order to prepare for the switchover from NSS to BoringSSL on iOS.

BUG=591545
Committed: https://crrev.com/b7f886b3d364497ff0422e9808d8ad04ffcb5c4d
Cr-Commit-Position: refs/heads/master@{#386819}

[svaldez, 2016-04-08 15:29:05]

svaldez@chromium.org changed reviewers:
+ davidben@chromium.org, rsleevi@chromium.org

[svaldez, 2016-04-08 15:29:06]

These are most of the remaining changes needed for BoringSSL on iOS. The only
remaining issues are that the nist-pkits data files still give issues when
trying to add them to the iOS xcodebuild. Still investigating.

Meanwhile, I'm running the trybots on
https://codereview.chromium.org/1872813002, which just flips the use_openssl
flag to always be on.

After both of these are committed, I'll be removing NSS and possibly all of the
use_openssl flags.

[Ryan Sleevi, 2016-04-08 20:01:58]

Mostly nits

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:143: // Ignore the signature algorithm for the
root (self-signed) certificate.
1) s/root (self-signed) certificate/trust anchor/ (no guarantee a TA is
self-signed)
2) I think we only want to ignore if it was trusted; if it was untrusted, we may
have an incomplete chain.

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:226: if (properties &&
CFArrayGetCount(properties) != 0) {
Can you expand documentation comment to explain this (and why it's brittle but
acceptable). This is non-intuitive (although I totally understand) and deserves
a comment.

Also, is this enough logic that we should break it into a helper function (which
would help readability)?

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:240: const CFIndex properties_length =
CFArrayGetCount(properties);
newline between 239 & 240 would help readability here.

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:243:
(CFDictionaryRef)CFArrayGetValueAtIndex(properties, i);
C-style casts aren't allowed ;(

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:245: (CFStringRef)CFDictionaryGetValue(dict,
CFSTR("value"));
Ditto

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:246: if (CFStringCompare(error, date_error, 0)
== kCFCompareEqualTo) {
Why not CFEqual for these?

[svaldez, 2016-04-08 20:22:48]

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:143: // Ignore the signature algorithm for the
root (self-signed) certificate.
On 2016/04/08 20:01:58, Ryan Sleevi wrote:
> 1) s/root (self-signed) certificate/trust anchor/ (no guarantee a TA is
> self-signed)
> 2) I think we only want to ignore if it was trusted; if it was untrusted, we
may
> have an incomplete chain.

This is only for setting the has_XXX bits on the verification result. Adding it
to the chain and copying the key hashes for verification are done earlier.

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:226: if (properties &&
CFArrayGetCount(properties) != 0) {
On 2016/04/08 20:01:58, Ryan Sleevi wrote:
> Can you expand documentation comment to explain this (and why it's brittle but
> acceptable). This is non-intuitive (although I totally understand) and
deserves
> a comment.
> 
> Also, is this enough logic that we should break it into a helper function
(which
> would help readability)?

Done.

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:240: const CFIndex properties_length =
CFArrayGetCount(properties);
On 2016/04/08 20:01:58, Ryan Sleevi wrote:
> newline between 239 & 240 would help readability here.

Done.

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:243:
(CFDictionaryRef)CFArrayGetValueAtIndex(properties, i);
On 2016/04/08 20:01:58, Ryan Sleevi wrote:
> C-style casts aren't allowed ;(

Done.

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:245: (CFStringRef)CFDictionaryGetValue(dict,
CFSTR("value"));
On 2016/04/08 20:01:58, Ryan Sleevi wrote:
> Ditto

Done.

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:246: if (CFStringCompare(error, date_error, 0)
== kCFCompareEqualTo) {
On 2016/04/08 20:01:58, Ryan Sleevi wrote:
> Why not CFEqual for these?

Done.

[Ryan Sleevi, 2016-04-08 20:44:21]

I think we still only want to give the "count - 1" for the case of a successful
validation; if it wasn't successful, just examine all the certs for the
has_logic.

Explanation included below.

Otherwise, this LG with nits, but I'll hold off on the TM for that :)

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/1871043003/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:143: // Ignore the signature algorithm for the
root (self-signed) certificate.
On 2016/04/08 20:22:47, svaldez wrote:
> On 2016/04/08 20:01:58, Ryan Sleevi wrote:
> > 1) s/root (self-signed) certificate/trust anchor/ (no guarantee a TA is
> > self-signed)
> > 2) I think we only want to ignore if it was trusted; if it was untrusted, we
> may
> > have an incomplete chain.
> 
> This is only for setting the has_XXX bits on the verification result. Adding
it
> to the chain and copying the key hashes for verification are done earlier.

Yes, but has_sha1 is valid if, for example, the intermediate has SHA-1 and we
couldn't build a path to the root, so the intermediate is (count - 1)

For example, see the logic here -
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/cert_veri...
- or
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/cert_veri...

https://codereview.chromium.org/1871043003/diff/120001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/1871043003/diff/120001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:175: // certificate chain to differentiate
between various verification failures.
Let's expand this more:

// The iOS APIs don't expose an API-stable set of reasons for certificate
validation failures.
// However, internally, the reason is tracked, and it's converted to user-facing
localized
// strings.
// In the absence of a consistent API, convert the English strings to their
localized counterpart,
// and then compare that with the error properties. If they're equal, it's a
strong sign that this
// was the cause for the error. While this will break if/when iOS changes the
contents of these
// strings, it's sufficient enough for now.
// TODO(rsleevi): https://crbug.com/601915 - Use a less brittle solution when
possible.

https://codereview.chromium.org/1871043003/diff/120001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:191: bundle, date_string, date_string,
CFSTR("SecCertificate"));
Can you add a DCHECK that each of the error strings (date_error, trust_error,
weak_error) returns a valid string?

This will help insulate against future changes to these strings in new iOS
releases, by triggering the DCHECKs during testing.

[svaldez, 2016-04-08 20:51:38]

https://codereview.chromium.org/1871043003/diff/120001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/1871043003/diff/120001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:175: // certificate chain to differentiate
between various verification failures.
On 2016/04/08 20:44:21, Ryan Sleevi wrote:
> Let's expand this more:
> 
> // The iOS APIs don't expose an API-stable set of reasons for certificate
> validation failures.
> // However, internally, the reason is tracked, and it's converted to
user-facing
> localized
> // strings.
> // In the absence of a consistent API, convert the English strings to their
> localized counterpart,
> // and then compare that with the error properties. If they're equal, it's a
> strong sign that this
> // was the cause for the error. While this will break if/when iOS changes the
> contents of these
> // strings, it's sufficient enough for now.
> // TODO(rsleevi): https://crbug.com/601915 - Use a less brittle solution when
> possible.

Done.

https://codereview.chromium.org/1871043003/diff/120001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:191: bundle, date_string, date_string,
CFSTR("SecCertificate"));
On 2016/04/08 20:44:21, Ryan Sleevi wrote:
> Can you add a DCHECK that each of the error strings (date_error, trust_error,
> weak_error) returns a valid string?
> 
> This will help insulate against future changes to these strings in new iOS
> releases, by triggering the DCHECKs during testing.

Done.

[svaldez, 2016-04-08 21:26:33]

Actually, it looks like I'm not getting the localized strings yet, looking into
whether this is because the strings aren't actually in the bundle, or something
else.

[svaldez, 2016-04-11 14:41:06]

I added a DATE_INVALID test to cert_verify_proc_unittest instead so that tests
will still fail if the localization strings end up changing.

[davidben, 2016-04-11 21:46:26]

https://codereview.chromium.org/1871043003/diff/240001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/1871043003/diff/240001/build/all.gyp#newcode268
build/all.gyp:268: '../third_party/boringssl/boringssl_tests.gyp:*',
Hrm. Do these not build at all? I would have thought that they'd build but the
janky runner wouldn't work since we can't fork. (Although this whole situation
is kind of silly. We really should parse that JSON file and whatnot.)

https://codereview.chromium.org/1871043003/diff/240001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/1871043003/diff/240001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_unittest.cc:229: #elif defined(OS_IOS)
defined(OS_IOS) && TARGET_IPHONE_SIMULATOR?

That way we can test that it behaves right on a real device.

[Ryan Sleevi, 2016-04-11 22:26:08]

https://codereview.chromium.org/1871043003/diff/240001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/1871043003/diff/240001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:209: bundle, weak_string, weak_string,
CFSTR("SecCertificate"));
Did you find out why you weren't getting the localized string?

I would think we need to be DCHECKing here to make sure we're getting result,
otherwise the premise that this will work doesn't work?

[svaldez, 2016-04-12 14:36:31]

https://codereview.chromium.org/1871043003/diff/240001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/1871043003/diff/240001/build/all.gyp#newcode268
build/all.gyp:268: '../third_party/boringssl/boringssl_tests.gyp:*',
On 2016/04/11 21:46:26, davidben (OOO 4-4 to 4-11) wrote:
> Hrm. Do these not build at all? I would have thought that they'd build but the
> janky runner wouldn't work since we can't fork. (Although this whole situation
> is kind of silly. We really should parse that JSON file and whatnot.)

Yeah, we can't just build random executables, and we also have issues moving
this check to boringssl_tests, since it needs at least one valid target or else
the buildsystem complains.

https://codereview.chromium.org/1871043003/diff/240001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/1871043003/diff/240001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:209: bundle, weak_string, weak_string,
CFSTR("SecCertificate"));
On 2016/04/11 22:26:08, Ryan Sleevi wrote:
> Did you find out why you weren't getting the localized string?
> 
> I would think we need to be DCHECKing here to make sure we're getting result,
> otherwise the premise that this will work doesn't work?

I suspect either these strings were never actually localized or the dev SDK
doesn't include them. I tried changing the language on the actual iPad, and
ended up still getting the English strings, though possibly only a subset of
languages are localized.

Hopefully the new test will mean that if this isn't working, then we'll have
test failures anyway.

https://codereview.chromium.org/1871043003/diff/240001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/1871043003/diff/240001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_unittest.cc:229: #elif defined(OS_IOS)
On 2016/04/11 21:46:26, davidben (OOO 4-4 to 4-11) wrote:
> defined(OS_IOS) && TARGET_IPHONE_SIMULATOR?
> 
> That way we can test that it behaves right on a real device.

Done.

[davidben, 2016-04-12 19:45:47]

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

[davidben, 2016-04-12 19:45:47]

lgtm, but get Ryan's review on the certificate bits.

https://codereview.chromium.org/1871043003/diff/240001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/1871043003/diff/240001/build/all.gyp#newcode268
build/all.gyp:268: '../third_party/boringssl/boringssl_tests.gyp:*',
On 2016/04/12 14:36:31, svaldez wrote:
> On 2016/04/11 21:46:26, davidben (OOO 4-4 to 4-11) wrote:
> > Hrm. Do these not build at all? I would have thought that they'd build but
the
> > janky runner wouldn't work since we can't fork. (Although this whole
situation
> > is kind of silly. We really should parse that JSON file and whatnot.)
> 
> Yeah, we can't just build random executables, and we also have issues moving
> this check to boringssl_tests, since it needs at least one valid target or
else
> the buildsystem complains.

Got it. Yeah, we should redo how these tests get built in Chromium. It's silly
because we're not even running them anyway.

[Ryan Sleevi, 2016-04-12 19:51:02]

lgtm

[svaldez, 2016-04-12 19:55:31]

svaldez@chromium.org changed reviewers:
+ thakis@chromium.org

[svaldez, 2016-04-12 19:55:32]

Hi,

Could you take a look at the build/all.gyp change. This should be the last CL
before the one to flip iOS to use BoringSSL instead of NSS.

[Nico, 2016-04-12 19:58:03]

lgtm

https://codereview.chromium.org/1871043003/diff/280001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/1871043003/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:180: // localized counterpart, and then compare
that with the error properties. If
Hahahahaha

:-(

[svaldez, 2016-04-12 19:59:44]

The CQ bit was checked by svaldez@chromium.org

[commit-bot: I haz the power, 2016-04-12 20:00:19]

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

[commit-bot: I haz the power, 2016-04-12 21:56:15]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-04-12 21:58:52]

Description was changed from

==========
Fixing BoringSSL on iOS

This adds the final changes to cert_verify_proc_ios and various tests
in order to prepare for the switchover from NSS to BoringSSL on iOS.

BUG=591545
==========

to

==========
Fixing BoringSSL on iOS

This adds the final changes to cert_verify_proc_ios and various tests
in order to prepare for the switchover from NSS to BoringSSL on iOS.

BUG=591545

Committed: https://crrev.com/b7f886b3d364497ff0422e9808d8ad04ffcb5c4d
Cr-Commit-Position: refs/heads/master@{#386819}
==========

[commit-bot: I haz the power, 2016-04-12 21:58:53]

Patchset 15 (id:??) landed as
https://crrev.com/b7f886b3d364497ff0422e9808d8ad04ffcb5c4d
Cr-Commit-Position: refs/heads/master@{#386819}

[Eugene But (OOO till 7-30), 2016-04-14 17:49:11]

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

[Eugene But (OOO till 7-30), 2016-04-14 17:49:13]

https://codereview.chromium.org/1871043003/diff/280001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_ios.cc (right):

https://codereview.chromium.org/1871043003/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_ios.cc:201: CFStringRef date_error =
CFBundleCopyLocalizedString(
This is a memory leak (and everything else returned from
CFBundleCopyLocalizedString.