Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11571059:
Add net/android/keystore.h (Closed)
|
Created:
8 years ago by digit1 Modified:
7 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionThis patch adds some Android-support code to allow the network
stack to use platform-specific private key objects to perform
signing in the context of SSL handshakes which require a client
certificate.
More specifically:
- Add net/android/keystore.h, which provides native
functions to operate on JNI references pointing to
java.security.PrivateKey objects provided by the
platform. I.e.:
net::android::GetPrivateKeyType()
net::android::SignWithPrivateKey()
Also provide a function that can get the system's own
EVP_PKEY* handle corresponding to a given PrivateKey
object. This uses reflection and should *only* be used
for RSA private keys when running on Android 4.0 and
4.1, in order to route around a platform bug that was
only fixed in 4.2.
net::android::GetOpenSSLSytstemHandleForPrivateKey()
See the comments in this source file for mode details:
net/android/java/org/chromium/net/AndroidKeyStore.java
- Add net/android/keystore_openssl.h, which provides
a function that can wrap an existing PrivateKey
JNI reference around an OpenSSL EVP_PKEY object
which uses custom DSA/RSA/ECDSA methods to perform
signing as expected to handle client certificates.
net::android::GetOpenSSLPrivateKeyWrapper()
- Add relevant unit tests for the new functions.
Note that the unit test comes with its own Java helper
function, which is used to create a platform PrivateKey
object from encoded PKCS#8 private key data.
This is called from the native unit test, but does not
constitute a new Java test (AndroidKeyStoreTestUtil.java).
- Add corresponding new test key files under
net/data/ssl/certificates/, and their generation
script in net/data/ssl/scripts/.
- Add net/android/private_key_type_list.h which is
used both from C++ and Java to define the list of
supported private key types used by this code.
- Minor improvements: Add a "release()" method to
crypto::ScopedOpenSSL, add missing BASE_EXPORT
to one base/android/jni_array.h function declaration.
BUG=166642
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181741
Patch Set 1 #Patch Set 2 : new version with simpler signing support #
Total comments: 22
Patch Set 3 : fix small issues only #
Total comments: 1
Patch Set 4 : First real implementation - RSA only #
Total comments: 1
Patch Set 5 : Add DSA + ECDSA test keys and signing tests #
Total comments: 30
Patch Set 6 : Add DSA/ECDSA methods + reflection hack for Android 4.0-4.1 #Patch Set 7 : Appease the angry 'findbugs' gods. #
Total comments: 46
Patch Set 8 : PEM data files + RSA_size() / DSA_size() support. #
Total comments: 7
Patch Set 9 : Add ECDSA_size() support + fix checks against DSA_size() + ECDSA_size() #Patch Set 10 : Fix bad include #Patch Set 11 : Simple rebase #Patch Set 12 : fix typo #
Total comments: 6
Patch Set 13 : removed non-const references #
Total comments: 69
Patch Set 14 : addressing ryan's remarks. #
Total comments: 8
Patch Set 15 : addressing style issues #
Total comments: 5
Patch Set 16 : git cl try #
Total comments: 18
Patch Set 17 : address marcus' nits. #Messages
Total messages: 37 (0 generated)
I've updated the CL description, and added Ryan as a reviewer to get his initial input. This is not the final version of this patch though. I'm really tempted to move all the ClientCertificateRequest to the content layer instead. This seems more logical to me, but probably makes the code a bit more difficult to understand.
I am not really comfortable with this approach. 1) net/ should not have any UI-facing components or interaction 2) The requirement of "main application thread" from net/ runs counter to how we've designed net/ (eg: to be 'mostly single threaded' UI interaction should be handled through net/ specifying a common interface, and then embedders implementing that interface to perform the necessary interaction. https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/keystore.h File net/android/keystore.h (right): https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/keystor... net/android/keystore.h:115: std::vector<uint8>* signature); DESIGN: This is not at all clear what type of signature will be generated. Is it ECDSA? PKCS#1 v1.5 RSA-SSA? RSA-OAEP? Custom padding here (eg: as used by SSL)?
Everybody loves a drive-by review on Friday afternoon, right? :) https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:53: if (algorithm == "RSA") Same comment as below about the guarantee of interning the Strings. I wouldn't rely on that; I'd use String.equals. https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:78: // Single signature instances. Used to perform signing with private I'd wrap these comments to the 100-column limit. https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:86: // to a given private key is initialized before a call to signWithPrivateKey Nit: End sentence with a period. I'm sorry to be so nit-picky; I'm just some random guy torturing you with a drive-by review. :( https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:91: if (algorithm == "RSA" && sRsaSignature == null) == isn't *guaranteed* to work, is it? I'd use "RSA".equals(algorithm) to be certain. (Is Dalvik guaranteed to perform the optimization that makes it work? I'm curious.) https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:106: // is closed while an asynchrouns request is still pending, the native Typo: asynchronous https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:121: // To solve this, start an AsyncTask when we an alias is received. "when we" --> "when" https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:218: byte[][] encodedPrincipals, Mixing naming conventions: key_types, encodedPrincipals. Note also that it's called "principals" in the Javadoc. Should match. https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:274: if (sRequestList == null) { Should this be synchronized? https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/keystore.h File net/android/keystore.h (right): https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/keystor... net/android/keystore.h:32: // creates an instance, and call its Start() routine in the main Typo: "calls" https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/keystor... net/android/keystore.h:39: // a "private key alias", which is a simple string used to uniquely How important is that uniqueness? If it is super important, how is uniqueness guaranteed/enforced? https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/keystor... net/android/keystore.h:115: std::vector<uint8>* signature); +1
I've uploaded a second patch that fixes the minor issues. I'm now preparing a new version that moves all the calls that must be performed on the UI thread to the content or browser layer. https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:53: if (algorithm == "RSA") I believe this is supported (looking at the Android sources, there are definitely Android apps and framework components comparing strings this way in a few places). However, I think it makes sense to switch to String.equals() instead, so I'll do it. Thanks https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:78: // Single signature instances. Used to perform signing with private I don't think this is necessary (I don't even use a 100-column in my editor). Just because you can doesn't mean you always should. https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:86: // to a given private key is initialized before a call to signWithPrivateKey On 2013/01/19 01:43:12, Chris P. wrote: > Nit: End sentence with a period. I'm sorry to be so nit-picky; I'm just some > random guy torturing you with a drive-by review. :( No, that's ok and welcome. I'll fix it, thanks :) https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:106: // is closed while an asynchrouns request is still pending, the native On 2013/01/19 01:43:12, Chris P. wrote: > Typo: asynchronous Done. https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:121: // To solve this, start an AsyncTask when we an alias is received. On 2013/01/19 01:43:12, Chris P. wrote: > "when we" --> "when" Done. https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:218: byte[][] encodedPrincipals, On 2013/01/19 01:43:12, Chris P. wrote: > Mixing naming conventions: key_types, encodedPrincipals. Note also that it's > called "principals" in the Javadoc. Should match. Done. https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/java/sr... net/android/java/src/org/chromium/net/AndroidKeyStore.java:274: if (sRequestList == null) { On 2013/01/19 01:43:12, Chris P. wrote: > Should this be synchronized? This shall only be called from the UI thread, so I don't think so. Adding an assert or check would make this clearer though. I'll try to find something. https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/keystore.h File net/android/keystore.h (right): https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/keystor... net/android/keystore.h:32: // creates an instance, and call its Start() routine in the main On 2013/01/19 01:43:12, Chris P. wrote: > Typo: "calls" Done. https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/keystor... net/android/keystore.h:39: // a "private key alias", which is a simple string used to uniquely On 2013/01/19 01:43:12, Chris P. wrote: > How important is that uniqueness? If it is super important, how is uniqueness > guaranteed/enforced? The user / system does that. When you install a private key / public key / certificate, a UI dialog is prompted asking you for the corresponding name for it. The same name is displayed when the user is asked to select a certificate (e.g. when calling android.KeyStore.choosePrivateKeyAlias()). The only important thing for us is that a certificate chain and its private key are stored under the same name (even if in practice they could be installed separately, e.g. through a <keygen> HTML5 operation). If you find this comment confusing, I can remove the "uniquely" from the comment, but it just describes how the system works. https://chromiumcodereview.appspot.com/11571059/diff/3001/net/android/keystor... net/android/keystore.h:115: std::vector<uint8>* signature); On 2013/01/18 20:05:48, Ryan Sleevi wrote: > DESIGN: This is not at all clear what type of signature will be generated. Is it > ECDSA? PKCS#1 v1.5 RSA-SSA? RSA-OAEP? Custom padding here (eg: as used by SSL)? Good question. I'm not sure how to best answer this. The goal is to implement the signature required by OpenSSL's client certificate support code (which will end up calling this through a custom method, like RSA_METHOD for RSA keys). From what I've seen in the OpenSSL code, (ssl3_send_client_verify under third_party/openssl/openssl/ssl/s3_clnt.c), for SSL version < DTLS 1.2, it ends up calling: - RSA_sign() on a tuple of MD5 + SHA1 hashes. - DSA_sign() on the SHA1 hash only. - ECDA_sign() on the SHA1 hash only. All these functions seem to have vanilla implementations (i.e. they don't take parameters for padding or other optional things, only the content of the digest and the key itself), and in the end that's what this method is supposed to emulate, by using platform APIs. If there is a fundamental reason why this can't work, please let me know. Off-topic note: For DTLS 1.2 support, things may be a bit more tricky to implement, because the equivalent of RSA_size(), DSA_size(), ECSA_size() is also needed, because the OpenSSL code path is different, but I'd prefer to leave this for a future patch. If you have any suggestion on how to improve this code / comment, please let me know.
Like I said, I'm not really comfortable with this approach. This definitely seems like it should be implementing ppi@'s ClientCertStore, in a layer above content/, to handle the UI interaction. The private key signing stuff also doesn't seem to fit. https://codereview.chromium.org/11571059/diff/9001/net/android/java/src/org/c... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://codereview.chromium.org/11571059/diff/9001/net/android/java/src/org/c... net/android/java/src/org/chromium/net/AndroidKeyStore.java:49: public static byte[] signWithPrivateKey(PrivateKey privateKey, Can you explain why going this route rather than an OpenSSL ENGINE? Can you describe how you see this integrating with SSLClientSocketOpenSSL?
At last, patch set 4 contains the first working version of signing support
through the Android platform key store / APIs.
I have updated the commit message to better explain what this does. Note that
there are a few caveats though:
- This only implements support for RSA keys. DSA and ECDSA are coming.
- For RSA, this only works on Android 4.2, due to a platform bug that
was only recently fixed. For details, see the comments at the end
of AndroidKeyStore.java.
In a nutshell, Signature.getInstance("NONEwithRSA") will always
throw an exception on ICS (4.0.x) and JellyBean (4.1.x). I've
tested on several devices (and reflashed a couple) to check.
This is really sad, since other algorithms (e.g. "MD5withRSA")
work pretty nicely.
"NONEwithRSA" is required by OpenSSL's current implementation, more
specifically by the code under
third_party/openssl/openssl/ssl/s3_clnt.c:ssl_send_client_verify:
Except for TLS 1.2 (which isn't supported yet in Chrome), it does
the MD5 and/or SHA1 hashing manually, they relies on the EVP_PKEY
to perform "pure" RSA / DSA / ECDSA signing on the result.
I think it might be possible to route around this bug by modifying
the OpenSSL code to allow something different, for example by letting
the user provide a callback that can be used to sign the relevant bytes
directly. Still, this is not trivial work, so I'd appreciate
your input on this. I also need to research which signature
algorithms are _really_ supported by all versions of Android since
4.0 to see if there are other similar traps.
From what I see in the AOSP bugfix, using reflection to access the
crypto framework internals might not be possible at all, due to lack
of the appropriate support code, but I'll investigate this as well.
In the meantime, I would appreciate if you could take a new look at the patch,
which is now completely different from previous iterations
(all the code to query client certificates from the system has been
removed), and let me know of glaring issues with it.
Thanks for your time.
https://chromiumcodereview.appspot.com/11571059/diff/10007/net/android/java/s... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/11571059/diff/10007/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:110: // throw NoSuchAlgorithmException on Android 4.2.1. Fixed in 4.2 This should read Android 4.1.2 :/
Patch set 5 adds test DSA and ECDSA key pairs, and corresponding signing tests. Except that the DSA and ECDSA "wrapper" tests are disabled for now, since I haven't implemented the corresponding support in net/android/keystore_openssl.cc yet. Also detect the target device's API level to prevent the RSA test to fail on Android 4.0 and 4.1. Note that the DSA and ECDSA tests do pass on these devices though.
https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:31: * PrivateKey object. Comment is wrong. It's not just RSA keys you're handling https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:41: synchronized (TAG) { Performing the signing entirely in a synchronized section? o_O Is it necessary, or is it just because you're relying on s/RsaSignature/sDsaSignature/sEcdsaSignature? Can you not exit the synchronized section once you've acquired the reference? https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:117: // and that SHA1withECDSA, its synonym, should be used instead. No, you misread. It's talking about the algorithm name "ECDSA", not "NONEwithECDSA". You can just remove this comment. https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto... File net/android/keystore.h (right): https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto... net/android/keystore.h:44: SSLClientCertType GetPrivateKeySigningType(jobject private_key); This is the wrong enum to use. The determination of what a key can be used for with respect to signature algorithms is, in part, determined by the client certificate and the keys authorized usages. You should just be talking about algorithms here (RSA, DSA, ECDSA). https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto... File net/android/keystore_unittest.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto... net/android/keystore_unittest.cc:119: const unsigned char test_rsa_key_pkcs8[] = { Why can you not just keep these as separate files, out of line from the source, like we do with certificates? Makes it much easier to maintain and debug. https://chromiumcodereview.appspot.com/11571059/diff/16001/net/net.gyp File net/net.gyp (right): https://chromiumcodereview.appspot.com/11571059/diff/16001/net/net.gyp#newcode62 net/net.gyp:62: 'android/keystore_openssl.h', Why is a custom keystore needed? It's Android, it's inherently OpenSSL. https://chromiumcodereview.appspot.com/11571059/diff/16001/net/net.gyp#newcod... net/net.gyp:1618: '<(SHARED_INTERMEDIATE_DIR)/net', Why do you need to include this directly? It should be handled by a 'direct_dependent_settings' from ssl_client_cert_types_java, which should be export_dependent_settings by net_java Or am I missing something? What header is being auto-generated?
https://chromiumcodereview.appspot.com/11571059/diff/16001/crypto/openssl_util.h File crypto/openssl_util.h (right): https://chromiumcodereview.appspot.com/11571059/diff/16001/crypto/openssl_uti... crypto/openssl_util.h:14: // A helper class that takes care of destroying OpenSSL objects when it goes out NIT: "when they go" https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. NIT: 2013. :) https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:32: * Assumes that initSignatureForKey() was called previously on the Eliminate the need for this assumption by having this function call initSignatureForKey (if necessary; i.e. if sFooSignature is null), and then you can make initSignatureForKey private and not bother callers with needing to know about it. https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:41: synchronized (TAG) { Seems odd to synchronize on an unrelated object; why not |this|? Or, why not declare the method synchronized? (Since the whole body already is.) And, yes, I agree with sleevi; it'd be nice not to need to synchronize this method. Maybe only initSignatureForKey needs to be synchronized, and once it has been called for our signature type, we can assume our signature object will never become null. Right? Since this is a lazy allocation of singletons thing, and not a constantly-updating-these-fields thing. https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:104: synchronized (TAG) { Same comment as above. https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:110: // throw NoSuchAlgorithmException on Android 4.0.x and 4.1.x, Fixed in 4.2 NIT: End with period, not comma. https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:111: // and higher. See https://android-review.googlesource.com/#/c/40352/ Anonymous internet users can't read this; that would be OK except this file is open source. Refer instead to a public bug. https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:119: } Should there be an else clause that warns the caller they asked for something that doesn't exist?
https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:32: * Assumes that initSignatureForKey() was called previously on the I should have documented this better when refactoring this code. It was more obvious in previous versions of the patch which included the client certificate request through an AsyncTask. The main issue is that Signature.getInstance() is a potentially blocking operation (at least on vanilla Java, it can touch on-disk databases the first time it is called, due to the way the crypto API works), so the plan is for initSignatureForKey() to be called in a background thread (the same one that called KeyStore.getPrivateKey(), which can also block). Note that the function is only used to perform lazy initialization of a static global variable. signWithPrivateKey() itself will always be called from the I/O thread, technically it needs to be synchronized with initSignatureForKey() to avoid memory-ordering issues on multi-core devices (the object will always be created before signWithPrivateKey() is called, but without synchronization of explicit memory barriers, the I/O thread might see the sRsaSignature reference before it observes the full initialization of the object). There are no real concerns about synchronizing several concurrent signWithPrivateKey() however. I'll have a look on the Android implementation to see if they have the same problem. If it can be guaranteed that the function never blocks, it's probably ok to call it directly instead. In the meantime, I'll update the comments. https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:41: synchronized (TAG) { On 2013/01/26 02:14:20, Chris P. wrote: > Seems odd to synchronize on an unrelated object; why not |this|? Or, why not > declare the method synchronized? (Since the whole body already is.) > This is a static method, using |this| cannot work. Moreover, both signWithPrivateKey() and initSignatureForPrivateKey() use the same mutable static members, and thus need to use the same lock. That's why a sycnrhonized function cannot work here. > And, yes, I agree with sleevi; it'd be nice not to need to synchronize this > method. Maybe only initSignatureForKey needs to be synchronized, and once it has > been called for our signature type, we can assume our signature object will > never become null. Right? Nope, because it's initialized in a different thread, we need some way to enforce memory ordering. The simplest is to use synchronization. An alternative is to use volatile variables and be very cautious, but that's always Very frankly, the synchronization here doesn't impact performance in any meaningful way (compared to signing), since all signing will happen in the same thread anyway (i.e. you won't run this in parallel). Since this is a lazy allocation of singletons thing, > and not a constantly-updating-these-fields thing. Technically, the state of the static members is modified in each signWithPrivateKey() invokation. I.e. this is not lazy-initialization of immutable objects. https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:41: synchronized (TAG) { On 2013/01/26 01:51:57, Ryan Sleevi wrote: > Performing the signing entirely in a synchronized section? o_O Is it necessary, > or is it just because you're relying on > s/RsaSignature/sDsaSignature/sEcdsaSignature? > See comment above, it's because initSignatureForPrivateKey() will be called from a different thread. > Can you not exit the synchronized section once you've acquired the reference? The method modifies the state of the static Signature objects. Technically, it is possible to do that out of the synchronized block because we know that this function will only be used from the I/O thread (i.e. it will never run in parallel). However, this wouldn't change anything about its performance, and this introduces a potential bug later if this assumption changes in the future, so I'm not really sure it's such a great idea. The radical "fix" would be to remove initSignatureForPrivateKey() entirely, i.e. consider that blocking the first time Signature.getInstance() is called is simply ok, or verifying with the platform that this will never happen otherwise. I'd be interested in your opinion on this. https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:111: // and higher. See https://android-review.googlesource.com/#/c/40352/ On 2013/01/26 02:14:20, Chris P. wrote: > Anonymous internet users can't read this; that would be OK except this file is > open source. Refer instead to a public bug. Really? This is a link to the public AOSP gerrit instance. It works pretty well in an incognito window on Chrome and Firefox, so seems definitely visible. There is no public bug for this issue (though the linked page gives the number of the internal buganizer entry for it). https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:117: // and that SHA1withECDSA, its synonym, should be used instead. On 2013/01/26 01:51:57, Ryan Sleevi wrote: > No, you misread. It's talking about the algorithm name "ECDSA", not > "NONEwithECDSA". You can just remove this comment. Oh right, thanks, I'll remove this comment. https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:119: } On 2013/01/26 02:14:20, Chris P. wrote: > Should there be an else clause that warns the caller they asked for something > that doesn't exist? This is later handled in signWithPrivateKey(). https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto... File net/android/keystore.h (right): https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto... net/android/keystore.h:44: SSLClientCertType GetPrivateKeySigningType(jobject private_key); On 2013/01/26 01:51:57, Ryan Sleevi wrote: > This is the wrong enum to use. > > The determination of what a key can be used for with respect to signature > algorithms is, in part, determined by the client certificate and the keys > authorized usages. > > You should just be talking about algorithms here (RSA, DSA, ECDSA). Ok, I'll not touch SSLClientCertType then, and try to define another enum for this. https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto... File net/android/keystore_unittest.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto... net/android/keystore_unittest.cc:119: const unsigned char test_rsa_key_pkcs8[] = { On 2013/01/26 01:51:57, Ryan Sleevi wrote: > Why can you not just keep these as separate files, out of line from the source, > like we do with certificates? Makes it much easier to maintain and debug. Yes, I'll move the data to files. https://chromiumcodereview.appspot.com/11571059/diff/16001/net/net.gyp File net/net.gyp (right): https://chromiumcodereview.appspot.com/11571059/diff/16001/net/net.gyp#newcode62 net/net.gyp:62: 'android/keystore_openssl.h', On 2013/01/26 01:51:57, Ryan Sleevi wrote: > Why is a custom keystore needed? It's Android, it's inherently OpenSSL. It's not a custom keystore, it's separation of concerns, i.e.: android/keystore.h only provides the native functions to use the platform APIs. android/keystore_openssl.h provides the function that provides an OpenSSL-specific wrapper around them. The main idea is that all OpenSSL-specific code is in android/keystore_openssl.[ch], while android/keystore.[hc] only contains JNI-specific code. I prefer to separate them because each one has its own subtle issues to deal with. I could rename the second set of files to keystore_openssl_wrapper.h if you prefer, though the recent Android 4.0 / 4.1 platform bug might require that we let go of this nice distinction :-/ https://chromiumcodereview.appspot.com/11571059/diff/16001/net/net.gyp#newcod... net/net.gyp:1618: '<(SHARED_INTERMEDIATE_DIR)/net', On 2013/01/26 01:51:57, Ryan Sleevi wrote: > Why do you need to include this directly? It should be handled by a > 'direct_dependent_settings' from ssl_client_cert_types_java, which should be > export_dependent_settings by net_java > > Or am I missing something? What header is being auto-generated? This is needed to include "net/jni_AndroidKeyStoreTestUtil.h" which is auto-generated from AndroidKeyStoreTestUtil.java. Otherwise, net/android/keystore_unittest.cc will not compile properly (it won't find the header). The non-test code depends on net_resources which has a similar gyp declaration, which is why it compiles (I suspect this is more unexpected than intentional though). I'm not too sure how to refactor this in a better way though. I'll try moving this to net_test_jni_headers as a direct_dependent_settings.
Minor remarks below - great to see the EVP_PKEY wrapper working! https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto... File net/android/keystore_unittest.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto... net/android/keystore_unittest.cc:610: // First, get the signature platform signature. nit: double "signature". https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto... net/android/keystore_unittest.cc:637: LOG(INFO) << "This test can't run on Android < 4.2"; nit: should this be indented with two spaces? https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto... net/android/keystore_unittest.cc:664: LOG(INFO) << "This test can't run on Android < 4.2"; nit: should this be indented with two spaces?
Patch set 6 finally looks like something acceptable, compared to the previous
version:
- The Signature singletons are gone. Inspection of the platform sources
shows that Signature.getInstance() can only block when another thread
is loading/installing a new cryptographic provider at the same time.
It's pretty safe to assume that this will never happen in the context of
Chromium. This could happen in WebView-using applications, but this
kind of condition would be extremely rare in practice.
- Added a new evil hack to work around platform bug that was only
fixed in Android 4.2. Only used on 4.0 and 4.1 when necessary and
contains plenty of sanity checks to fail without crashing if running in
a heavily "OEM-optimized" system image.
- Leave SSLClientCertType alone, instead define net::android::PrivateKeyType
enum to enumerate, well, private key types.
- Move all test data bytes to files under net/data/ssl/certificates/ +
add a generation script.
- Provide custom DSA_METHOD and ECDSA_METHOD implementations
to make the wrapper EVP_PKEY work with DSA and ECDSA keys.
- Complete unit test coverage. All 'AndroidKeyStore' net_unittests passed on
a 4.1.2 device. I'll test them on 4.0 and 4.2 very soon.
I'd be glad if Ryan and Adam could take a deep look at the code that is now in
net/android/keystore_openssl.cc, because it somehow relies on OpenSSL internals
(I really couldn't find a way to avoid that).
As usual, suggestions for improvements are welcomed.
https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto...
File net/android/keystore_unittest.cc (right):
https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto...
net/android/keystore_unittest.cc:637: LOG(INFO) << "This test can't run on
Android < 4.2";
On 2013/01/29 17:57:10, ppi wrote:
> nit: should this be indented with two spaces?
Done.
https://chromiumcodereview.appspot.com/11571059/diff/16001/net/android/keysto...
net/android/keystore_unittest.cc:664: LOG(INFO) << "This test can't run on
Android < 4.2";
On 2013/01/29 17:57:11, ppi wrote:
> nit: should this be indented with two spaces?
Yes, I probably forgot to change my editor's setting while switching between
Java and C++ sources :-) Done.
LGTM I'm sorry that you had to see that side of OpenSSL :) Currently I see that you're only supporting NID_md5_sha1, which is sufficient for TLS <= 1.1 and saves having to worry about PKCS#1 ASN.1 because there isn't any. However, we will be implementing TLS 1.2 support in Chrome this quarter and that will bring SHA-256 signatures into play. I think you will be fine with just a couple of tweaks if you assuming that the PKCS#1 ASN.1 is done before the EVP_PKEY functions are called. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/java/s... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:38: * @param message The message to sign. I'm assuming here that |message| has already been hashed? In the case of RSA PKCS#1, the message also needs to be wrapped in ASN.1 that describes the hash function used so I guess that's included for the RSA case too? Probably worth mentioning that in the comment here. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:185: // result of the getOpenSSLKey(). This is an integer which "which value"? Maybe "This is an integer who's value is the address"... https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... File net/android/keystore.h (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore.h:37: // callback, so must end up implementing the same thing than s/than/as/ https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore.h:42: // |message| is the input message. ditto with the comment around |message| - prehashed? pre-PKCS#1 encoded? https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... File net/android/keystore_openssl.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:32: // I note that you have a number of "//" lines in here. I would personally remove them but, if you like it, I've no problem with that either. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:35: // Internally, it can hold a pointer to a RSA, DSA or ECDA structure, ECDA -> ECDSA https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:162: // will always pass the NID_md5_sha1 |type| value. This will stop being true when we add TLS 1.2 support, which is an OKR for this quarter. TLS 1.2 can negotiate the hash function to use and typically uses SHA-256. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... File net/android/keystore_openssl.h (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.h:17: // delete line with just "//"?
On 2013/01/30 14:28:53, agl wrote: > LGTM > > I'm sorry that you had to see that side of OpenSSL :) > > Currently I see that you're only supporting NID_md5_sha1, which is sufficient > for TLS <= 1.1 and saves having to worry about PKCS#1 ASN.1 because there isn't > any. > > However, we will be implementing TLS 1.2 support in Chrome this quarter and that > will bring SHA-256 signatures into play. I think you will be fine with just a > couple of tweaks if you assuming that the PKCS#1 ASN.1 is done before the > EVP_PKEY functions are called. > Yes, the limitation is intentional. I had a deep look at the OpenSSL code that deals with TLS 1.2 support (in ssl3_send_verify_cert() under openssl/ssl/s3_clnt.c), and it _seems_ to me that in certain cases, the code may end up calling RSA_size(), which does not support custom RSA_METHODs. Same problem with DSA_size() / ECDSA_size(). I don't see a way to support this use case without modifying the library in rather invasive ways. But maybe there is a guarantee that this will never happen, it's really hard to be sure when tracing all possible code paths though. Didn't realize this is a Q1 OKR, this is going to make things ... interesting. I'll file a bug to discuss these issues in more detail with you though.
By the way, testing shows that the reflection hack fails on some Android 4.0 devices. After lurking the platform's history, it looks like the internal OpenSSLRSAPrivateKey class we're looking for was introduced in 4.0.4, and I have a device running 4.0.3 where the unit tests fail. I think I have a plan for these older 4.0 versions, and will update the patch as needed. Note sure why the android_db_triggered_tests fail though, this seems related to something different, and I can't reproduce it locally for now. The linux_redux build is broken to no avail (in completely unrelated ways). Some of the issues are in WebKit, so I don't know how long it will take to get them fixed. I have a trivial fix for the android_dbg_clang build breakage though. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/java/s... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:38: * @param message The message to sign. On 2013/01/30 14:28:53, agl wrote: > I'm assuming here that |message| has already been hashed? In the case of RSA > PKCS#1, the message also needs to be wrapped in ASN.1 that describes the hash > function used so I guess that's included for the RSA case too? > > Probably worth mentioning that in the comment here. Yes, what the function receives is the actual hash computed by OpenSSL itself, and it should behave exactly like RSA_sign(NID_md5_sha1,...), since that's what the library is going to end up calling. I'll update the comment to be more precise, e.g. copying from the OpenSSL doc: """ If type is NID_md5_sha1, an SSL signature (MD5 and SHA1 message digests with PKCS #1 padding and no algorithm identifier) is created. """ https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:185: // result of the getOpenSSLKey(). This is an integer which On 2013/01/30 14:28:53, agl wrote: > "which value"? Maybe "This is an integer who's value is the address"... thanks, I'll fix this to "whose value".
https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/java/s... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:46: public static byte[] signWithPrivateKey(PrivateKey privateKey, perhaps name this "rawSignWithPrivateKey", since it's relying on |message| being a fully formatted/padded message that is <= modulus length? https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:206: return evp_pkey; I am nervous about the potential for ABI issues here. How do you plan to guard against breakages? https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... File net/android/keystore.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore.cc:46: JavaByteArrayToByteVector(env, signature_ref.obj(), signature); Is this guaranteed to be no-fail? https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore.cc:61: return reinterpret_cast<EVP_PKEY*>(pkey); Please make sure palmer is happy with this. I'm surprised it's not intptr_t, at least... https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... File net/android/keystore_openssl.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:27: #include <openssl/../../crypto/ecdsa/ecs_locl.h> This certainly seems the wrong way to include this header. Can we not modify the include path we use for openssl.gyp to allow the 'correct' path to be determined (eg: openssl/src/crypto/ecdsa/ecs_local.h, for example). I want to try and find a solution that doesn't "#include <foo/../../bar>" https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:185: memcpy(signature, &result[0], result.size()); BUG: No check that result.size() == RSA_size(rsa), which is the expectation of how much memory will be provided in this method call. RSA_size uses r->n (eg: ignoring the method), which is going to fail under your synthetic key because you've failed to define the public parameters for it, so there's no reliable way for the underlying OpenSSL calls to be "well behaved" I'm not sure that your RSA_method approach is fully correct because of that. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:236: &signature)) { I don't believe the indents are correct here for the conditionals https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... File net/android/keystore_unittest.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_unittest.cc:222: 1, ECDSA_verify(0, digest, digest_len, sigbuf, siglen, pub_key)); BUG: If this ASSERT fails, you end up leaking pub_key Same throughout this file where you end up using ASSERTs https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_unittest.cc:239: // Note that the code in ssl/s3_clnt.c does something similar. This does not seem like something we should rely on. I expect it works for the engines because RSA_size just works based on knowing the public modulus, and that's not seen as secret data in any (external) OpenSSL engine or PKCS#11 card that I'm aware of. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_unittest.cc:247: ASSERT_TRUE(rsa != NULL); nit: ASSERT_TRUE(rsa) (and same throughout this file) https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_unittest.cc:380: ASSERT_TRUE(openssl_key.get() != NULL); Drop the != NULL (same as before and same throughout) https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_unittest.cc:384: // See third_party/openssl/openssl/crypto/rsa/rsa_sign.c You don't need to cross-reference this. The 36 bytes = size of MD5 + size of SHA1, same as the NID claims :) https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/privat... File net/android/private_key_type_list.h (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/privat... net/android/private_key_type_list.h:12: DEFINE_PRIVATE_KEY_TYPE(INVALID,255) Fix your whitespace. You use two spaces for RSA/DSA and no spaces here. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/data/ssl/certi... File net/data/ssl/certificates/README (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/data/ssl/certi... net/data/ssl/certificates/README:147: properly. See the generate-android-test-keys.sh script. Binary files like this make it hard to store and hard to run tryjobs. Can you "ascii armor" / "pem encode" them?
https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/java/s... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:46: public static byte[] signWithPrivateKey(PrivateKey privateKey, On 2013/01/31 03:09:53, Ryan Sleevi wrote: > perhaps name this "rawSignWithPrivateKey", since it's relying on |message| being > a fully formatted/padded message that is <= modulus length? Done. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:206: return evp_pkey; On 2013/01/31 03:09:53, Ryan Sleevi wrote: > I am nervous about the potential for ABI issues here. How do you plan to guard > against breakages? I'm concerned about it too. I think longer term, it would be nice if OpenSSL could have the option to use a callback to perform the hashing + signing directly. This assumes that Android 4.0.4 -> 4.1.x provide all the algorithms we need though, and I didn't take the time to look at the details. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... File net/android/keystore.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore.cc:46: JavaByteArrayToByteVector(env, signature_ref.obj(), signature); On 2013/01/31 03:09:53, Ryan Sleevi wrote: > Is this guaranteed to be no-fail? Yes. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore.cc:61: return reinterpret_cast<EVP_PKEY*>(pkey); On 2013/01/31 03:09:53, Ryan Sleevi wrote: > Please make sure palmer is happy with this. I'm surprised it's not intptr_t, at > least... I've added a technical note. The address is stored as a Java integer in a system-private object, and it retrieved as is. Since Android < 4.2 will never run on a 64-bit system, that should not be a concern. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... File net/android/keystore.h (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore.h:37: // callback, so must end up implementing the same thing than On 2013/01/30 14:28:53, agl wrote: > s/than/as/ Done. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore.h:42: // |message| is the input message. On 2013/01/30 14:28:53, agl wrote: > ditto with the comment around |message| - prehashed? pre-PKCS#1 encoded? I've clarified the comment. Thanks. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... File net/android/keystore_openssl.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:27: #include <openssl/../../crypto/ecdsa/ecs_locl.h> On 2013/01/31 03:09:53, Ryan Sleevi wrote: > This certainly seems the wrong way to include this header. > > Can we not modify the include path we use for openssl.gyp to allow the 'correct' > path to be determined (eg: openssl/src/crypto/ecdsa/ecs_local.h, for example). > > I want to try and find a solution that doesn't "#include <foo/../../bar>" I've added ../third_party/openssl/ to the 'include_dirs' directive in net.gyp and this seems to work with #include <openssl/crypto/ecdsa/ecs_locl.h>, i.e. without touching the other includes. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:32: // On 2013/01/30 14:28:53, agl wrote: > I note that you have a number of "//" lines in here. I would personally remove > them but, if you like it, I've no problem with that either. Done. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:35: // Internally, it can hold a pointer to a RSA, DSA or ECDA structure, On 2013/01/30 14:28:53, agl wrote: > ECDA -> ECDSA Done. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:162: // will always pass the NID_md5_sha1 |type| value. On 2013/01/30 14:28:53, agl wrote: > This will stop being true when we add TLS 1.2 support, which is an OKR for this > quarter. TLS 1.2 can negotiate the hash function to use and typically uses > SHA-256. I know, that's why I've put the check here. I don't know enough OpenSSL to know what to do exactly here if the 'type' is different. My understanding is that for TLS 1.2, RSA_sign() will never be called directly. Instead, the RsaMethodPrivEnv() method will have to be implemented because it can deal with extra padding requirements and stuff. If you have any kind of pointers to more details, I'd be happy to look at them. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:185: memcpy(signature, &result[0], result.size()); On 2013/01/31 03:09:53, Ryan Sleevi wrote: > BUG: > > No check that result.size() == RSA_size(rsa), which is the expectation of how > much memory will be provided in this method call. > > RSA_size uses r->n (eg: ignoring the method), which is going to fail under your > synthetic key because you've failed to define the public parameters for it, so > there's no reliable way for the underlying OpenSSL calls to be "well behaved" > > I'm not sure that your RSA_method approach is fully correct because of that. I have added code that setups r->n in the custom RSA key, This makes RSA_size() return the right value. Added a check here for it + a unittest. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:236: &signature)) { On 2013/01/31 03:09:53, Ryan Sleevi wrote: > I don't believe the indents are correct here for the conditionals I'm not sure, what would you propose instead? https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:239: Note: I've added code to implement DSA_size() properly, and I've added a check here for signature.size() == DSA_size(dsa) - 2 here. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:386: I'd like to add a check for signature.size() == ECDA_size() - 2 here, but I'm not sure how to setup the EC_KEY so that ECDSA_size() would always return the right value for it. It looks like what is needed is setup the key's group with a value that has the same order than the PrivateKey. However, I'm not too familiar with elliptical cryptography. I can see a way to retrieve the PrivateKey's parameters with platform APIs, but I miss a way to create an EC_KEY / EC_GROUP with this information. Any idea? https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... File net/android/keystore_openssl.h (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.h:17: // On 2013/01/30 14:28:53, agl wrote: > delete line with just "//"? Done. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... File net/android/keystore_unittest.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_unittest.cc:222: 1, ECDSA_verify(0, digest, digest_len, sigbuf, siglen, pub_key)); On 2013/01/31 03:09:53, Ryan Sleevi wrote: > BUG: If this ASSERT fails, you end up leaking pub_key > > Same throughout this file where you end up using ASSERTs Ah, I didn't realize we didn't want memory leaks, even when the unit tests are failing. I've cleaned up the whole source file then. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_unittest.cc:239: // Note that the code in ssl/s3_clnt.c does something similar. I've update the code appropriately. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_unittest.cc:380: ASSERT_TRUE(openssl_key.get() != NULL); On 2013/01/31 03:09:53, Ryan Sleevi wrote: > Drop the != NULL (same as before and same throughout) I was just using the recommendations from the GTest FAQ [1]. I've updated all the code though. [1] https://code.google.com/p/googletest/wiki/FAQ#Why_does_Google_Test_support_EX... https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/privat... File net/android/private_key_type_list.h (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/privat... net/android/private_key_type_list.h:12: DEFINE_PRIVATE_KEY_TYPE(INVALID,255) On 2013/01/31 03:09:53, Ryan Sleevi wrote: > Fix your whitespace. You use two spaces for RSA/DSA and no spaces here. Done. https://chromiumcodereview.appspot.com/11571059/diff/20004/net/data/ssl/certi... File net/data/ssl/certificates/README (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/data/ssl/certi... net/data/ssl/certificates/README:147: properly. See the generate-android-test-keys.sh script. All files are now PEM-encoded.
Note that the latest patch also adds a new hack for Android 4.0 -> 4.0.3. On these platforms, it's possible to extract the PrivateKey encoded content directly. This makes it possible to create an EVP_PKEY directly for it, and return it as the "wrapper". In this specific case, the JNI reference can be safely discarded because OpenSSL will never use it to perform the signing. I've tested the current patch on Android 4.0.3, 4.0.4, 4.1.2 and 4.2 succesfully (at least the unit tests pass on all of these). Now however that the unit tests only use private keys that are created with the platform APIs from their encoded PKCS#8 data. It's not possible to test this with actual installed client certificates there, because the platform doesn't provide any way to install or retrieve these without prompting the user with a UI dialog. Now, all that seems to be missing is support for ECDSA_size().
https://chromiumcodereview.appspot.com/11571059/diff/35002/net/android/keysto... File net/android/keystore.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/35002/net/android/keysto... net/android/keystore.cc:106: // ported to 64-bit environments, if ever). OK. Does it make sense to actually declare it as a jint, too? (even though I am sure int == jint in this situation)
I haven't reviewed the test changes. Just responding to your ECC question https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... File net/android/keystore_openssl.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:386: On 2013/01/31 17:44:30, digit1 wrote: > I'd like to add a check for signature.size() == ECDA_size() - 2 here, but I'm > not sure how to setup the EC_KEY so that ECDSA_size() would always return the > right value for it. > > It looks like what is needed is setup the key's group with a value that has the > same order than the PrivateKey. However, I'm not too familiar with elliptical > cryptography. I can see a way to retrieve the PrivateKey's parameters with > platform APIs, but I miss a way to create an EC_KEY / EC_GROUP with this > information. Any idea? So the ECParameterSpec ( http://docs.oracle.com/javase/1.5.0/docs/api/java/security/spec/ECParameterSp... ) for an ECKey ( http://docs.oracle.com/javase/1.5.0/docs/api/java/security/interfaces/ECKey.h... ), which an ECPrivateKey subclasses ( http://docs.oracle.com/javase/1.5.0/docs/api/java/security/interfaces/ECPriva... ) should contain the parameters from the PrivateKey ECDSA_size uses EC_GROUP_get_order to find out the number of bits in the order to work through the size of r & s (combined, they make up the signature) EC_GROUP_get_order requires that EC_KEY_get0_group returns an EC_GROUP You can set the EC_GROUP for an EC_KEY by using EC_KEY_set_group, which requires you supply an EC_GROUP (and that it be dup-able) The EC_GROUP_new requires an EC_METHOD to handle the group parameters. EG_GROUP_new uses meth->group_init to initialize the parameters, which it expects to fill in the order, the cofactor, and the curve name/flags EC_GROUP_free expects to be able to use meth->group_finish The order is available via ECParameterSpec::getOrder I'm *pretty* sure that that is the only thing you *actually* need to stub in to be able to fake it. Adam should confirm. https://chromiumcodereview.appspot.com/11571059/diff/35002/net/android/java/s... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/11571059/diff/35002/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:58: * Returns the 'Q' parameter og a given DSA private key as a byte typo: og/of https://chromiumcodereview.appspot.com/11571059/diff/35002/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:108: * with PKCS#1 padding). typo: extra ) https://chromiumcodereview.appspot.com/11571059/diff/35002/net/android/keysto... File net/android/keystore_openssl.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/35002/net/android/keysto... net/android/keystore_openssl.cc:242: ); style: whitespace for );
Latest version fixes ECDSA_size() support. I also fixed the signature size checks for DSA and ECDSA. I discovered that they would fail randomly because the actual signature size is not constant (probably due to the random bits used to compute them). I'm assuming that the signature size if always <= DSA_size() / ECDSA_size(). If this is not true please let me know, I didn't really find some definitive information about this. Tested on 4.0.3 / 4.0.4 / 4.1.2 / 4.2.1, https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... File net/android/keystore_openssl.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/20004/net/android/keysto... net/android/keystore_openssl.cc:386: That's essentially what I ended up implementing. I'm creating a group with a default EC_METHOD, then modifying the order member in-place. Seems to work right. https://chromiumcodereview.appspot.com/11571059/diff/35002/net/android/java/s... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/11571059/diff/35002/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:58: * Returns the 'Q' parameter og a given DSA private key as a byte On 2013/02/01 00:51:30, Ryan Sleevi wrote: > typo: og/of Done. https://chromiumcodereview.appspot.com/11571059/diff/35002/net/android/java/s... net/android/java/src/org/chromium/net/AndroidKeyStore.java:108: * with PKCS#1 padding). On 2013/02/01 00:51:30, Ryan Sleevi wrote: > typo: extra ) Done. https://chromiumcodereview.appspot.com/11571059/diff/35002/net/android/keysto... File net/android/keystore_openssl.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/35002/net/android/keysto... net/android/keystore_openssl.cc:242: ); On 2013/02/01 00:51:30, Ryan Sleevi wrote: > style: whitespace for ); Done.
Comments on the diff from patchset 8, which mostly LG. I will make a full review later on to make sure I haven't missed anything. https://chromiumcodereview.appspot.com/11571059/diff/30019/net/android/keysto... File net/android/keystore_openssl.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/30019/net/android/keysto... net/android/keystore_openssl.cc:231: bool CopyBigNumFromBytes(BIGNUM& num, STYLE: Passing non-const references like this is not accepted in Chromium code http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere... https://chromiumcodereview.appspot.com/11571059/diff/30019/net/android/keysto... net/android/keystore_openssl.cc:252: const std::vector<uint8>& new_bytes) { Here, both reference & indent https://chromiumcodereview.appspot.com/11571059/diff/30019/net/android/keysto... net/android/keystore_openssl.cc:354: // Note: With DSA, the actual signature might be smaller than DSA_size(). Correct. It should be within +/- 2, IIRC (depending on leading octets of the encoded big integers) https://chromiumcodereview.appspot.com/11571059/diff/30019/net/android/keysto... net/android/keystore_openssl.cc:588: ScopedEC_GROUP group(EC_GROUP_new(EC_GFp_nist_method())); Adam should double check this
https://chromiumcodereview.appspot.com/11571059/diff/30019/net/android/keysto... File net/android/keystore_openssl.cc (right): https://chromiumcodereview.appspot.com/11571059/diff/30019/net/android/keysto... net/android/keystore_openssl.cc:231: bool CopyBigNumFromBytes(BIGNUM& num, ok, I've fixed that. Thanks for the pointer. https://chromiumcodereview.appspot.com/11571059/diff/30019/net/android/keysto... net/android/keystore_openssl.cc:252: const std::vector<uint8>& new_bytes) { On 2013/02/01 21:50:58, Ryan Sleevi wrote: > Here, both reference & indent Done.
A lot of style issues this time around, a few TODOs, and some questions below. Also, one (class of) bugs flagged BUG https://codereview.chromium.org/11571059/diff/51001/crypto/openssl_util.h File crypto/openssl_util.h (left): https://codereview.chromium.org/11571059/diff/51001/crypto/openssl_util.h#old... crypto/openssl_util.h:17: class ScopedOpenSSL { As a TODO, we should move this towards using the new scoped_ptr<> classes rather than having yet-another scoped type. It's on my radar to do the same for NSS, just FYI. https://codereview.chromium.org/11571059/diff/51001/net/android/java/src/org/... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://codereview.chromium.org/11571059/diff/51001/net/android/java/src/org/... net/android/java/src/org/chromium/net/AndroidKeyStore.java:112: * PrivateKey object for client certificate support. These comments (for remaining native code) all have the style of describing how the function *is used*, rather than how it *should be used*. Similar to discussions on passive vs active voice (eg: why "we" is discouraged in comments), these comments should provide clear guidance on the correct way to use this, rather than try to describe how it's being used. https://codereview.chromium.org/11571059/diff/51001/net/android/java/src/org/... net/android/java/src/org/chromium/net/AndroidKeyStore.java:127: * with PKCS#1 padding. comment nit: s/pure/raw/ comment nit: The message itself must a combined, 36-byte MD5+SHA1 message digest padded to the length of the modulus using PKCS#1 padding. https://codereview.chromium.org/11571059/diff/51001/net/android/java/src/org/... net/android/java/src/org/chromium/net/AndroidKeyStore.java:132: * compute a direct DSA/ECDSA signature for it. comment nit: s/will be/must be/ https://codereview.chromium.org/11571059/diff/51001/net/android/java/src/org/... net/android/java/src/org/chromium/net/AndroidKeyStore.java:153: // and higher. See https://android-review.googlesource.com/#/c/40352/ comment nit: This is no longer "what looks like", right? This is a confirmed bug, with workaround (as seen on lines 138-140). Perhaps this should be removed? https://codereview.chromium.org/11571059/diff/51001/net/android/java/src/org/... net/android/java/src/org/chromium/net/AndroidKeyStore.java:208: * before Android 4.2). comment nit: These sorts of positional references in comments make it hard to move the code, or understand "how far above" to read. perhaps a better way (of no doubt many): "This is used for situations when "NONEwithRSA" signatures are unavailable, as described in rawSignDigestWithPrivateKey" https://codereview.chromium.org/11571059/diff/51001/net/android/keystore.h File net/android/keystore.h (right): https://codereview.chromium.org/11571059/diff/51001/net/android/keystore.h#ne... net/android/keystore.h:19: typedef struct evp_pkey_st EVP_PKEY; comment nit: Be concrete in which OpenSSL headers you're avoiding including https://codereview.chromium.org/11571059/diff/51001/net/android/keystore.h#ne... net/android/keystore.h:21: // Misc classes to access the Android platform KeyStore. comment nit: These are not classes https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... File net/android/keystore_openssl.cc (right): https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:36: // comment nit: Other types of signing may be useful outside of the context of client certs, so I don't think this comment is entirely clear or reflective of the situation. For example, your RSA method will accept well-formatted messages of any form (eg: PSS, PKCS#1v1.5 with SHA-256, etc). Your DSA/ECDSA will accept any hash form as well. Also, remove extra // or remove the blank line (line 37). Same also for line 106. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:99: // comment nit: superflous pretty-formatting is discouraged, as per http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Vertic... https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:197: if (expected_size != result.size()) { Are you sure != is correct? I think this should be > as well, contingent upon how RSA_size handles leading zeroes... https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:199: result.size() << ", expected " << expected_size; See http://dev.chromium.org/developers/coding-style This should be: LOG(ERROR) << "RSA Signature size mismatch, actual: " << result.size() << ", expected " << expected_size; https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:233: const std::vector<uint8>& new_bytes) { STYLE: These functions follow the wrong order, as per http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... inputs, input/outputs, outputs. See also SwapBigNumPtrFromBytes https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:242: BN_free(new_num); Why not BN_swap? Or even more, why not just the significantly more efficient BIGNUM* new_num = BN_bin2bn(data, length, num); Which directly updates num in place? Unless you're trying to early-free memory (eg: the classic swap trick), it seems like a write-in-place is more desirable here. Otherwise, use the swap trick and avoid a copy op. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:266: return true; I'm nervous about the correctness of this, in the event that others have stored |*num_ptr|. Using BN_swap is more correct here. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:270: void GetRsaPkeyWrapper(ScopedEVP_PKEY& pkey, STYLE: non-const references are no good http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere... https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:297: ScopedJavaGlobalRef<jobject>& private_key) { STYLE: same here re: non-const refs. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:309: private_key.obj(), &encoded)) { style: indent an additional four spaces. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:353: &signature)) { style: indent an additional four spaces. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:361: signature.size() << ", expected <= " << max_expected_size; style: same comments as before regarding << wrapping https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:362: return 0; style: return NULL, not 0. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:479: // in the future. This sort of subtlety leaves me afraid of a double-free being introduced. Please make this error 'more' serious. I would rather we crash if the library pre-conditions fail, rather than introduce a security bug. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:506: return s_ecdsa_ex_data_index; So it's clear for general reference, I don't think this is the general recommended Chromium practice. The only time we use pthread_once in Chromium code is in explicit sandbox init code. I cannot help but feel this is the wrong abstraction, in how it couples things, even if this is "just Android" code. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:528: &signature)) { style: indents are wrong https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:538: signature.size() << ", expected <= " << max_expected_size; style: << is wrong https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:601: } style: Don't use the extra function-level block. It doesn't seem to provide any benefits here. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... File net/android/keystore_unittest.cc (right): https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:70: ScopedPKCS8_PRIV_KEY_INFO; style: Doesn't this fit on the same line? https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:121: ASSERT_TRUE(bio.get()) << GetOpenSSLErrorString(); Why do you copy it into a BIO, when PEM_read_PrivateKey exists? https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:143: reinterpret_cast<const char*>(&bytes[0]), Why do you not use |p| here? Further, why do you copy into |bytes|, and not simply use base::WriteInto? https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:151: GetPrivateKeyPkcs8Bytes(pkey, pkcs8); BUG: Any one of these can fail their ASSERTs, but the behaviour of the ASSERT is just to return (for a void function). Thus, for each of these functions, you may be operating on junk data. You should make the functions bool, and then use the ASSERTs here. Or further propagate the bool (the more logical option, IMO) and ASSERT within the test itself. Or you need to manually be checking that no errors have been received and if so, return, which is needlessly complex. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:166: const_cast<char*>(data.data()), static_cast<int>(data.size()))); same comments RE: BIO and ASSERT https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:203: ImportPrivateKeyFileAsPkcs8(kTestRsaKeyFile, &key); same comments here re: ASSERT failures propagating https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:238: ScopedEVP_PKEY pkey(NULL); nit: do not explicitly NULL initialize like this (in anticipation of eventually switching to scoped_ptr) https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:258: const char kTestEcdsaHash[] = "0123456789ABCDEFGHIJ"; style: Place these constants at the beginning (line 71) Also, for consistency, use FilePath::CharType explicitly, not char, for the paths. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:388: ASSERT_EQ(openssl_signature[n], signature[n]); Use SCOPED_TRACE here, as otherwise the gtest output will not indicate n. nit/todo: Seems like we should define a custom pred_formater for handling binary output as hex, given the frequency of "compare two byte buffers" comes up. https://codereview.chromium.org/11571059/diff/51001/net/data/ssl/certificates... File net/data/ssl/certificates/README (right): https://codereview.chromium.org/11571059/diff/51001/net/data/ssl/certificates... net/data/ssl/certificates/README:143: - android-test-key-ecdsa-pubic.der update these with the new file names.
https://codereview.chromium.org/11571059/diff/51001/net/android/java/src/org/... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://codereview.chromium.org/11571059/diff/51001/net/android/java/src/org/... net/android/java/src/org/chromium/net/AndroidKeyStore.java:112: * PrivateKey object for client certificate support. I've updated the comments so they could be a bit more direct. https://codereview.chromium.org/11571059/diff/51001/net/android/java/src/org/... net/android/java/src/org/chromium/net/AndroidKeyStore.java:127: * with PKCS#1 padding. On 2013/02/04 22:53:27, Ryan Sleevi wrote: > comment nit: s/pure/raw/ > comment nit: > The message itself must a combined, 36-byte MD5+SHA1 message digest padded to > the length of the modulus using PKCS#1 padding. Done. https://codereview.chromium.org/11571059/diff/51001/net/android/java/src/org/... net/android/java/src/org/chromium/net/AndroidKeyStore.java:132: * compute a direct DSA/ECDSA signature for it. On 2013/02/04 22:53:27, Ryan Sleevi wrote: > comment nit: s/will be/must be/ Done. https://codereview.chromium.org/11571059/diff/51001/net/android/java/src/org/... net/android/java/src/org/chromium/net/AndroidKeyStore.java:153: // and higher. See https://android-review.googlesource.com/#/c/40352/ On 2013/02/04 22:53:27, Ryan Sleevi wrote: > comment nit: This is no longer "what looks like", right? This is a confirmed > bug, with workaround (as seen on lines 138-140). Perhaps this should be removed? Done. https://codereview.chromium.org/11571059/diff/51001/net/android/java/src/org/... net/android/java/src/org/chromium/net/AndroidKeyStore.java:208: * before Android 4.2). On 2013/02/04 22:53:27, Ryan Sleevi wrote: > comment nit: These sorts of positional references in comments make it hard to > move the code, or understand "how far above" to read. > > perhaps a better way (of no doubt many): > "This is used for situations when "NONEwithRSA" signatures are unavailable, as > described in rawSignDigestWithPrivateKey" Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore.h File net/android/keystore.h (right): https://codereview.chromium.org/11571059/diff/51001/net/android/keystore.h#ne... net/android/keystore.h:19: typedef struct evp_pkey_st EVP_PKEY; On 2013/02/04 22:53:27, Ryan Sleevi wrote: > comment nit: Be concrete in which OpenSSL headers you're avoiding including Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore.h#ne... net/android/keystore.h:21: // Misc classes to access the Android platform KeyStore. On 2013/02/04 22:53:27, Ryan Sleevi wrote: > comment nit: These are not classes Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... File net/android/keystore_openssl.cc (right): https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:36: // On 2013/02/04 22:53:27, Ryan Sleevi wrote: > comment nit: Other types of signing may be useful outside of the context of > client certs, so I don't think this comment is entirely clear or reflective of > the situation. > > For example, your RSA method will accept well-formatted messages of any form > (eg: PSS, PKCS#1v1.5 with SHA-256, etc). Your DSA/ECDSA will accept any hash > form as well. > OpenSSL provides many ways to sign messages, not all of them will end up calling RSA_sign(). I have updated the comment to reflect that and explain that this code doesn't try to support all use cases. > Also, remove extra // or remove the blank line (line 37). > Same also for line 106. Done https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:99: // On 2013/02/04 22:53:27, Ryan Sleevi wrote: > comment nit: superflous pretty-formatting is discouraged, as per > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Vertic... Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:197: if (expected_size != result.size()) { On 2013/02/04 22:53:27, Ryan Sleevi wrote: > Are you sure != is correct? I think this should be > as well, contingent upon > how RSA_size handles leading zeroes... Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:199: result.size() << ", expected " << expected_size; On 2013/02/04 22:53:27, Ryan Sleevi wrote: > See http://dev.chromium.org/developers/coding-style > > This should be: > LOG(ERROR) << "RSA Signature size mismatch, actual: " > << result.size() << ", expected " << expected_size; Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:233: const std::vector<uint8>& new_bytes) { On 2013/02/04 22:53:27, Ryan Sleevi wrote: > STYLE: These functions follow the wrong order, as per > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > > inputs, input/outputs, outputs. > > See also SwapBigNumPtrFromBytes Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:242: BN_free(new_num); Yes, that seems preferrable. Thanks. Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:266: return true; I've changed the code to use BN_bin2bn(...,old_num) to perform the change in-place if old_num != NULL, the BN_free() / BN_swap() is no longer needed. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:270: void GetRsaPkeyWrapper(ScopedEVP_PKEY& pkey, Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:297: ScopedJavaGlobalRef<jobject>& private_key) { On 2013/02/04 22:53:27, Ryan Sleevi wrote: > STYLE: same here re: non-const refs. Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:309: private_key.obj(), &encoded)) { On 2013/02/04 22:53:27, Ryan Sleevi wrote: > style: indent an additional four spaces. Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:353: &signature)) { On 2013/02/04 22:53:27, Ryan Sleevi wrote: > style: indent an additional four spaces. Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:361: signature.size() << ", expected <= " << max_expected_size; On 2013/02/04 22:53:27, Ryan Sleevi wrote: > style: same comments as before regarding << wrapping Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:362: return 0; On 2013/02/04 22:53:27, Ryan Sleevi wrote: > style: return NULL, not 0. Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:479: // in the future. On 2013/02/04 22:53:27, Ryan Sleevi wrote: > This sort of subtlety leaves me afraid of a double-free being introduced. > > Please make this error 'more' serious. I would rather we crash if the library > pre-conditions fail, rather than introduce a security bug. Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:506: return s_ecdsa_ex_data_index; On 2013/02/04 22:53:27, Ryan Sleevi wrote: > So it's clear for general reference, I don't think this is the general > recommended Chromium practice. > > The only time we use pthread_once in Chromium code is in explicit sandbox init > code. I cannot help but feel this is the wrong abstraction, in how it couples > things, even if this is "just Android" code. I'm not sure I understand. Are you saying it's not ok to use platform-specific APIs in platform-specific code? The only alternative provided by base/ to to thread-safe lazy initialization are Singleton and LazyInstance. Singleton requires creating a heap-based object, which seems overkill here, so I've used a LazyInstance instead. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:528: &signature)) { On 2013/02/04 22:53:27, Ryan Sleevi wrote: > style: indents are wrong Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:538: signature.size() << ", expected <= " << max_expected_size; On 2013/02/04 22:53:27, Ryan Sleevi wrote: > style: << is wrong Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:601: } On 2013/02/04 22:53:27, Ryan Sleevi wrote: > style: Don't use the extra function-level block. It doesn't seem to provide any > benefits here. Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... File net/android/keystore_unittest.cc (right): https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:70: ScopedPKCS8_PRIV_KEY_INFO; No, it doesn't fit in 80 columns. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:121: ASSERT_TRUE(bio.get()) << GetOpenSSLErrorString(); On 2013/02/04 22:53:27, Ryan Sleevi wrote: > Why do you copy it into a BIO, when PEM_read_PrivateKey exists? Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:143: reinterpret_cast<const char*>(&bytes[0]), On 2013/02/04 22:53:27, Ryan Sleevi wrote: > Why do you not use |p| here? > Because i2d_PKCS8_PRIV_KEY_INFO only works with an 'unsigned char**' parameter. > Further, why do you copy into |bytes|, and not simply use base::WriteInto? I changed the code to use this. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:151: GetPrivateKeyPkcs8Bytes(pkey, pkcs8); You can't use ASSERT in a function that doesn't return void. I've changed the code to propagate the error and moved the ASSERTS to the TEST() functions. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:238: ScopedEVP_PKEY pkey(NULL); On 2013/02/04 22:53:27, Ryan Sleevi wrote: > nit: do not explicitly NULL initialize like this (in anticipation of eventually > switching to scoped_ptr) Done. https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_unit... net/android/keystore_unittest.cc:388: ASSERT_EQ(openssl_signature[n], signature[n]); On 2013/02/04 22:53:27, Ryan Sleevi wrote: > Use SCOPED_TRACE here, as otherwise the gtest output will not indicate n. > I've changed the code to output this through LOG(ERROR) since ASSERT_ and EXPECT_ functions can't be used on functions that don't return void. > nit/todo: Seems like we should define a custom pred_formater for handling binary > output as hex, given the frequency of "compare two byte buffers" comes up. https://codereview.chromium.org/11571059/diff/51001/net/data/ssl/certificates... File net/data/ssl/certificates/README (right): https://codereview.chromium.org/11571059/diff/51001/net/data/ssl/certificates... net/data/ssl/certificates/README:143: - android-test-key-ecdsa-pubic.der On 2013/02/04 22:53:27, Ryan Sleevi wrote: > update these with the new file names. Done.
https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... File net/android/keystore_openssl.cc (right): https://codereview.chromium.org/11571059/diff/51001/net/android/keystore_open... net/android/keystore_openssl.cc:506: return s_ecdsa_ex_data_index; On 2013/02/05 14:31:58, digit1 wrote: > On 2013/02/04 22:53:27, Ryan Sleevi wrote: > > So it's clear for general reference, I don't think this is the general > > recommended Chromium practice. > > > > The only time we use pthread_once in Chromium code is in explicit sandbox init > > code. I cannot help but feel this is the wrong abstraction, in how it couples > > things, even if this is "just Android" code. > > I'm not sure I understand. Are you saying it's not ok to use platform-specific > APIs in platform-specific code? I'm saying I don't know, but that I hadn't seen this pattern by looking through codesearch. It may have been that we never needed to use pthread_once in our code, but it just was something that surprised me. > > The only alternative provided by base/ to to thread-safe lazy initialization are > Singleton and LazyInstance. Singleton requires creating a heap-based object, > which seems overkill here, so I've used a LazyInstance instead. LazyInstance seems fine. https://codereview.chromium.org/11571059/diff/59001/net/android/keystore_open... File net/android/keystore_openssl.cc (right): https://codereview.chromium.org/11571059/diff/59001/net/android/keystore_open... net/android/keystore_openssl.cc:196: private_key, message_piece, &result)) { This should be +4 from the ( (!...RawSignDigestWithPrivateKey( private_key, ...) However, is there a reason that this code in the unnamed namespace isn't entirely within net::android first (line 99)? namespace net { namespace android { namespace { That would avoid needing to do all the net::android::Foo functions here. https://codereview.chromium.org/11571059/diff/59001/net/android/keystore_open... net/android/keystore_openssl.cc:363: &signature)) { Did you fail to upload that change? I don't see it in the inter-diff https://codereview.chromium.org/11571059/diff/59001/net/android/keystore_open... net/android/keystore_openssl.cc:550: private_key, digest, &signature)) { still wrong indent. It should be 4 from the if() indent (which is 4) - meaning a total of 8. https://codereview.chromium.org/11571059/diff/59001/net/android/keystore_unit... File net/android/keystore_unittest.cc (right): https://codereview.chromium.org/11571059/diff/59001/net/android/keystore_unit... net/android/keystore_unittest.cc:111: unsigned char* OpenSSLWriteInto(std::string* str, size_t size) { NACK on this. The point of using WriteInto was to centralize the places where we violate the STL. And the string + 1 is intentional, again due to the guarantees of the STL and how it's implemented.
https://codereview.chromium.org/11571059/diff/59001/net/android/keystore_open... File net/android/keystore_openssl.cc (right): https://codereview.chromium.org/11571059/diff/59001/net/android/keystore_open... net/android/keystore_openssl.cc:196: private_key, message_piece, &result)) { On 2013/02/06 22:48:02, Ryan Sleevi wrote: > This should be +4 from the ( > (!...RawSignDigestWithPrivateKey( > private_key, ...) > Ok, I didn't realize line continuation indents accumulated. I'll fix this here and other parts. > However, is there a reason that this code in the unnamed namespace isn't > entirely within net::android first (line 99)? > > namespace net { > namespace android { > namespace { > I guess personal preference. Putting it in a single anonymous namespace tends to make compiler warnings and error messages less verbose during development. Also, I prefer to be explicit about the net::android:: calls here to distinguish properly which outside functions are being called. But if you prefer I can remove this. I don't think it really matters much in the end. > > That would avoid needing to do all the net::android::Foo functions here. https://codereview.chromium.org/11571059/diff/59001/net/android/keystore_open... net/android/keystore_openssl.cc:363: &signature)) { On 2013/02/06 22:48:02, Ryan Sleevi wrote: > Did you fail to upload that change? I don't see it in the inter-diff Done. https://codereview.chromium.org/11571059/diff/59001/net/android/keystore_open... net/android/keystore_openssl.cc:550: private_key, digest, &signature)) { On 2013/02/06 22:48:02, Ryan Sleevi wrote: > still wrong indent. It should be 4 from the if() indent (which is 4) - meaning a > total of 8. Done. https://codereview.chromium.org/11571059/diff/59001/net/android/keystore_unit... File net/android/keystore_unittest.cc (right): https://codereview.chromium.org/11571059/diff/59001/net/android/keystore_unit... net/android/keystore_unittest.cc:111: unsigned char* OpenSSLWriteInto(std::string* str, size_t size) { I see, I've fixed this, thanks for the clarification.
https://codereview.chromium.org/11571059/diff/61005/net/android/keystore_open... File net/android/keystore_openssl.cc (right): https://codereview.chromium.org/11571059/diff/61005/net/android/keystore_open... net/android/keystore_openssl.cc:230: /* .flags = */ RSA_METHOD_FLAG_NO_CHECK, Fyi, this issue was detected through manual testing (and a bit of OpenSSL spelunking :-)).
LGTM, mod nit https://codereview.chromium.org/11571059/diff/61005/net/android/keystore_open... File net/android/keystore_openssl.cc (right): https://codereview.chromium.org/11571059/diff/61005/net/android/keystore_open... net/android/keystore_openssl.cc:369: &signature)) { style nit: still wrong if (!RawSignDigestWithPrivateKey( private_key, base::StringPiece( ... https://codereview.chromium.org/11571059/diff/61005/net/android/keystore_unit... File net/android/keystore_unittest.cc (right): https://codereview.chromium.org/11571059/diff/61005/net/android/keystore_unit... net/android/keystore_unittest.cc:109: return reinterpret_cast<unsigned char*>(WriteInto(str, size+1)); nit: "size+1" -> "size + 1" (whitespace)
https://codereview.chromium.org/11571059/diff/61005/net/android/keystore_open... File net/android/keystore_openssl.cc (right): https://codereview.chromium.org/11571059/diff/61005/net/android/keystore_open... net/android/keystore_openssl.cc:369: &signature)) { Oh damn, done. thanks. https://codereview.chromium.org/11571059/diff/61005/net/android/keystore_unit... File net/android/keystore_unittest.cc (right): https://codereview.chromium.org/11571059/diff/61005/net/android/keystore_unit... net/android/keystore_unittest.cc:109: return reinterpret_cast<unsigned char*>(WriteInto(str, size+1)); On 2013/02/08 22:43:32, Ryan Sleevi wrote: > nit: "size+1" -> "size + 1" (whitespace) Done.
I just added bulach and yfriedman as reviewers, since they are base/android OWNERS. Guys, can I ask you to take a look at the base/android/jni_array.h change. Thanks in advance.
lgtm, thanks digit! just some drive-by suggestions below: https://codereview.chromium.org/11571059/diff/73016/net/android/java/src/org/... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://codereview.chromium.org/11571059/diff/73016/net/android/java/src/org/... net/android/java/src/org/chromium/net/AndroidKeyStore.java:31: private static final String TAG = AndroidKeyStore.class.getName(); nit: the next file has this string hardcoded, I think it's more common to do so.. https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_open... File net/android/keystore_openssl.cc (right): https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_open... net/android/keystore_openssl.cc:162: // Ensure the global JNI reference is destroyed with this key. nit: perhaps just to clarify the flow: // Ensure it destroys the global JNI reference transferred by // RSA_set_app_data when this key wrapper was created by // GetRsaPkeyWrapper. https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_open... net/android/keystore_openssl.cc:409: // Free the global JNI reference. nit: as above, maybe clarify where this was acquired https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_open... net/android/keystore_openssl.cc:480: // custom EC_KEY + ECDSA_METHOD is released when the key is destroyed. nit: as above https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_unit... File net/android/keystore_unittest.cc (right): https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_unit... net/android/keystore_unittest.cc:141: // Convert to PKCS#8 object. nit: unindent 141-153 https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_unit... net/android/keystore_unittest.cc:325: { nit: I think this goes on the case line (same below..) https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_unit... net/android/keystore_unittest.cc:432: << "actual " << signature[n] << ", expected " nit: maybe base::HexEncode for the byte values? https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_unit... net/android/keystore_unittest.cc:489: InitEnv(); nit: these two lines seem to be repeated, consider moving to a SetUp() method in a test harness and use TEST_F rather than TEST macros https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_unit... net/android/keystore_unittest.cc:500: ScopedJava key_java = GetPKCS8PrivateKeyJava(PRIVATE_KEY_TYPE_RSA, nit: first param in the next line
https://codereview.chromium.org/11571059/diff/73016/net/android/java/src/org/... File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://codereview.chromium.org/11571059/diff/73016/net/android/java/src/org/... net/android/java/src/org/chromium/net/AndroidKeyStore.java:31: private static final String TAG = AndroidKeyStore.class.getName(); On 2013/02/11 11:59:05, bulach wrote: > nit: the next file has this string hardcoded, I think it's more common to do > so.. Done. https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_open... File net/android/keystore_openssl.cc (right): https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_open... net/android/keystore_openssl.cc:162: // Ensure the global JNI reference is destroyed with this key. On 2013/02/11 11:59:05, bulach wrote: > nit: perhaps just to clarify the flow: > > // Ensure it destroys the global JNI reference transferred by > // RSA_set_app_data when this key wrapper was created by > // GetRsaPkeyWrapper. I've clarified the comment a bit, but I prefer not to refer to the function where the object is created, because this is bounded to change over time. https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_open... net/android/keystore_openssl.cc:409: // Free the global JNI reference. On 2013/02/11 11:59:05, bulach wrote: > nit: as above, maybe clarify where this was acquired Done. https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_open... net/android/keystore_openssl.cc:480: // custom EC_KEY + ECDSA_METHOD is released when the key is destroyed. On 2013/02/11 11:59:05, bulach wrote: > nit: as above Done. https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_unit... File net/android/keystore_unittest.cc (right): https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_unit... net/android/keystore_unittest.cc:141: // Convert to PKCS#8 object. On 2013/02/11 11:59:05, bulach wrote: > nit: unindent 141-153 Done. https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_unit... net/android/keystore_unittest.cc:325: { On 2013/02/11 11:59:05, bulach wrote: > nit: I think this goes on the case line (same below..) Done. https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_unit... net/android/keystore_unittest.cc:432: << "actual " << signature[n] << ", expected " On 2013/02/11 11:59:05, bulach wrote: > nit: maybe base::HexEncode for the byte values? Done. https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_unit... net/android/keystore_unittest.cc:489: InitEnv(); On 2013/02/11 11:59:05, bulach wrote: > nit: these two lines seem to be repeated, consider moving to a SetUp() method in > a test harness and use TEST_F rather than TEST macros InitEnv() doesn't repeat much. And it's not possible to move the err_trace(FROM_HERE) in a common SetupTest() because the FROM_HERE macro expands to a source-specific value, so this won't be correct (i.e. would not report the current location). In other words, it's probably not very useful here. https://codereview.chromium.org/11571059/diff/73016/net/android/keystore_unit... net/android/keystore_unittest.cc:500: ScopedJava key_java = GetPKCS8PrivateKeyJava(PRIVATE_KEY_TYPE_RSA, On 2013/02/11 11:59:05, bulach wrote: > nit: first param in the next line I believe this line follows the Google coding style closely (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...). I haven't found anything in the Chromium or Google style guide about forcing the first parameter on the next line in this case. I understand it's acceptable in cases, but not mandatory. Let me know otherwise.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/11571059/64006
Message was sent while issue was closed.
Change committed as 181741 |
