Ship minimal PBKDF2-SHA256 native library for Android

RESOLVED FIXED in Firefox 29

Status

Android Background Services
Crypto
P2
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: rnewman, Assigned: mcomella)

Tracking

(Blocks: 4 bugs)

unspecified
Firefox 30
All
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox29 fixed, firefox30 fixed, fennec30+)

Details

(Whiteboard: [qa-])

Attachments

(6 attachments, 25 obsolete attachments)

57 bytes, text/x-github-pull-request
nalexander
: review+
Details | Review | Splinter Review
14.74 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
4.30 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
11.13 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
8.52 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
2.62 KB, patch
glandium
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
See also Bug 912275.

This starts off on my plate (ripping code out of OpenSSL), and will probably move over to Nick to land.
(Reporter)

Comment 1

4 years ago
scrypt's own PBKDF2-SHA256 implementation bumps our key stretching time from 7.3 to 8.5 seconds, but it's much easier to ship than OpenSSL's…
Whiteboard: [qa-]
(Reporter)

Updated

4 years ago
Blocks: 905433
(Reporter)

Comment 2

4 years ago
https://github.com/mozilla-services/android-sync/pull/353
Flags: needinfo?(nalexander)
This starts by building an android-sync branch with commits 4fc71c8..33ed0fe and then updating ./fennec-copy-code.sh to land these pieces into mozilla-central.  rnewman and I discussed landing into mobile/android/components/crypto or similar.

Then we need to add mobile/android/components/crypto/Makefile.in that builds a libcrypto.so file.

Then we need to get that libcrypto.so linked against the build; this requires editing packager.mk and treating our new libcrypto.so like libmozglue.so.
Flags: needinfo?(nalexander)
Here's a preliminary try build with some robocop tests hacked to check this:

https://tbpl.mozilla.org/?tree=Try&rev=b73a477edbd4
(In reply to Nick Alexander :nalexander from comment #4)
> Here's a preliminary try build with some robocop tests hacked to check this:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=b73a477edbd4

You can see the goodness at testJarReader.
I'm not going to go so far as to split this ticket right now, but current thinking [1] is that the FxA setup will require only about 1k rounds of PBKDF2.  This is definitely outside of "needs native crypto" territory, but I think we should still land native PBKDF2 for perf reasons.  Landing native scrypt is more speculative.

[1] https://github.com/mozilla/fxa-auth-server/wiki/onepw-protocol
Created attachment 8355045 [details] [diff] [review]
915312.1.patch

Straight copy of just sha256.{c,h}.
Attachment #8355045 - Flags: review?(rnewman)
Created attachment 8355046 [details] [diff] [review]
915312.2.patch

Part 2.  This actually gets used in a part 3, github link forthcoming.

rnewman for structure, glandium for build system.

glandium: this is intended to *not* link against libmozglue.  The generated library will be opened using System.loadLibrary from code that runs from a background service (i.e., when Gecko is not around and shouldn't be loaded).

rnewman: we'll just copy the built .so files into android-sync for now.  See github link.
Attachment #8355046 - Flags: review?(rnewman)
Attachment #8355046 - Flags: review?(mh+mozilla)
Created attachment 8355047 [details] [review]
Part 3 on github.
Attachment #8355047 - Flags: review?(rnewman)
As far as I know we tend to avoid adding crypto code and use what we already have instead. Note using  libmozglue doesn't mean you have to use or load gecko. So I think it would be better to load nss and use its crypto code instead of importing new code to do that. But that's only my bystander view. Brian, what do you think?
(Reporter)

Comment 11

4 years ago
Last I checked, NSS didn't implement PBKDF2-SHA256 -- Bug 554827 -- so it's not suitable for our purposes. (I doubt that bug will be fixed and landed in Firefox 29, given that my comment in *July* still hasn't been answered.)

And in any case, using NSS (11MB) just to do PBKDF2 (which our own native code does in ~80KB, IIRC) seems like overkill.
(Reporter)

Comment 12

4 years ago
Comment on attachment 8355045 [details] [diff] [review]
915312.1.patch

Review of attachment 8355045 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/components/crypto/sha256.c
@@ +22,5 @@
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +#include "scrypt_platform.h"

Where's this file? (It's upstream: <https://github.com/mozilla-services/android-sync/blob/a931b473527685f9e9d610af223c2395a0054be5/jni/scrypt/include/scrypt_platform.h>)
(Reporter)

Comment 13

4 years ago
Comment on attachment 8355046 [details] [diff] [review]
915312.2.patch

Review of attachment 8355046 [details] [diff] [review]:
-----------------------------------------------------------------

I can't technically review nativecrypto.c, 'cos I wrote it, remember? :)

So treat this as r+ from me on the import, and r+ from you on the code!

::: mobile/android/components/crypto/nativecrypto.c
@@ +22,5 @@
> +  uint64_t cc;
> +  size_t dk;
> +  jbyteArray result;
> +
> +	jbyte *pj = (*env)->GetByteArrayElements(env, password, 0);

Indentation.

::: mobile/android/components/crypto/sha256.c
@@ -22,5 @@
>   * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
>   * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>   * SUCH DAMAGE.
>   */
> -#include "scrypt_platform.h"

Aha. Could you fold this change into the previous patch?
Attachment #8355046 - Flags: review?(rnewman) → review+
(Reporter)

Updated

4 years ago
Attachment #8355045 - Flags: review?(rnewman) → review+
(Reporter)

Updated

4 years ago
Attachment #8355047 - Flags: review?(rnewman) → review+
(In reply to Mike Hommey [:glandium] from comment #10)
> As far as I know we tend to avoid adding crypto code and use what we already
> have instead. Note using  libmozglue doesn't mean you have to use or load
> gecko. So I think it would be better to load nss and use its crypto code
> instead of importing new code to do that. But that's only my bystander view.
> Brian, what do you think?

It is easy to add SHA-2 support to NSS's PBKDF2 implementation, e.g. following the pattern used for HKDF in https://hg.mozilla.org/projects/nss/annotate/2380cf2250c4/lib/softoken/pkcs11c.c#l6540.

To put what I've previously said politely more bluntly, I think that scrypt is a very bad choice for this application. scrypt requires a huge amount of memory to be useful and the application we're using it for is heavily memory constrained. The "scrypt-helper" server idea is very clever but it seems like a very poor latency/complexity/reliability tradeoff. The concern about using PBKDF2 exclusively is that GPUs and ASICs are much faster than CPUs, so attackers would have the upper hand. But, if we GPU-accelerated our PKBDF2 implementation then that concern would be better mitigated.

Adding scrypt support to NSS would be possible but unfun. If somebody wants to try, I recommend following the example of my HKDF patch, which is one of the first NSS patches I wrote. Or, people can just use a standalone scrypt. The standalone one would probably be better because it wouldn't take resources away from NSS people to review it.

If we're going to use a traditional (non-GPU-accelerated) PBKDF2 then it would be best, IMO, to extend NSS's PKBDF2 implementation to support SHA2 (following the HKDF example) instead of having a separate implementation and I'm happy to help ensure that that patch gets reviewed promptly. However, if the people working on this bug think a standalone implementation is better and they have qualified reviewers for a new PBKDF2 implementation from outside the PSM/NSS teams then I don't really care; I don't think that this *has* to be in NSS, especially if we'd be better off in the long term do a GPU-accelerated version which probably would live outside of NSS anyway.

FWIW, there are some non-technical reasons why it is better to centralize crypto code in NSS, but those reasons don't apply here, AFAICT. Nobody in an environment where crypto certification and whatnot is important is going to be using the Sync/Accounts feature for regulated work.
(In reply to Mike Hommey [:glandium] from comment #10)
> As far as I know we tend to avoid adding crypto code and use what we already
> have instead. Note using  libmozglue doesn't mean you have to use or load
> gecko. So I think it would be better to load nss and use its crypto code
> instead of importing new code to do that. But that's only my bystander view.
> Brian, what do you think?

It is easy to add SHA-2 support to NSS's PBKDF2 implementation, e.g. following the pattern used for HKDF in https://hg.mozilla.org/projects/nss/annotate/2380cf2250c4/lib/softoken/pkcs11c.c#l6540.

To put what I've previously said politely more bluntly, I think that scrypt is a very bad choice for this application. scrypt requires a huge amount of memory to be useful and the application we're using it for is heavily memory constrained. The "scrypt-helper" server idea is very clever but it seems like a very poor latency/complexity/reliability tradeoff. The concern about using PBKDF2 exclusively is that GPUs and ASICs are much faster than CPUs, so attackers would have the upper hand. But, if we GPU-accelerated our PKBDF2 implementation then that concern would be better mitigated.

Adding scrypt support to NSS would be possible but unfun. If somebody wants to try, I recommend following the example of my HKDF patch, which is one of the first NSS patches I wrote. Or, people can just use a standalone scrypt. The standalone one would probably be better because it wouldn't take resources away from NSS people to review it.

If we're going to use a traditional (non-GPU-accelerated) PBKDF2 then it would be best, IMO, to extend NSS's PKBDF2 implementation to support SHA2 (following the HKDF example) instead of having a separate implementation and I'm happy to help ensure that that patch gets reviewed promptly. However, if the people working on this bug think a standalone implementation is better and they have qualified reviewers for a new PBKDF2 implementation from outside the PSM/NSS teams then I don't really care; I don't think that this *has* to be in NSS, especially if we'd be better off in the long term do a GPU-accelerated version which probably would live outside of NSS anyway.

FWIW, there are some non-technical reasons why it is better to centralize crypto code in NSS, but those reasons don't apply here, AFAICT. Nobody in an environment where crypto certification and whatnot is important is going to be using the Sync/Accounts feature for regulated work.
(Reporter)

Comment 16

4 years ago
Thanks for the quick response, Brian!

(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #14)

> It is easy to add SHA-2 support to NSS's PBKDF2 implementation

Can that (Bug 554827) land in m-c before the 29 merge (and ideally next week)? If not, how long would it take?

 
> To put what I've previously said politely more bluntly, I think that scrypt
> is a very bad choice for this application. scrypt requires a huge amount of
> memory to be useful and the application we're using it for is heavily memory
> constrained. The "scrypt-helper" server idea is very clever but it seems
> like a very poor latency/complexity/reliability tradeoff.

As I understand it, the current "onepw" Firefox Account auth scheme doesn't use client-side scrypt at all, and it's not part of the code we're landing here.

(Also as I understand it: things are heading towards a weaker default, and an optional standalone key, Just Like Chrome®.)


> using PBKDF2 exclusively is that GPUs and ASICs are much faster than CPUs,
> so attackers would have the upper hand. But, if we GPU-accelerated our
> PKBDF2 implementation then that concern would be better mitigated.

Have you talked through this concern with the folks behind "onepw" (warner, others?)?


> I'm happy to help ensure that that patch gets reviewed promptly. However, if
> the people working on this bug think a standalone implementation is better
> and they have qualified reviewers for a new PBKDF2 implementation from
> outside the PSM/NSS teams then I don't really care; I don't think that this
> *has* to be in NSS, especially if we'd be better off in the long term do a
> GPU-accelerated version which probably would live outside of NSS anyway.

The main concerns here (in my mind, at least) are summarized in Comment 11:

* whether loading NSS during FxA setup is an unacceptable memory or time expense (I haven't measured)
* whether we can get an NSS implementation of PBKDF2-SHA256, and appropriate JNI calling code, that's in the same perf ballpark as this one, landed in a massive and externally imposed rush.
Flags: needinfo?(brian)
Summary: Ship minimal PBKDF2-SHA256 and scrypt native libraries for Android → Ship minimal PBKDF2-SHA256 native library for Android
I read this to say that bsmith has no major concern with shipping this code specifically for FxAccounts.  FxAccounts team has a month to get a v1 shipped, and we can't afford the hours to land PBKDF2 + SHA256 in NSS and get the JNI code working in that time frame.  If this is something we want for the future, let's land PBKDF2 + SHA256 in NSS (Bug 554827) and file a follow-up to use libnss3 via JNI from Android background services.

re: libmozglue, there's no reason for Android background services to load it.

re: sec review of this code: fair point.  We have sec team people involved already (dchan, sarentz).  I'll ask their opinion.

glandium, can we get your comments on the build system pieces?
Flags: needinfo?(sarentz)
Flags: needinfo?(dchan)
Comment on attachment 8355046 [details] [diff] [review]
915312.2.patch

Review of attachment 8355046 [details] [diff] [review]:
-----------------------------------------------------------------

If you don't want to go the NSS route, fine. I still don't see why this would have to be in a separate library though. Why not just put it in mozglue? After all it's not a big library, and loading it doesn't make you *have* to load anything with the custom linker.
Attachment #8355046 - Flags: review?(mh+mozilla)
(In reply to Richard Newman [:rnewman] from comment #16)
> > It is easy to add SHA-2 support to NSS's PBKDF2 implementation
> 
> Can that (Bug 554827) land in m-c before the 29 merge (and ideally next
> week)? If not, how long would it take?

If somebody writes the patch, I can review it this week. However...

> Have you talked through this concern with the folks behind "onepw" (warner,
> others?)?

Yes, I shared my thoughts on this with bwarner a very long time ago (1 or 2 years ago), and again last month, in much more detail than I gave in my comment above.

> The main concerns here (in my mind, at least) are summarized in Comment 11:
> 
> * whether loading NSS during FxA setup is an unacceptable memory or time
> expense (I haven't measured)

To be clear, I think that this is a valid thing to be concerned about. But, if we're concerned about the memory overhead or added latency of loading NSS then there's no way we can do scrypt, because scrypt requires much more memory when computed locally and scrypt helper adds much more latency than loading NSS.
Flags: needinfo?(brian)
(In reply to Nick Alexander :nalexander from comment #17)
> I read this to say that bsmith has no major concern with shipping this code
> specifically for FxAccounts.  FxAccounts team has a month to get a v1
> shipped, and we can't afford the hours to land PBKDF2 + SHA256 in NSS and
> get the JNI code working in that time frame.  If this is something we want
> for the future, let's land PBKDF2 + SHA256 in NSS (Bug 554827) and file a
> follow-up to use libnss3 via JNI from Android background services.

In case it isn't clear, I agree that this is a reasonable path to take.

Updated

4 years ago
No longer blocks: 905433

Updated

4 years ago
tracking-fennec: --- → 30+
Priority: -- → P2
(Reporter)

Updated

4 years ago
Blocks: 799726, 967996
Blocks: 959652
As discussed on IRC, I taking this over which entails adding sha1 for use in bug 959652, and shipping this as part of mozglue.
Assignee: rnewman → michael.l.comella
Created attachment 8375719 [details] [diff] [review]
Part 2: Add sha_fast (sha1) directly.

Add sha1 directly from nss in my local checkout of m-c. I assume we shouldn't symlink, or use the files directly, so our things don't break if the libraries are ever updated.
Attachment #8375719 - Flags: review?(nalexander)
Created attachment 8375720 [details] [diff] [review]
Part 3: Rename sha_fast to sha1.

Renamed to sha1 from sha_fast.
Attachment #8375720 - Flags: review?(nalexander)
Created attachment 8375722 [details] [diff] [review]
Part 4: Add sha1 deps directly.

Add the sha1.c/h deps directly from <m-c>/nspr/pr/include/.
Attachment #8375722 - Flags: review?(nalexander)
Created attachment 8375724 [details] [diff] [review]
Part 5: Fix local compilation errors.

Make the appropriate changes to get these files compiling locally (i.e. ) sans . Is this what you had in mind?
Attachment #8375724 - Flags: review?(nalexander)
(In reply to Michael Comella (:mcomella) from comment #25)
> Make the appropriate changes to get these files compiling locally (i.e. )
> sans . Is this what you had in mind?

Ha ha, used backticks in the shell. Should read:

Make the appropriate changes to get these files compiling locally (i.e. `gcc sha1.c`) sans `int main() { }`. Is this what you had in mind?

Note that I messed up in part 3 - I changed `#include "prcpucfg.h" to `#include <nspr/prcpucfg.h>` because it wouldn't compile otherwise on my machine. I'll fix that if you think these patches are in the right direction.
* I'm not entirely against this approach, but it's not what I expected.

* Is there a part 1?  It's best to build on top of the WIP code we have for PBKDF2, if you're not.  That shows the way.

* I don't think this approach for importing a bunch of PR_* stuff is correct; that should all be available to libmozglue (in some way), like http://mxr.mozilla.org/mozilla-central/source/mozglue/android/NSSBridge.cpp#33.

* I'd need to see the actual moz.build/Makefile.in stuff (build on what we already have) to know how painful the approach here is.  Sorry :(

* I had expected us to import a smaller, self contained sha1 implementation, such as (consults Google) http://oauth.googlecode.com/svn../code/c/liboauth/src/sha1.c  This is almost certainly easier than whatever we're doing.  Consider it?

If you think you can make this NSS code work, roll on.  But I strongly recommend importing a simpler (one file, no deps, minimal includes) implementation that you can get working from Java in the old framework (libnativecrypto) and we then migrate to libmozglue rather than trying to coerce NSS code to our will.
Attachment #8375720 - Flags: review?(nalexander)
Attachment #8375719 - Flags: review?(nalexander)
Attachment #8375722 - Flags: review?(nalexander)
Attachment #8375724 - Flags: review?(nalexander)
(In reply to Nick Alexander :nalexander from comment #27)
> * Is there a part 1?  It's best to build on top of the WIP code we have for
> PBKDF2, if you're not.  That shows the way.

That was the plan - part 1 is your patch attached here.

> * I had expected us to import a smaller, self contained sha1 implementation,
> such as (consults Google)
> http://oauth.googlecode.com/svn../code/c/liboauth/src/sha1.c  This is almost
> certainly easier than whatever we're doing.  Consider it?

I was trying to go with the larger lib implementations (while crossing my fingers at minimal deps) because I assumed they're more likely to be correct, secure, and well tested. But yes, from a convenience and size perspective, I'd much prefer to go with a smaller standalone - I'll try that instead.
Created attachment 8378662 [details] [diff] [review]
Part 1: Initial commit. r=rnewman
Created attachment 8378663 [details] [diff] [review]
Part 2: Build native crypto into mozglue. r=glandium

This isn't at all ready to land:

* need to split into two commits, one native code + build, the other
  Java and tests;
* need to make sure test is reasonable on infra;
* need to land on a-s;

but it should serve as a base for SHA1 work.
These patches don't seem to work - it still complains about the missing library. Perhaps in your testing you had an old .a lying around in your objdir (I incorrectly did that when I first compiled this)?

I agree with your hypothesis on IRC that it's probably a build ordering issue - any ideas on how to fix that?
Comment on attachment 8375719 [details] [diff] [review]
Part 2: Add sha_fast (sha1) directly.

Obsoleting patches using nss for sha1.
Attachment #8375719 - Attachment is obsolete: true
Attachment #8375720 - Attachment is obsolete: true
Attachment #8375722 - Attachment is obsolete: true
Attachment #8375724 - Attachment is obsolete: true
If you're going to use SHA-1, there's already an implementation in mfbt (so, in mozglue).
(Reporter)

Updated

4 years ago
Whiteboard: [qa-] → [qa-][parallel]
Created attachment 8381531 [details] [review]
Part 3 (github) - Add NativeCrypto.java to expose native routines

Will build upon patch 1 (importing source), 2 (compiling into mozglue), and 4 (testing); these will be uploaded later.
Attachment #8355047 - Attachment is obsolete: true
Attachment #8381531 - Flags: review?(nalexander)
Created attachment 8381537 [details] [diff] [review]
Part 1: Directly import sha files.
Comment on attachment 8381537 [details] [diff] [review]
Part 1: Directly import sha files.

Review of attachment 8381537 [details] [diff] [review]:
-----------------------------------------------------------------

Same as nalexander's previous patch, but moved to <m-c>/mozglue/android/crypto/
Attachment #8381537 - Flags: review?(rnewman)
Attachment #8355045 - Attachment is obsolete: true
Comment on attachment 8378662 [details] [diff] [review]
Part 1: Initial commit. r=rnewman

Obsoleting mozglue patches built into .../components/crypto/, which could not be built there because mozglue is built before that directly is reached
Attachment #8378662 - Attachment is obsolete: true
Attachment #8378663 - Attachment is obsolete: true
Created attachment 8381791 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

rnewman for compile changes, glandium for build. Similar to nalexander's part 2.
Attachment #8381791 - Flags: review?(mh+mozilla)
Attachment #8381791 - Flags: review?(rnewman)
Created attachment 8381798 [details] [diff] [review]
Part 4: Test native crypto.

Tests for sha256 are from nalexander's patch. I added SHA-1 tests.
Attachment #8381798 - Flags: review?(rnewman)
Attachment #8355046 - Attachment is obsolete: true
(Reporter)

Comment 40

4 years ago
Comment on attachment 8381537 [details] [diff] [review]
Part 1: Directly import sha files.

Review of attachment 8381537 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/android/crypto/sha256.c
@@ +8,5 @@
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.

You should ping licensing@mozilla.org (or a legal person directly) to ask about the correct way to follow this second requirement -- to include the copyright notice in Fennec.
Attachment #8381537 - Flags: review?(rnewman) → review+
(Reporter)

Comment 41

4 years ago
Comment on attachment 8381791 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

Review of attachment 8381791 [details] [diff] [review]:
-----------------------------------------------------------------

Please push this through try to spot failures on supported platforms.

This looks good to me, with obvious caveats that I'm about as familiar with C++ as I am with large-scale logging operations.

::: mozglue/android/crypto/nativecrypto.cpp
@@ +16,5 @@
> +/**
> + * Helper function to invoke native PBKDF2 function with JNI
> + * arguments.
> + */
> +extern "C" JNIEXPORT jbyteArray JNICALL Java_org_mozilla_gecko_sync_crypto_NativeCrypto_pbkdf2SHA256

Does this match the name you want it to have in Java?

@@ +34,5 @@
> +
> +  uint8_t *out = (uint8_t *) malloc(sizeof(uint8_t) * dkLen);
> +
> +  if (out == NULL) {
> +    return NULL;

This will leak the two arrays. You need to release them as you do in 46/47.

@@ +38,5 @@
> +    return NULL;
> +  }
> +
> +  cc = (uint64_t) c;
> +  dk = (size_t) dkLen;

Just do these casts where you declare the variables.

@@ +45,5 @@
> +
> +  env->ReleaseByteArrayElements(password, pj, JNI_ABORT);
> +  env->ReleaseByteArrayElements(salt, sj, JNI_ABORT);
> +
> +  result = env->NewByteArray(dkLen);

This can return NULL. In that case, you need to free `out` and return NULL.

@@ +71,5 @@
> +  sha1.finish(hashResult);
> +
> +  env->ReleaseByteArrayElements(jstr, str, JNI_ABORT);
> +
> +  out = env->NewByteArray(SHA1Sum::HashSize);

Check return value.

::: mozglue/android/crypto/sha256.c
@@ +37,5 @@
> +{
> +	const uint8_t *p = (uint8_t const *)pp;
> +
> +	return ((uint32_t)(p[3]) + ((uint32_t)(p[2]) << 8) +
> +	    ((uint32_t)(p[1]) << 16) + ((uint32_t)(p[0]) << 24));

Newline after each +, align.
Attachment #8381791 - Flags: review?(rnewman) → review+
(Reporter)

Comment 42

4 years ago
Comment on attachment 8381798 [details] [diff] [review]
Part 4: Test native crypto.

Review of attachment 8381798 [details] [diff] [review]:
-----------------------------------------------------------------

Much of this code looks like I wrote it, so I gently question the fairness of me reviewing it. But assuming the test vectors are correct, lgtm.

::: mobile/android/base/tests/testNativeCrypto.java
@@ +1,1 @@
> +package org.mozilla.gecko.tests;

License.

@@ +1,3 @@
> +package org.mozilla.gecko.tests;
> +
> +import static org.mozilla.gecko.tests.helpers.AssertionHelper.*;

*sheds a tear*

@@ +15,5 @@
> +
> +// Test vectors from
> +// SHA-256: <https://github.com/ircmaxell/PHP-PasswordLib/blob/master/test/Data/Vectors/pbkdf2-draft-josefsson-sha256.test-vectors>
> +//           <https://gitorious.org/scrypt/nettle-scrypt/blobs/37c0d5288e991604fe33dba2f1724986a8dddf56/testsuite/pbkdf2-test.c
> +// SHA-1: <http://oauth.googlecode.com/svn/code/c/liboauth/src/sha1.c>

Block comment.

@@ +21,5 @@
> +  /**
> +   * Robocop supports only a single test function per test class. Therefore, we
> +   * have a single top-level test function that dispatches to sub-tests,
> +   * accepting that we might fail part way through the cycle. Proper JUnit 3
> +   * testing can't land soon enough!

File a follow-up depending on that.

@@ +116,5 @@
> +  }
> +
> +  /**
> +   * Test to ensure the output of our SHA1 algo is the same as MessageDigest's. This is important
> +   * because we intend to replace MessageDigest in FHR with this SHA-1 algo (bug 959652).

Thank you!

@@ +138,5 @@
> +  }
> +
> +  private void checkPBKDF2SHA256(String p, String s, int c, int dkLen, final String expectedStr)
> +      throws GeneralSecurityException, UnsupportedEncodingException {
> +    final long start = System.currentTimeMillis();

Let's use 

http://developer.android.com/reference/android/os/SystemClock.html#elapsedRealtime%28%29
Attachment #8381798 - Flags: review?(rnewman) → review+
Created attachment 8382539 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

(In reply to Richard Newman [:rnewman] from comment #41)
> This looks good to me, with obvious caveats that I'm about as familiar with
> C++ as I am with large-scale logging operations.

Should we have a second r?

> Does this match the name you want it to have in Java?

Appears to. Do you see otherwise?

I largerly rewrote the sha256 method (I should really look more carefully at
code when I take over patches!) to avoid the memory leak issues and ridiculous
naming. It's now similar to the sha-1 method, so hopefully easier to read!
Sorry for the double review request.

> Check return value.

Done!

> Newline after each +, align.

Done.
Attachment #8382539 - Flags: review?(rnewman)
Attachment #8381791 - Attachment is obsolete: true
Attachment #8381791 - Flags: review?(mh+mozilla)
Attachment #8382539 - Flags: review?(mh+mozilla)
Blocks: 977335
Created attachment 8382554 [details] [diff] [review]
Part 4: Test native crypto.

(In reply to Richard Newman [:rnewman] from comment #42)
> Much of this code looks like I wrote it, so I gently question the fairness
> of me reviewing it. But assuming the test vectors are correct, lgtm.

Should I r? nalexander?

> License.

Done.

> Block comment.

Done, though I'm not sure how you'd feel about the formatting I did.

> File a follow-up depending on that.
bug 977335.

> Let's use 
> 
> http://developer.android.com/reference/android/os/SystemClock.
> html#elapsedRealtime%28%29

Done.
Attachment #8381798 - Attachment is obsolete: true
Comment on attachment 8382554 [details] [diff] [review]
Part 4: Test native crypto.

Move rnewman r+.
Attachment #8382554 - Flags: review+
These changes should only touch android (mozglue should not build <mozglue>/android/ on other platforms) and I wasn't sure if there were any test suites that cover mozglue so I just ran robocop (which should cover build, normal startup/usage, and particularly testNativeCrypto):

https://tbpl.mozilla.org/?tree=Try&rev=901127dc6301
I still think "crypto" is a misleading name.
Do you have any alternative recommendations?
hash?

Note, you should probably ask (Waldo?) if there's an interest in putting sha256 in mfbt, since there is sha1 there already.
(Reporter)

Comment 50

4 years ago
(In reply to Mike Hommey [:glandium] from comment #49)
> hash?

PBKDF2 is a key derivation algorithm. That counts as crypto in my book.
(Reporter)

Comment 51

4 years ago
(In reply to Michael Comella (:mcomella) from comment #44)
> Created attachment 8382554 [details] [diff] [review]
> Part 4: Test native crypto.
>
> Should I r? nalexander?

wfm.
(In reply to Richard Newman [:rnewman] from comment #50)
> (In reply to Mike Hommey [:glandium] from comment #49)
> > hash?
> 
> PBKDF2 is a key derivation algorithm. That counts as crypto in my book.

Then rename sha256.c
(Reporter)

Comment 53

4 years ago
Michael: could you please churn all of these patches to put the sha* files in hash/, and everything else in crypto/?
(Reporter)

Comment 54

4 years ago
Though it's worth noting: "SHA-2 is a set of *crypto*graphic hash functions…".
(Reporter)

Comment 55

4 years ago
Comment on attachment 8382539 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

Review of attachment 8382539 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.

::: mozglue/android/crypto/nativecrypto.cpp
@@ +16,5 @@
> +/**
> + * Helper function to invoke native PBKDF2 function with JNI
> + * arguments.
> + */
> +extern "C" JNIEXPORT jbyteArray JNICALL Java_org_mozilla_gecko_sync_crypto_NativeCrypto_pbkdf2SHA256

My question about the name was: did you want to keep 'sync' in there, given that this is now living in mozglue?
Attachment #8382539 - Flags: review?(rnewman) → review+
Comment on attachment 8382554 [details] [diff] [review]
Part 4: Test native crypto.

Requesting from nalexander because rnewman claims to have written most of this. :)
Attachment #8382554 - Flags: review?(nalexander)
Created attachment 8383364 [details] [diff] [review]
Part 1: Directly import sha files.

Rebased.
Attachment #8381537 - Attachment is obsolete: true
Attachment #8383364 - Flags: review+
Created attachment 8383367 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

Rebased.
Attachment #8382539 - Attachment is obsolete: true
Attachment #8382539 - Flags: review?(mh+mozilla)
Comment on attachment 8383367 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

Move sha* files to hash/ and kept nativecrypto* in crypto.
Attachment #8383367 - Flags: review?(mh+mozilla)
Comment on attachment 8383367 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

Move forward rnewman r+.
Attachment #8383367 - Flags: review+
Created attachment 8383370 [details] [diff] [review]
Part 4: Test native crypto.

Rebased.
Attachment #8383370 - Flags: review?(nalexander)
Attachment #8382554 - Attachment is obsolete: true
Attachment #8382554 - Flags: review?(nalexander)
Comment on attachment 8383370 [details] [diff] [review]
Part 4: Test native crypto.

Move forward rnewman r+.
Attachment #8383370 - Flags: review+
Comment on attachment 8383367 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

Review of attachment 8383367 [details] [diff] [review]:
-----------------------------------------------------------------

since the sha256* files contain more than sha256, they should probably be renamed to pbkdf2_sha256. I also don't see a reason for the subdirectories. nativecrypto should be NativeCrypto, too.
Attachment #8383367 - Flags: review?(mh+mozilla) → feedback-
Created attachment 8383881 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

Made the changes recommended in comment 63.
Attachment #8383881 - Flags: review?(mh+mozilla)
Attachment #8383367 - Attachment is obsolete: true
Created attachment 8384014 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

Moved NativeCrypto to omg.background.nativecode from part 3, as nalexander suggested, and updated these methods accordingly.
Attachment #8384014 - Flags: review?(mh+mozilla)
Attachment #8383881 - Attachment is obsolete: true
Attachment #8383881 - Flags: review?(mh+mozilla)
Created attachment 8384017 [details] [diff] [review]
Part 4: Test native crypto.

Rebase.
Attachment #8384017 - Flags: review?(nalexander)
Attachment #8383370 - Attachment is obsolete: true
Attachment #8383370 - Flags: review?(nalexander)
Sorry, I messed up the patches - working on an update. Hold off on a review until I'm finished please.
Created attachment 8384691 [details] [diff] [review]
Part 1: Directly import sha files.

Correction: Moved sha256.* to mozglue/android/ (from m/a/hash/).
Attachment #8383364 - Attachment is obsolete: true
Created attachment 8384696 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

Correction: Rebase on correction in part 1. Should be good to review.
Attachment #8384696 - Flags: review?(mh+mozilla)
Attachment #8384014 - Attachment is obsolete: true
Attachment #8384014 - Flags: review?(mh+mozilla)
Attachment #8384691 - Flags: review+
Comment on attachment 8381531 [details] [review]
Part 3 (github) - Add NativeCrypto.java to expose native routines

I folded this down into a few small commits, and fixed `mvn integration-test`.  If it's good for you, I'd like to land

https://github.com/mozilla-services/android-sync/tree/nalexander/bug-915312-squashed/

on a-s for this part.
Attachment #8381531 - Flags: review?(nalexander) → review+
Comment on attachment 8384017 [details] [diff] [review]
Part 4: Test native crypto.

Review of attachment 8384017 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately, this needs JUnit 3 -> RC love.  Otherwise looks fine.  I'd want to see a green try build on ARM v6 and ARM v7 before landing, though.

::: mobile/android/base/tests/testNativeCrypto.java
@@ +18,5 @@
> +import android.os.SystemClock;
> +
> +/**
> + * Tests the Java wrapper over native implementations of crypto code. Test vectors from:
> + *   * SHA-256:

PBKDF2SHA256.

@@ +24,5 @@
> +       - <https://gitorious.org/scrypt/nettle-scrypt/blobs/37c0d5288e991604fe33dba2f1724986a8dddf56/testsuite/pbkdf2-test.c>
> +     * SHA-1:
> +       - <http://oauth.googlecode.com/svn/code/c/liboauth/src/sha1.c>
> + */
> +public class testNativeCrypto extends UITest {

Why is this a |UITest|?

@@ +39,5 @@
> +    // minidumps directory may not be created before the process is
> +    // taken down, causing bug 722166. But we can't run the test and then block
> +    // for Gecko:Ready, since it may have arrived before we block. So we wait.
> +    // Again, JUnit 3 can't land soon enough!
> +    GeckoHelper.blockForReady();

Sigh.

@@ +117,5 @@
> +      final byte[] input = inputs[i].getBytes("US-ASCII");
> +      final String expected = expecteds[i];
> +
> +      final byte[] actual = NativeCrypto.sha1(input);
> +      assertNotNull("Hashed value is non-null", actual);

So this is JUnit 3 syntax, right?  But we're in a Robocop test.  This probably doesn't work well on infra.

@@ +124,5 @@
> +  }
> +
> +  /**
> +   * Test to ensure the output of our SHA1 algo is the same as MessageDigest's. This is important
> +   * because we intend to replace MessageDigest in FHR with this SHA-1 algo (bug 959652).

Excellent test.

@@ +153,5 @@
> +    assertNotNull("Hash result is non-null", key);
> +
> +    final long end = SystemClock.elapsedRealtime();
> +
> +    System.err.println("SHA-256 " + c + " took " + (end - start) + "ms");

|Assert.info| this.  No sense printing somewhere we won't see it.

@@ +158,5 @@
> +    if (expectedStr == null) {
> +      return;
> +    }
> +
> +    assertEquals("Hash result is the appropriate length", dkLen,

Ditto JUnit 3 (throughout).
Attachment #8384017 - Flags: review?(nalexander) → feedback+
Blocks: 978944
(In reply to Nick Alexander :nalexander from comment #71)
> Unfortunately, this needs JUnit 3 -> RC love.  Otherwise looks fine.  I'd
> want to see a green try build on ARM v6 and ARM v7 before landing, though.

https://tbpl.mozilla.org/?tree=Try&rev=e79ee8d13855

> Why is this a |UITest|?

UITest is essentially BaseTest v2 so I was just upgrading from the previous patch (until a standalone junit 3 comes along).

> @@ +117,5 @@
> > +      final byte[] input = inputs[i].getBytes("US-ASCII");
> > +      final String expected = expecteds[i];
> > +
> > +      final byte[] actual = NativeCrypto.sha1(input);
> > +      assertNotNull("Hashed value is non-null", actual);
> 
> So this is JUnit 3 syntax, right?  But we're in a Robocop test.  This
> probably doesn't work well on infra.

UITest uses the JUnit 3 API. However, the AssertionHelper import apparently does not override the built-in JUnit assertions, so I filed bug 976775 to change the UITest assertions by renaming them to `fAssert*`. Either this will land and I'll fix it there, or I will land that and fix it here.
Comment on attachment 8384696 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

Review of attachment 8384696 [details] [diff] [review]:
-----------------------------------------------------------------

Seems sane, but i'd like someone more familiar with JNI to take a look, like Kats.

::: mozglue/android/pbkdf2_sha256.c
@@ +51,5 @@
> +	p[3] = x & 0xff;
> +	p[2] = (x >> 8) & 0xff;
> +	p[1] = (x >> 16) & 0xff;
> +	p[0] = (x >> 24) & 0xff;
> +}

The rename and this change should both be in part 1.

That being said, this should be using mfbt/Endian.h. Yes, this means converting to C++. Followup?

::: mozglue/android/pbkdf2_sha256.h
@@ +64,5 @@
>      uint64_t, uint8_t *, size_t);
>  
> +#ifdef __cplusplus
> +}
> +#endif

likewise
Attachment #8384696 - Flags: review?(mh+mozilla)
Attachment #8384696 - Flags: review?(bugmail.mozilla)
Attachment #8384696 - Flags: review+
Blocks: 979078
Comment on attachment 8384696 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

Review of attachment 8384696 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the JNI stuff with comments addressed

::: mozglue/android/NativeCrypto.cpp
@@ +18,5 @@
> + * arguments.
> + */
> +extern "C" JNIEXPORT jbyteArray JNICALL Java_org_mozilla_gecko_background_nativecode_NativeCrypto_pbkdf2SHA256
> +    (JNIEnv *env, jclass jc, jbyteArray jpassword, jbyteArray jsalt, jint c, jint dkLen) {
> +  jbyte *password = env->GetByteArrayElements(jpassword, 0);

nit: s/0/NULL/ for clarity, since it's a pointer argument

@@ +21,5 @@
> +    (JNIEnv *env, jclass jc, jbyteArray jpassword, jbyteArray jsalt, jint c, jint dkLen) {
> +  jbyte *password = env->GetByteArrayElements(jpassword, 0);
> +  size_t passwordLen = env->GetArrayLength(jpassword);
> +
> +  jbyte *salt = env->GetByteArrayElements(jsalt, 0);

Ditto

@@ +24,5 @@
> +
> +  jbyte *salt = env->GetByteArrayElements(jsalt, 0);
> +  size_t saltLen = env->GetArrayLength(jsalt);
> +
> +  uint8_t hashResult[dkLen];

Guard against dkLen being negative, becase http://stackoverflow.com/questions/3783282/declaring-an-array-of-negative-length

@@ +47,5 @@
> + * Helper function to invoke native SHA-1 function with JNI arguments.
> + */
> +extern "C" JNIEXPORT jbyteArray JNICALL Java_org_mozilla_gecko_background_nativecode_NativeCrypto_sha1
> +    (JNIEnv *env, jclass jc, jbyteArray jstr) {
> +  jbyte *str = env->GetByteArrayElements(jstr, 0);

Ditto on NULL
Attachment #8384696 - Flags: review?(bugmail.mozilla) → review+
Blocks: 980463
Created attachment 8386960 [details] [diff] [review]
Part 1: Import sha files.

(In reply to Mike Hommey [:glandium] from comment #73)
> The rename and this change should both be in part 1.

Done.

> That being said, this should be using mfbt/Endian.h. Yes, this means
> converting to C++. Followup?

bug 980463.
Attachment #8384691 - Attachment is obsolete: true
Created attachment 8386969 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

Rebase with nits. I'll add a test in part 4 for the added exception handling.
Attachment #8384696 - Attachment is obsolete: true
Comment on attachment 8386969 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

Review of attachment 8386969 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/android/NativeCrypto.cpp
@@ +20,5 @@
> +extern "C" JNIEXPORT jbyteArray JNICALL Java_org_mozilla_gecko_background_nativecode_NativeCrypto_pbkdf2SHA256
> +    (JNIEnv *env, jclass jc, jbyteArray jpassword, jbyteArray jsalt, jint c, jint dkLen) {
> +  if (dkLen < 0) {
> +    env->ThrowNew(env->FindClass("java/lang/IllegalArgumentException"),
> +                  "dkLen should not be less than 0");

You should probably return NULL right after doing this.
Created attachment 8387006 [details] [diff] [review]
Part 2: Build native crypto into mozglue.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #77)
> You should probably return NULL right after doing this.

:)
Attachment #8386969 - Attachment is obsolete: true
Created attachment 8387009 [details] [diff] [review]
Part 3: (import from github) Add NativeCrypto Java interface.

Rebase.
Attachment #8387009 - Attachment description: Part 3: Add NativeCrypto Java interface. → Part 3: (import from github) Add NativeCrypto Java interface.
Created attachment 8387017 [details] [diff] [review]
Part 4: Test native crypto.

Rebase; add invalid arg test.
Attachment #8384017 - Attachment is obsolete: true
Comment on attachment 8386960 [details] [diff] [review]
Part 1: Import sha files.

Moving r+'s.
Attachment #8386960 - Flags: review+
Attachment #8387006 - Flags: review+
Attachment #8387009 - Flags: review+
Attachment #8387017 - Flags: review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/91d31fa12043
remote:   https://hg.mozilla.org/integration/fx-team/rev/55fff001e796
remote:   https://hg.mozilla.org/integration/fx-team/rev/5baa22c057c0
remote:   https://hg.mozilla.org/integration/fx-team/rev/4f2c6a223cb4

Will land on a-s once the build passes and I can steal libmozglue.so.
s/NULL/nullptr/
https://hg.mozilla.org/mozilla-central/rev/91d31fa12043
https://hg.mozilla.org/mozilla-central/rev/55fff001e796
https://hg.mozilla.org/mozilla-central/rev/5baa22c057c0
https://hg.mozilla.org/mozilla-central/rev/4f2c6a223cb4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(In reply to Mike Hommey [:glandium] from comment #83)
> s/NULL/nullptr/

Is this for the return statements, in addition to the array allocation parameters?
See bug 784739. We don't use NULL is non third party code anymore.
Created attachment 8388700 [details] [diff] [review]
Followup: Replace NULL with nullptr.

Tests passed locally.
Attachment #8388700 - Flags: review?(mh+mozilla)
Created attachment 8388706 [details] [diff] [review]
Followup: NULL -> nullptr

Tests passed locally.
Attachment #8388706 - Flags: review?(mh+mozilla)
Created attachment 8388716 [details] [diff] [review]
Followup: Replace NULL with nullptr.

Tests passed locally.
Attachment #8388716 - Flags: review?(mh+mozilla)
Attachment #8388700 - Attachment is obsolete: true
Attachment #8388700 - Flags: review?(mh+mozilla)
Attachment #8388706 - Attachment is obsolete: true
Attachment #8388706 - Flags: review?(mh+mozilla)
Sorry for spam, but I don't think bzexport handled bug 981714 well.
This landed new tests on a-s that needed to be landed on m-c:

https://hg.mozilla.org/integration/fx-team/rev/ef0be52057a8
https://hg.mozilla.org/mozilla-central/rev/ef0be52057a8
(Reporter)

Comment 93

4 years ago
Is there a good reason not to uplift this, given that it's well-tested, and would allow the carefully guarded code in Bug 978944 to dramatically improve the user experience for the first release of FxA Sync?
Whiteboard: [qa-][parallel] → [qa-]
Target Milestone: --- → Firefox 30
(In reply to Richard Newman [:rnewman] from comment #93)
> Is there a good reason not to uplift this, given that it's well-tested,

Just to be explicit: you claim this is well-tested because of

1) manual Nightly testing; and
2) the Robocop tests.

In any case, I agree that we should uplift this if it applies cleanly to Aurora.
(Reporter)

Comment 95

4 years ago
(In reply to Nick Alexander :nalexander from comment #94)

> Just to be explicit: you claim this is well-tested because of
> 
> 1) manual Nightly testing; and
> 2) the Robocop tests.

Correct. We should get visible bustage if this implementation diverges from the old one.
(In reply to Richard Newman [:rnewman] from comment #93)
> Is there a good reason not to uplift this

Not that I can tell.
Comment on attachment 8386960 [details] [diff] [review]
Part 1: Import sha files.

Uplift flag applies to parts 1 - 4.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: 
  Standalone - nothing. However, this will hopefully be uplifted with bug 978944, which utilizes this library in Firefox Accounts, where, if denied, users will have a less performant (in CPU cycles and memory bloat) SHA-256 implementation. I'm not familiar with the nitty gritty details.

Testing completed (on m-c, etc.): 
  Nightly since 3/12, unit tests

Risk to taking this patch (and alternatives if risky):
  Low - the code is library-like and currently unused (see user impact for reasons for uplift).

String or IDL/UUID changes made by this patch: None
Attachment #8386960 - Flags: approval-mozilla-aurora?
status-firefox29: --- → affected
status-firefox30: --- → fixed
Attachment #8386960 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/7356e75b4691
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/7b8cc6627638
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/d2caf2d560db
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/4b1d124a4f3c
No longer blocks: 979078
Depends on: 979078
status-firefox29: affected → fixed
Flags: needinfo?(sarentz)
Comment on attachment 8388716 [details] [diff] [review]
Followup: Replace NULL with nullptr.

Review of attachment 8388716 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I didn't come to this quick enough. That's an unfortunate consequence of me using http://harthur.github.io/bugzilla-todos/ for so long, and it not showing review requests on closed bugs.

This may need a refresh, and to be filed as a separate bug.
Attachment #8388716 - Flags: review?(mh+mozilla) → review+
(Reporter)

Comment 100

3 years ago
Michael, wanna land the last bit? :D
Flags: needinfo?(dchan) → needinfo?(michael.l.comella)
https://hg.mozilla.org/integration/fx-team/rev/b18a55eb6d3c
Nope.
Flags: needinfo?(michael.l.comella)
https://hg.mozilla.org/mozilla-central/rev/b18a55eb6d3c
You need to log in before you can comment on or make changes to this bug.