Fix VariationsSeedStoreTest.VerifySeedSignature in the OpenSSL build.

Fix VariationsSeedStoreTest.VerifySeedSignature in the OpenSSL build.

Unlike NSS, which decodes the signature in VerifyInit, OpenSSL decodes the
signature late, after digesting the data, and doesn't distinguish signature
decode failure from signature mismatch. Adjust test expectations for this.

This does not appear to have any practical impact on our code except that some
histogram buckets will merge.

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

[Alexei Svitkine (slow), 2014-07-23 21:03:13]

Hmm, how did this work before? I thought we used OpenSSL on Android and these
tests have passed there?

[davidben, 2014-07-23 21:03:57]

On 2014/07/23 21:03:13, Alexei Svitkine wrote:
> Hmm, how did this work before? I thought we used OpenSSL on Android and these
> tests have passed there?

See line 186. The tests are ifdef'd out on Android (and iOS) with a comment:
  // Signature verification is not enabled on mobile.

[Alexei Svitkine (slow), 2014-07-23 21:10:06]

On 2014/07/23 21:03:57, David Benjamin wrote:
> On 2014/07/23 21:03:13, Alexei Svitkine wrote:
> > Hmm, how did this work before? I thought we used OpenSSL on Android and
these
> > tests have passed there?
> 
> See line 186. The tests are ifdef'd out on Android (and iOS) with a comment:
>   // Signature verification is not enabled on mobile.

Ah, right. That makes sense.

Can you update the VariationSeedSignature enum in histograms.xml to change
"Invalid Seed" to something like "Invalid Seed or Signature".

Is it not possible to disambiguate the cases with OpenSSL? This histogram
provides some visibility into what software in the wild that might be tampering
with these prefs is trying to do, so being able to differentiate the cases is at
least somewhat useful. (If it's really infeasible, probably we can live with it
- but ideally we want to have this insight.)

[davidben, 2014-07-23 21:41:19]

On 2014/07/23 21:10:06, Alexei Svitkine wrote:
> On 2014/07/23 21:03:57, David Benjamin wrote:
> > On 2014/07/23 21:03:13, Alexei Svitkine wrote:
> > > Hmm, how did this work before? I thought we used OpenSSL on Android and
> these
> > > tests have passed there?
> > 
> > See line 186. The tests are ifdef'd out on Android (and iOS) with a comment:
> >   // Signature verification is not enabled on mobile.
> 
> Ah, right. That makes sense.
> 
> Can you update the VariationSeedSignature enum in histograms.xml to change
> "Invalid Seed" to something like "Invalid Seed or Signature".

Changed it to "Invalid Signature for Seed" which is somewhat more accurate (see
below) and matches the "Valid Signature for Seed" label.

> Is it not possible to disambiguate the cases with OpenSSL? This histogram
> provides some visibility into what software in the wild that might be
tampering
> with these prefs is trying to do, so being able to differentiate the cases is
at
> least somewhat useful. (If it's really infeasible, probably we can live with
it
> - but ideally we want to have this insight.)

We can probably get the bit out by tweaking verifier.VerifyFinal(). Either by
checking the OpenSSL error queue or just seeing if the return value is 0 or -1.
(-1 is used for a number of errors including decode failure, but most of them
shouldn't be able to happen because the public key is hardcoded and I think our
malloc aborts if it fails?) That would require changing the
crypto::SignatureVerifier interface. The current distinction is just based on
whether VerifyInit() or VerifyFinal() failed which isn't the order OpenSSL does
things in.

Ryan, do you have opinions on this? I dunno if the distinction is worth
bothering with. This is the difference between "the signature is not valid for
the seed but at least it parses as a pair for DER-encoded INTEGERs" and "the
signature is not valid for the seed because it is nonsense and doesn't even have
the right structure". (Calling the former "invalid seed" and latter "invalid
signature" is somewhat misleading. The former case could also have been a legit
seed with a signature from the wrong key or has a bit-flip somewhere.)

[Alexei Svitkine (slow), 2014-07-23 21:50:58]

LGTM

I'm OK with not bothering trying to surface this distinction in OpenSSL if it
requires changing our crypto API functions. Thanks for explanation.

[Ryan Sleevi, 2014-07-23 22:03:55]

On 2014/07/23 21:50:58, Alexei Svitkine wrote:
> LGTM
> 
> I'm OK with not bothering trying to surface this distinction in OpenSSL if it
> requires changing our crypto API functions. Thanks for explanation.

Yeah, if Alexei is fine, let's not change. Both the NSS and OpenSSL approach are
valid, so I feel like we should design our code agnostic to that detail.

[davidben, 2014-07-23 22:28:03]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-07-23 22:30:21]

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

[commit-bot: I haz the power, 2014-07-24 11:43:42]

Change committed as 285196