Closed Bug 915312 Opened 11 years ago Closed 10 years ago

Ship minimal PBKDF2-SHA256 native library for Android

Categories

(Android Background Services Graveyard :: Crypto, defect, P2)

All
Android
defect

Tracking

(firefox29 fixed, firefox30 fixed, fennec30+)

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
fennec 30+ ---

People

(Reporter: rnewman, Assigned: mcomella)

References

Details

(Whiteboard: [qa-])

Attachments

(6 files, 25 obsolete files)

57 bytes, text/x-github-pull-request
nalexander
: review+
Details | 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
See also Bug 912275.

This starts off on my plate (ripping code out of OpenSSL), and will probably move over to Nick to land.
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-]
Blocks: 905433
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
Attached patch 915312.1.patch (obsolete) — Splinter Review
Straight copy of just sha256.{c,h}.
Attachment #8355045 - Flags: review?(rnewman)
Attached patch 915312.2.patch (obsolete) — Splinter Review
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)
Attached file Part 3 on github. (obsolete) —
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?
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.
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>)
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+
Attachment #8355045 - Flags: review?(rnewman) → review+
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.
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.
No longer blocks: 905433
tracking-fennec: --- → 30+
Priority: -- → P2
Blocks: 799726, 967996
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
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)
Attached patch Part 3: Rename sha_fast to sha1. (obsolete) — Splinter Review
Renamed to sha1 from sha_fast.
Attachment #8375720 - Flags: review?(nalexander)
Attached patch Part 4: Add sha1 deps directly. (obsolete) — Splinter Review
Add the sha1.c/h deps directly from <m-c>/nspr/pr/include/.
Attachment #8375722 - Flags: review?(nalexander)
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.
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
If you're going to use SHA-1, there's already an implementation in mfbt (so, in mozglue).
Whiteboard: [qa-] → [qa-][parallel]
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)
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)
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
rnewman for compile changes, glandium for build. Similar to nalexander's part 2.
Attachment #8381791 - Flags: review?(mh+mozilla)
Attached patch Part 4: Test native crypto. (obsolete) — Splinter Review
Tests for sha256 are from nalexander's patch. I added SHA-1 tests.
Attachment #8381798 - Flags: review?(rnewman)
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+
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+
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+
(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)
Attached patch Part 4: Test native crypto. (obsolete) — Splinter Review
(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.
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.
(In reply to Mike Hommey [:glandium] from comment #49)
> hash?

PBKDF2 is a key derivation algorithm. That counts as crypto in my book.
(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
Michael: could you please churn all of these patches to put the sha* files in hash/, and everything else in crypto/?
Though it's worth noting: "SHA-2 is a set of *crypto*graphic hash functions…".
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)
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+
Attached patch Part 4: Test native crypto. (obsolete) — Splinter Review
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-
Made the changes recommended in comment 63.
Attachment #8383881 - Flags: review?(mh+mozilla)
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)
Attached patch Part 4: Test native crypto. (obsolete) — Splinter Review
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.
Correction: Moved sha256.* to mozglue/android/ (from m/a/hash/).
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)
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+
(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+
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+
(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.
Rebase with nits. I'll add a test in part 4 for the added exception handling.
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #77)
> You should probably return NULL right after doing this.

:)
Attachment #8387009 - Attachment description: Part 3: Add NativeCrypto Java interface. → Part 3: (import from github) Add NativeCrypto Java interface.
Comment on attachment 8386960 [details] [diff] [review]
Part 1: Import sha files.

Moving r+'s.
Attachment #8386960 - Flags: review+
s/NULL/nullptr/
(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.
Tests passed locally.
Attachment #8388700 - Flags: review?(mh+mozilla)
Attached patch Followup: NULL -> nullptr (obsolete) — Splinter Review
Tests passed locally.
Attachment #8388706 - Flags: review?(mh+mozilla)
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
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.
(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?
Attachment #8386960 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No longer blocks: 979078
Depends on: 979078
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+
Michael, wanna land the last bit? :D
Flags: needinfo?(dchan) → needinfo?(michael.l.comella)
Nope.
Flags: needinfo?(michael.l.comella)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: