Refactor WebCrypto code

Refactor WebCrypto code.

Split up the monstrously large platform_crypto_nss.cc, platform_crypto_openssl.cc into multiple files.

-----------
Overview:
-----------

* algorithm_implementation.h:

This defines a base class AlgorithmImplementation, which has virtual methods for synchronous encrypt/decrypt/generatekey. All of the information about an algorithm is now encapsulated by an AlgorithmImplementation. So for instance the JWK specific knowledge, key usages for each key type are pulled into this interface.

* algorithm_registry.cc:

Contains a mapping from WebCryptoAlgorithmID --> AlgorithmImplementation, stored by a singleton.

* algorithm_dispatch.cc:

Given parameters from Blink, looks up the appropriate AlgorithmImplementation in the registry and dispatches the operation. Also implements wrap/unwrap in terms of encrypt/decrypt.

* structured_clone.cc:

Contains the code related to structured cloning (which still needs some cleanup, and is implemented in terms of import/export).

* nss/*, openssl/*:

Contains the AlgorithmImplementation concrete classes for each algorithm.

This reorganization also unintentionally fixes a few bugs.

  * ExportKey() for spki/pkcs8/raw uses the already serialized key data rather than re-exporting
  * Some exception codes were fixed.

BUG=389325, 389342, 389327, 374912
R=jochen@chromium.org, rsleevi@chromium.org

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

[Ryan Sleevi, 2014-07-10 23:20:54]

Quick scan. Very limited.

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
File content/child/webcrypto/algorithm.h (right):

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:37: class AlgorithmImplementation {
We generally encourage pure virtual interfaces when describing this.

Here, you've provided default values for everything, which is a bit odd, though
understandable.

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:56: virtual Status VerifySignature(const
blink::WebCryptoAlgorithm& algorithm,
Why is this VerifySignature, when the corresponding WebCrypto method is "Verify"
(a name chosen because it also verifies MACs, which are not signatures)

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:67: blink::WebCryptoAlgorithmId algorithm)
const;
This feels like it's at the wrong layer.

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:133: blink::WebCryptoKey* key) const;
Not sure why this multitude of methods, rather than having an enum for format,
and letting the implementation handle format dispatch internally.

Ditto export key.

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
File content/child/webcrypto/algorithm_registry.h (right):

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
content/child/webcrypto/algorithm_registry.h:18: const AlgorithmImplementation**
impl);
This leaves me unclear what the lifetime semantics are for
AlgorithmImplementations

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
File content/child/webcrypto/jwk.h (right):

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
content/child/webcrypto/jwk.h:68: std::string* qi);
Feels like these properties should be rolled into a helper-struct type.

[eroman, 2014-07-11 00:27:36]

Thanks for the initial comments. I will start work on translating OpenSSL code
and ping when I am ready for an uber-review.

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
File content/child/webcrypto/algorithm.h (right):

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:56: virtual Status VerifySignature(const
blink::WebCryptoAlgorithm& algorithm,
On 2014/07/10 23:20:54, Ryan Sleevi wrote:
> Why is this VerifySignature, when the corresponding WebCrypto method is
"Verify"
> (a name chosen because it also verifies MACs, which are not signatures)

History: the Blink name for the method is verifySignature() because verify()
collided with a macro on MAC OS X.

I should be able to call this Verify() without such a collision, will try the
change.

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:67: blink::WebCryptoAlgorithmId algorithm)
const;
On 2014/07/10 23:20:54, Ryan Sleevi wrote:
> This feels like it's at the wrong layer.

OK I'll try to move this out. It was convenient having it here since it avoided
needing to define extra headers and openssl vs blink dispatches.

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:133: blink::WebCryptoKey* key) const;
On 2014/07/10 23:20:54, Ryan Sleevi wrote:
> Not sure why this multitude of methods, rather than having an enum for format,
> and letting the implementation handle format dispatch internally.
> 
> Ditto export key.

This is about reducing code duplication.

Without a central dispatch, every implementation of AlgorithmImplementation ends
up needing to do something like:

switch (format) {
  case Spki:
    return ImportSpki(...);
  case Pkcs8:
    return ImportPkcs8(...);
  case Jwk:
    return ImportJwk(...);
  default:
    return Status::ErrorUnsupportedImportKeyFormat();
}

Not the worst thing in the world, but something we can avoid.

Do you want me to change it?

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
File content/child/webcrypto/algorithm_registry.h (right):

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
content/child/webcrypto/algorithm_registry.h:18: const AlgorithmImplementation**
impl);
On 2014/07/10 23:20:54, Ryan Sleevi wrote:
> This leaves me unclear what the lifetime semantics are for
> AlgorithmImplementations

The AlgorithmImplementations are threadsafe singletons that are constructed on
the first webcrypto operation. They only contain const methods and are glorified
vtables.

I will see if there is a clearer way to indicate this. One possibility is to
remove algorithm_registry.{h,cc} and move the singleton as an implementation
detail of algorithm_dispatch.cc.

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
File content/child/webcrypto/jwk.h (right):

https://codereview.chromium.org/379383002/diff/110001/content/child/webcrypto...
content/child/webcrypto/jwk.h:68: std::string* qi);
On 2014/07/10 23:20:54, Ryan Sleevi wrote:
> Feels like these properties should be rolled into a helper-struct type.

Good idea.

[eroman, 2014-07-11 23:08:40]

This is ready for review now (added the OpenSSL bits in).

I won't be splitting the unit-tests as part of this refactor.

[Ryan Sleevi, 2014-07-12 00:55:28]

I tried plowing through this more, but ran out of steam midway through the /nss
files.

Another batch of comments, rather than waiting until I've reviewed it all.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
File content/child/webcrypto/algorithm.h (right):

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:37: class AlgorithmImplementation {
All of these methods should be documented.

It may be sufficient to simply say something like

// The following methods are responsible for performing the algorithm-specific
operations of the same name, as defined in web-crypto

or something less... awful.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:69: // GenerateKeyPair().
This is documenting how some other class uses this interface, which is not
generally appropriate.

Instead, document what is expected of callers, including any preconditions

e.g. VerifyKeyUsagesBeforeGenerateKey MUST be called before GenerateKeyPair

Also document what a derived implementation is expected to do in these methods.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:96: // importing a key, or unwrapping a key.
Again, layering.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:100: // this case the import function will
be responsible for checking the usage.
This belongs in ImportKeyJwk. It's unclear from your documentation, for example,
if VerifyKeyUsagesBeforeImportKey is expected to be called for JWK keys, and if
so, what the |format| value will be.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
File content/child/webcrypto/algorithm_dispatch.h (right):

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/algorithm_dispatch.h:26: // by blink can be assumed:
inconsistent capitalization of Blink (25 vs 26)

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
File content/child/webcrypto/algorithm_registry.h (right):

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/algorithm_registry.h:26: const AlgorithmImplementation**
impl);
How is this model supposed to support IPC-based crypto in the future?

That is, you're (indirectly) continuing a Singleton, where before, there was a
little bit of distinction (via PlatformCrypto having the virtual methods)

I'm having trouble convincing myself this is an idea we'll grow with, and wonder
whether the implementations shouldn't hang off a context object that is passed
around.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
File content/child/webcrypto/jwk.cc (right):

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:242: utf8_bytes->assign(json.data(), json.data()
+ json.size());
Why don't you need to cast here?

json is std::string, meaning data is char*
utf8_bytes is vector<uint8>

If you're able to get away without casting, can you not use json.begin(),
json.end()?

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:437: return
Status::ErrorJwkAlgorithmInconsistent();
Why do you place this check here, but then you return an std::string on line
440, suggesting the caller still needs to check if empty(), meaning no alg
present?

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:470: ignore_result(value.release());
This needs more documentation, because I had serious read flags and had to check
the API contracts here, because it "looked" like a UAF

// Release |value|, as ownership will be transferred to |dict| via
// |dict_value|, which points to the same object as |value|.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:569: "",
std::string()

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:588:
MakeJwkAesAlgorithmName(algorithm_name_suffix, 32)) {
Doesn't this seem like it should be up to the implementation to determine
whether these key sizes are valid?

That is, you seem to be trying to keep knowledge about the set of valid
algorithm names out of this, but then have this?

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
File content/child/webcrypto/nss/aes_key_nss.h (right):

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/nss/aes_key_nss.h:27: AesAlgorithm(CK_MECHANISM_TYPE
import_mechanism, const char* jwk_suffix);
Is this really a suffix or a prefix? And what's the lifetime here? I fear you
potentially allow stack-allocated variables in, which then cause lifetime issues
on line 63. Why not make it explicit with std::string(), and let the small
string optimization amortize any perceived performance overheads?

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/nss/aes_key_nss.h:36: blink::WebCryptoKeyUsageMask
usage_mask) const OVERRIDE;
Please keep the ordering the same on all derived methods. This is out of order
with the main Algorithm.

https://codereview.chromium.org/379383002/diff/210055/content/child/webcrypto...
File content/child/webcrypto/algorithm.h (right):

https://codereview.chromium.org/379383002/diff/210055/content/child/webcrypto...
content/child/webcrypto/algorithm.h:6: #define
CONTENT_CHILD_WEBCRYPTO_CRYPTO_ALGORITHM_H_
This file should be ALGORITHM_IMPLEMENTATION_H / AlgorithmImplementation.h to
match the class.

[eroman, 2014-07-12 01:59:31]

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
File content/child/webcrypto/algorithm.h (right):

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:37: class AlgorithmImplementation {
On 2014/07/12 00:55:27, Ryan Sleevi wrote:
> All of these methods should be documented.
> 
> It may be sufficient to simply say something like
> 
> // The following methods are responsible for performing the algorithm-specific
> operations of the same name, as defined in web-crypto
> 
> or something less... awful.

Done.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:69: // GenerateKeyPair().
On 2014/07/12 00:55:27, Ryan Sleevi wrote:
> This is documenting how some other class uses this interface, which is not
> generally appropriate.
> 
> Instead, document what is expected of callers, including any preconditions
> 
> e.g. VerifyKeyUsagesBeforeGenerateKey MUST be called before GenerateKeyPair


Done.

> Also document what a derived implementation is expected to do in these
methods.

Done

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:96: // importing a key, or unwrapping a key.
On 2014/07/12 00:55:27, Ryan Sleevi wrote:
> Again, layering.

Done.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/algorithm.h:100: // this case the import function will
be responsible for checking the usage.
On 2014/07/12 00:55:27, Ryan Sleevi wrote:
> This belongs in ImportKeyJwk. It's unclear from your documentation, for
example,
> if VerifyKeyUsagesBeforeImportKey is expected to be called for JWK keys, and
if
> so, what the |format| value will be.

Modified the comment. In the case of JWK, VerifyKeyUsagesBeforeImportKey() is
called with format=JWK.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
File content/child/webcrypto/algorithm_dispatch.h (right):

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/algorithm_dispatch.h:26: // by blink can be assumed:
On 2014/07/12 00:55:27, Ryan Sleevi wrote:
> inconsistent capitalization of Blink (25 vs 26)

Done.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
File content/child/webcrypto/algorithm_registry.h (right):

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/algorithm_registry.h:26: const AlgorithmImplementation**
impl);
On 2014/07/12 00:55:27, Ryan Sleevi wrote:
> How is this model supposed to support IPC-based crypto in the future?
> 
> That is, you're (indirectly) continuing a Singleton, where before, there was a
> little bit of distinction (via PlatformCrypto having the virtual methods)
> 
> I'm having trouble convincing myself this is an idea we'll grow with, and
wonder
> whether the implementations shouldn't hang off a context object that is passed
> around.

This is no different than before. Previously the platform crypto layer was just
a set of global functions so this isn't weaking any IPC handling.

The layering right now goes like:

(1) blink --> (2) WebCryptoImpl --> (3) post to crypto thread --> (4)
algorithm_dispatch.cc --> (5) AlgorithmImplementation

Depending what we want to achieve with the browser side crypto, I could imagine
a desin where you modify step (2), so instead of posting to the crypto thread it
serializes the blink::WebCryptoAlgorithm and calls an async IPC. The browser
side would then unmarshal the algorithm and be ready to resume things at step
(2)

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
File content/child/webcrypto/jwk.cc (right):

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:242: utf8_bytes->assign(json.data(), json.data()
+ json.size());
On 2014/07/12 00:55:27, Ryan Sleevi wrote:
> Why don't you need to cast here?
> 
> json is std::string, meaning data is char*
> utf8_bytes is vector<uint8>
> 
> If you're able to get away without casting, can you not use json.begin(),
> json.end()?

Done.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:437: return
Status::ErrorJwkAlgorithmInconsistent();
On 2014/07/12 00:55:27, Ryan Sleevi wrote:
> Why do you place this check here, but then you return an std::string on line
> 440, suggesting the caller still needs to check if empty(), meaning no alg
> present?

Thanks for pointing this out, the current structure is in fact quite difficult
to read.

Basically I did witchcraft which allows calling VerifyAlg() in one of two modes:

 (1) (expected_algorithm="", actual_algorithm=XXXX)
 (2) (expected_algorithm=<value>, actual_algorithm=NULL)

Mode 2 means that if "alg" is present in the JWK, it must match
expected_algorithm, otherwise fail with inconsistent algorithm. This is the
simple case.

Mode 1 is used for AES algorithms, and means "get the value of alg". If an "alg"
was present, it is written back to actual_algorithm. If not actual_algorithm is
set to "". If "alg" actually was the empty string, then an error is returned, to
avoid it being interpreted as no algorithm.

I will split these up into separate functions, so there is no need to explain in
detail how the parameters work.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:470: ignore_result(value.release());
On 2014/07/12 00:55:27, Ryan Sleevi wrote:
> This needs more documentation, because I had serious read flags and had to
check
> the API contracts here, because it "looked" like a UAF
> 
> // Release |value|, as ownership will be transferred to |dict| via
> // |dict_value|, which points to the same object as |value|.

Done.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:588:
MakeJwkAesAlgorithmName(algorithm_name_suffix, 32)) {
On 2014/07/12 00:55:27, Ryan Sleevi wrote:
> Doesn't this seem like it should be up to the implementation to determine
> whether these key sizes are valid?
> 
> That is, you seem to be trying to keep knowledge about the set of valid
> algorithm names out of this, but then have this?

Indeed. Initially I had the code as part of the implementation, and had jwk.cc
simply return the value of "alg":

https://codereview.chromium.org/379383002/diff/160001/content/child/webcrypto...

Once I added the OpenSSL code I found myself duplicating that same code. So I
moved the code to a common location so it could be shared between openssl/nss,
and I figured that location should be jwk.cc

I would like both the RSA JWK parsing, and AES JWK parsing to be part of the
AlgorithmImplementation code once the NSS code is deleted, perhaps a TODO in the
meantime?

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
File content/child/webcrypto/nss/aes_key_nss.h (right):

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/nss/aes_key_nss.h:27: AesAlgorithm(CK_MECHANISM_TYPE
import_mechanism, const char* jwk_suffix);
On 2014/07/12 00:55:27, Ryan Sleevi wrote:
> Is this really a suffix or a prefix? 

A128CBC --> "CBC" is the jwk_suffix expected here.
Similarly A192KW --> "KW" is the jwk_suffix

And what's the lifetime here? I fear you
> potentially allow stack-allocated variables in, which then cause lifetime
issues
> on line 63. Why not make it explicit with std::string(), and let the small
> string optimization amortize any perceived performance overheads?

Done

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/nss/aes_key_nss.h:36: blink::WebCryptoKeyUsageMask
usage_mask) const OVERRIDE;
On 2014/07/12 00:55:27, Ryan Sleevi wrote:
> Please keep the ordering the same on all derived methods. This is out of order
> with the main Algorithm.

Acknowledged.

[eroman, 2014-07-14 21:56:10]

Uploaded a new patchset with the fixes.

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
File content/child/webcrypto/jwk.cc (right):

https://codereview.chromium.org/379383002/diff/200001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:437: return
Status::ErrorJwkAlgorithmInconsistent();
On 2014/07/12 01:59:30, eroman wrote:
> On 2014/07/12 00:55:27, Ryan Sleevi wrote:
> > Why do you place this check here, but then you return an std::string on line
> > 440, suggesting the caller still needs to check if empty(), meaning no alg
> > present?
> 
> Thanks for pointing this out, the current structure is in fact quite difficult
> to read.
> 
> Basically I did witchcraft which allows calling VerifyAlg() in one of two
modes:
> 
>  (1) (expected_algorithm="", actual_algorithm=XXXX)
>  (2) (expected_algorithm=<value>, actual_algorithm=NULL)
> 
> Mode 2 means that if "alg" is present in the JWK, it must match
> expected_algorithm, otherwise fail with inconsistent algorithm. This is the
> simple case.
> 
> Mode 1 is used for AES algorithms, and means "get the value of alg". If an
"alg"
> was present, it is written back to actual_algorithm. If not actual_algorithm
is
> set to "". If "alg" actually was the empty string, then an error is returned,
to
> avoid it being interpreted as no algorithm.
> 
> I will split these up into separate functions, so there is no need to explain
in
> detail how the parameters work.

Done.

[Ryan Sleevi, 2014-07-17 00:06:55]

Another pass, but the monstrosity of the CL makes me confident I may have missed
some things. I'm also not sure how this will interface with the BoringSSL
changes, but I'm assuming you're staying on top of those.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/aes_gcm_nss.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/aes_gcm_nss.cc:104: return
Status::ErrorDataTooLarge();
base::CheckedNumeric<> for a follow-up if you want.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/aes_gcm_nss.cc:124: PK11_EncryptDecryptFunction func
=
pedantry: encrypt_or_decrypt_function?

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/aes_key_nss.h (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/aes_key_nss.h:23: const std::string& jwk_suffix);
Document what these parameters mean. jwk_suffix in particular.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/aes_key_nss.h:28: const std::string& jwk_suffix);
document the other parameters.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/aes_kw_nss.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/aes_kw_nss.cc:28: Status DoUnwrapSymKeyAesKw(const
CryptoData& wrapped_key_data,
Document why this exists as working on a SymKey (141-142 notes it, but I should
be able to read the function, rather than its use, to determine its
purpose/design choices)

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/hmac_nss.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/hmac_nss.cc:201: DCHECK_EQ(buffer->size(),
signature_item.len);
What is this DCHECK measuring? The caller of this function doesn't affect this.
It seems like you're trying ot (rightly) defend against NSS bugs, in which case,
should this be a CHECK()? Seems it'd be security relevant if the two weren't
equal.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/rsa_key_nss.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_key_nss.cc:29: // TODO(eroman): Fix handling of
empty biginteger.
BUG #?

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_key_nss.cc:412: //                   
http://crbug.com/378315
Can you do this now, for all non-Linux platforms?

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_key_nss.cc:482: DCHECK(spki_der->len);
Also questioning whether this should be CHECK.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_key_nss.cc:730: // TODO(eroman): This is
probably going to be the same as the input.
Not sure what this comment means, but no, it's not guaranteed it will output the
same data as the input (nor would we necessarily want it to, since we're liberal
on acceptable import data)

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_key_nss.cc:784: std::vector<uint8> spki_data;
ditto

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/rsa_key_nss.h (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_key_nss.h:23: blink::WebCryptoKeyUsageMask
all_private_key_usages)
Document

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/rsa_oaep_nss.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_oaep_nss.cc:113: DCHECK_LE(output_len,
buffer->size());
CHECK?

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_oaep_nss.cc:156: DCHECK_LE(output_len,
buffer->size());
CHECK?

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/sha_nss.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/sha_nss.cc:39: class DigestorNSS : public
blink::WebCryptoDigestor {
document

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/util_nss.h (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/util_nss.h:57: // Singleton to abstract away
dynamically loading libnss3.so
No it doesn't.

Document.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/openssl/aes_key_openssl.h (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/openssl/aes_key_openssl.h:23: explicit
AesAlgorithm(const std::string& jwk_suffix);
Document (in particular, jwk_suffix)

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/openssl/hmac_openssl.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/openssl/hmac_openssl.cc:44: // case. Handle this
specific case by pointing to an empty array.
Fixed by BoringSSL?

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/openssl/util_openssl.h (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/openssl/util_openssl.h:8: #include <openssl/ossl_typ.h>
Not really needed, is it Can just forward declare EVP_MD.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/webcrypto_util.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/webcrypto_util.cc:194: *tag_length_bits != 128)
Document where these values come from.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/webcrypto_util.cc:207: if (*keylen_bits == 192)
Document why.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/webcrypto_util.cc:214: unsigned int* keylen_bits) {
GetHmacKeyGenLengthInBits?

keylen_bits won't be a variable at the callsite.

[Ryan Sleevi, 2014-07-17 00:08:26]

Oh, I should have added a more general request:

You're very sparse on documentation. I know that was/is an old blink thing, but
I've found it very hard to review what some of the types are.

In general, I think some documentation on the base interfaces (i.e. the virtual
bits), as well as special notes for things like the constructors and some of
their parameters. Also, for some of the helpers. Additionally, inline comments
whenever something 'weird' happens, such as a check - why is that check there,
is it arbitrary or spec wise, etc.

[eroman, 2014-07-17 20:37:27]

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/aes_gcm_nss.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/aes_gcm_nss.cc:104: return
Status::ErrorDataTooLarge();
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> base::CheckedNumeric<> for a follow-up if you want.

Will follow up through:
https://code.google.com/p/chromium/issues/detail?id=394821

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/aes_gcm_nss.cc:124: PK11_EncryptDecryptFunction func
=
On 2014/07/17 00:06:53, Ryan Sleevi wrote:
> pedantry: encrypt_or_decrypt_function?

Done.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/aes_key_nss.h (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/aes_key_nss.h:23: const std::string& jwk_suffix);
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> Document what these parameters mean. jwk_suffix in particular.

Done. Documented as:

  // Constructs an AES algorithm whose keys will be imported using the NSS
  // mechanism |import_mechanism| and NSS flags |import_flags|.
  // |all_key_usages| is the set of all WebCrypto key usages that are
  // allowed for imported or generated keys. |jwk_suffix| is the suffix
  // used when constructing JWK names for the algorithm. For instance A128CBC
  // is the JWK name for 128-bit AES-CBC. The |jwk_suffix| in this case would
  // be "CBC".

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/aes_key_nss.h:28: const std::string& jwk_suffix);
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> document the other parameters.
Done. Documented as:

  // This is the same as other AesAlgorithm constructor, however |import_flags|
  // and |all_key_usages| are pre-filled to values for encryption/decryption
  // algorithms (supports usages for: encrypt, decrypt, wrap, unwrap).

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/aes_kw_nss.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/aes_kw_nss.cc:28: Status DoUnwrapSymKeyAesKw(const
CryptoData& wrapped_key_data,
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> Document why this exists as working on a SymKey (141-142 notes it, but I
should
> be able to read the function, rather than its use, to determine its
> purpose/design choices)

Done. Documented as:

// The result of unwrapping is a SymKey rather than a buffer. This is a
// consequence of how NSS exposes AES-KW. Subsequent code can extract the value
// of the sym key to interpret it as key bytes in another format.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/hmac_nss.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/hmac_nss.cc:201: DCHECK_EQ(buffer->size(),
signature_item.len);
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> What is this DCHECK measuring? The caller of this function doesn't affect
this.
> It seems like you're trying ot (rightly) defend against NSS bugs, in which
case,
> should this be a CHECK()? Seems it'd be security relevant if the two weren't
> equal.

Done, changed to CHECK().

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/rsa_key_nss.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_key_nss.cc:29: // TODO(eroman): Fix handling of
empty biginteger.
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> BUG #?

Done. Added http://crubg.com/373552

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_key_nss.cc:412: //                   
http://crbug.com/378315
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> Can you do this now, for all non-Linux platforms?

This is already done. I have removed the TODO.

Note that what shows up as additions in this mega CL are primarily just moves of
existing code (I tried to change as little as possible).

That said, thanks for pointing out this problem!

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_key_nss.cc:482: DCHECK(spki_der->len);
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> Also questioning whether this should be CHECK.

Removed the DCHECKs (they provide little value to begin with).

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/rsa_key_nss.h (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_key_nss.h:23: blink::WebCryptoKeyUsageMask
all_private_key_usages)
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> Document

Done. I documented the class as:

// Base class for an RSA algorithm whose keys additionaly have a hash parameter
// bound to them. Provides functionality for generating, importing, and
// exporting keys.


And the constructor as:

  // Constructs an RSA algorithm which will use the NSS flags |generate_flags|
  // when generating keys. |all_public_key_usages| and |all_private_key_usages|
  // are the set of WebCrypto key usages that are valid for created keys
  // (public and private respectively).

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/rsa_oaep_nss.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_oaep_nss.cc:113: DCHECK_LE(output_len,
buffer->size());
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> CHECK?

Done.

Thanks for reading through everything in detail, including the unchanged code
from before!

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_oaep_nss.cc:156: DCHECK_LE(output_len,
buffer->size());
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> CHECK?

Done.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/sha_nss.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/sha_nss.cc:39: class DigestorNSS : public
blink::WebCryptoDigestor {
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> document

Done: Documented as:

// Multipart input SHA digestor.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/util_nss.h (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/util_nss.h:57: // Singleton to abstract away
dynamically loading libnss3.so
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> No it doesn't.
> 
> Document.

Changed to:

// Singleton to do feature detection for AES-GCM and RSA-OAEP.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/openssl/aes_key_openssl.h (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/openssl/aes_key_openssl.h:23: explicit
AesAlgorithm(const std::string& jwk_suffix);
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> Document (in particular, jwk_suffix)

Done. Documented as:

  // This is the same as the other AesAlgorithm constructor where
|all_key_usages|
  // is pre-filled to values for encryption/decryption algorithms (supports
  // usages for: encrypt, decrypt, wrap, unwrap).

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/openssl/hmac_openssl.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/openssl/hmac_openssl.cc:44: // case. Handle this
specific case by pointing to an empty array.
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> Fixed by BoringSSL?

Not sure. Will add it as an action item to investigate.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/openssl/util_openssl.h (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/openssl/util_openssl.h:8: #include <openssl/ossl_typ.h>
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> Not really needed, is it Can just forward declare EVP_MD.

How would the forward declaration look? I tried adding:

struct EVP_MD;

But got a definition conflict.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/webcrypto_util.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/webcrypto_util.cc:194: *tag_length_bits != 128)
On 2014/07/17 00:06:55, Ryan Sleevi wrote:
> Document where these values come from.

Done. Documented as:

// The WebCrypto spec defines the default value for the tag length, as well as
// the allowed values for tag length.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/webcrypto_util.cc:207: if (*keylen_bits == 192)
On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> Document why.

Done. Documented as:

// BoringSSL does not support 192-bit AES.

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/webcrypto_util.cc:214: unsigned int* keylen_bits) {
On 2014/07/17 00:06:55, Ryan Sleevi wrote:
> GetHmacKeyGenLengthInBits?
> 
> keylen_bits won't be a variable at the callsite.

Done.

[Ryan Sleevi, 2014-07-17 22:42:56]

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/sha_nss.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/sha_nss.cc:39: class DigestorNSS : public
blink::WebCryptoDigestor {
On 2014/07/17 20:37:26, eroman wrote:
> On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> > document
> 
> Done: Documented as:
> 
> // Multipart input SHA digestor.

Doesn't really help, at least for my novel reading.

// Implementation of blink::WebCryptoDigester, an internal
// Blink detail not part of WebCrypto, that allows chunks
// of data to be streamed in before computing a SHA-1
// digest (as opposed to ShaImplementation, which computes
// digests over complete messages)

It does surprise me that it's in a directory called "webcrypto", since this
interface is not part of WebCrypto. Can leave for follow-up CL?

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
File content/child/webcrypto/algorithm_implementation.h (right):

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
content/child/webcrypto/algorithm_implementation.h:47: std::vector<uint8>*
buffer) const;
future CL update: uint8 is deprecated (and you're missing the #include of the
correct types, base/basictypes.h)

uint8_t is the approved hotness, with a <stdint.h> #include.

For now, IWYU basictypes, in the future, nuke for <stdint.h>

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
File content/child/webcrypto/jwk.h (right):

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
content/child/webcrypto/jwk.h:23: void WriteSecretKeyJwk(const CryptoData&
raw_key_data,
Documentation for all these.

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
content/child/webcrypto/jwk.h:64: struct JwkRsaInfo {
Document, including noting, for example, that we don't support multi-prime RSA.
(hence no othinfo)

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
File content/child/webcrypto/nss/rsa_key_nss.h (right):

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_key_nss.h:27: // (public and private
respectively).
pedantry: Can you provide an example of what an all_public_key_usages would look
like?

You just mean like if an algorithm only supports encrypt/decrypt, it'll be the
OR of them, right?

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
File content/child/webcrypto/nss/util_nss.h (right):

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
content/child/webcrypto/nss/util_nss.h:57: // Singleton to do feature detection
for AES-GCM and RSA-OAEP.
// Singleton that detects whether or not AES-GCM and
// RSA-OAEP are supported by the version of NSS being used.
// On non-Linux platforms, Chromium embedders ship with a
// fixed version of NSS, and these are always available.
// However, on Linux (and ChromeOS), NSS is provided by the
// system, and thus not all algorithms may be available
// or be safe to use.

[eroman, 2014-07-17 23:33:24]

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
File content/child/webcrypto/nss/sha_nss.cc (right):

https://codereview.chromium.org/379383002/diff/320001/content/child/webcrypto...
content/child/webcrypto/nss/sha_nss.cc:39: class DigestorNSS : public
blink::WebCryptoDigestor {
On 2014/07/17 22:42:55, Ryan Sleevi wrote:
> On 2014/07/17 20:37:26, eroman wrote:
> > On 2014/07/17 00:06:54, Ryan Sleevi wrote:
> > > document
> > 
> > Done: Documented as:
> > 
> > // Multipart input SHA digestor.
> 
> Doesn't really help, at least for my novel reading.
> 
> // Implementation of blink::WebCryptoDigester, an internal
> // Blink detail not part of WebCrypto, that allows chunks
> // of data to be streamed in before computing a SHA-1
> // digest (as opposed to ShaImplementation, which computes
> // digests over complete messages)
> 
> It does surprise me that it's in a directory called "webcrypto", since this
> interface is not part of WebCrypto. Can leave for follow-up CL?

Done. Will followup in issue 394945.

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
File content/child/webcrypto/algorithm_implementation.h (right):

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
content/child/webcrypto/algorithm_implementation.h:47: std::vector<uint8>*
buffer) const;
On 2014/07/17 22:42:55, Ryan Sleevi wrote:
> future CL update: uint8 is deprecated (and you're missing the #include of the
> correct types, base/basictypes.h)
> 
> uint8_t is the approved hotness, with a <stdint.h> #include.
> 
> For now, IWYU basictypes, in the future, nuke for <stdint.h>

Will follow up in issue 394944

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
File content/child/webcrypto/jwk.h (right):

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
content/child/webcrypto/jwk.h:23: void WriteSecretKeyJwk(const CryptoData&
raw_key_data,
On 2014/07/17 22:42:55, Ryan Sleevi wrote:
> Documentation for all these.

Done.

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
content/child/webcrypto/jwk.h:64: struct JwkRsaInfo {
On 2014/07/17 22:42:55, Ryan Sleevi wrote:
> Document, including noting, for example, that we don't support multi-prime
RSA.
> (hence no othinfo)

Done.

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
File content/child/webcrypto/nss/rsa_key_nss.h (right):

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
content/child/webcrypto/nss/rsa_key_nss.h:27: // (public and private
respectively).
On 2014/07/17 22:42:55, Ryan Sleevi wrote:
> pedantry: Can you provide an example of what an all_public_key_usages would
look
> like?
> 
> You just mean like if an algorithm only supports encrypt/decrypt, it'll be the
> OR of them, right?

Correct. I have added the additional comment:

  // For instance if public keys support encryption and wrapping, and private
  // keys support decryption and unwrapping callers should set:
  //    all_public_key_usages = UsageEncrypt | UsageWrap
  //    all_private_key_usages = UsageDecrypt | UsageUnwrap
  // This information is used when importing or generating keys, to enforce
  // that only valid key usages are allowed.

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
File content/child/webcrypto/nss/util_nss.h (right):

https://codereview.chromium.org/379383002/diff/340001/content/child/webcrypto...
content/child/webcrypto/nss/util_nss.h:57: // Singleton to do feature detection
for AES-GCM and RSA-OAEP.
On 2014/07/17 22:42:55, Ryan Sleevi wrote:
> // Singleton that detects whether or not AES-GCM and
> // RSA-OAEP are supported by the version of NSS being used.
> // On non-Linux platforms, Chromium embedders ship with a
> // fixed version of NSS, and these are always available.
> // However, on Linux (and ChromeOS), NSS is provided by the
> // system, and thus not all algorithms may be available
> // or be safe to use.

Done.

[Ryan Sleevi, 2014-07-17 23:38:54]

Last review was a LGTM, mod those nits.

[eroman, 2014-07-18 00:00:56]

@jochen: Can you grant OWNERS approval for the deletion in
content/child/blink_platform_impl.cc ? Thanks!

[jochen (gone - plz use gerrit), 2014-07-18 09:12:56]

lgtm

[eroman, 2014-07-18 18:10:57]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-07-18 18:14:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eroman@chromium.org/379383002/400001

[commit-bot: I haz the power, 2014-07-18 20:19:40]

Change committed as 284192

[eroman, 2014-07-18 22:25:13]

Note: I reverted this in r284215, due to a failing test on one bot:

  DigestSampleSetsInChunks

I believe the problem is that the Digestor goes through a codepath which does
not initialize NSS first. So if the test is run individually it will fail.

[eroman, 2014-07-18 23:39:16]

Will try re-landing with the call to init for digestor!

[eroman, 2014-07-18 23:39:28]

The CQ bit was checked by eroman@chromium.org

[eroman, 2014-07-18 23:39:37]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2014-07-18 23:39:58]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-07-18 23:41:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eroman@chromium.org/379383002/480001

[eroman, 2014-07-19 00:55:04]

Committed patchset #25 manually as r284267 (presubmit successful).