Closed
Bug 915312
Opened 11 years ago
Closed 11 years ago
Ship minimal PBKDF2-SHA256 native library for Android
Categories
(Android Background Services Graveyard :: Crypto, defect, P2)
Tracking
(firefox29 fixed, firefox30 fixed, fennec30+)
RESOLVED
FIXED
Firefox 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+
Sylvestre
:
approval-mozilla-aurora+
|
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.
Reporter | ||
Comment 1•11 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…
Updated•11 years ago
|
Whiteboard: [qa-]
Reporter | ||
Comment 2•11 years ago
|
||
Flags: needinfo?(nalexander)
Comment 3•11 years ago
|
||
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)
Comment 4•11 years ago
|
||
Here's a preliminary try build with some robocop tests hacked to check this:
https://tbpl.mozilla.org/?tree=Try&rev=b73a477edbd4
Comment 5•11 years ago
|
||
(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.
Comment 6•11 years ago
|
||
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
Comment 7•11 years ago
|
||
Straight copy of just sha256.{c,h}.
Attachment #8355045 -
Flags: review?(rnewman)
Comment 8•11 years ago
|
||
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)
Comment 9•11 years ago
|
||
Attachment #8355047 -
Flags: review?(rnewman)
Comment 10•11 years ago
|
||
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•11 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•11 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•11 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•11 years ago
|
Attachment #8355045 -
Flags: review?(rnewman) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #8355047 -
Flags: review?(rnewman) → review+
Comment 14•11 years ago
|
||
(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.
Comment 15•11 years ago
|
||
(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•11 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
Comment 17•11 years ago
|
||
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 18•11 years ago
|
||
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)
Comment 19•11 years ago
|
||
(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)
Comment 20•11 years ago
|
||
(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•11 years ago
|
tracking-fennec: --- → 30+
Priority: -- → P2
Reporter | ||
Updated•11 years ago
|
Assignee | ||
Comment 21•11 years ago
|
||
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
Assignee | ||
Comment 22•11 years ago
|
||
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)
Assignee | ||
Comment 23•11 years ago
|
||
Renamed to sha1 from sha_fast.
Attachment #8375720 -
Flags: review?(nalexander)
Assignee | ||
Comment 24•11 years ago
|
||
Add the sha1.c/h deps directly from <m-c>/nspr/pr/include/.
Attachment #8375722 -
Flags: review?(nalexander)
Assignee | ||
Comment 25•11 years ago
|
||
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)
Assignee | ||
Comment 26•11 years ago
|
||
(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.
Comment 27•11 years ago
|
||
* 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.
Updated•11 years ago
|
Attachment #8375720 -
Flags: review?(nalexander)
Updated•11 years ago
|
Attachment #8375719 -
Flags: review?(nalexander)
Updated•11 years ago
|
Attachment #8375722 -
Flags: review?(nalexander)
Updated•11 years ago
|
Attachment #8375724 -
Flags: review?(nalexander)
Assignee | ||
Comment 28•11 years ago
|
||
(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.
Comment 29•11 years ago
|
||
Comment 30•11 years ago
|
||
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.
Assignee | ||
Comment 31•11 years ago
|
||
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?
Assignee | ||
Comment 32•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Attachment #8375720 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8375722 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8375724 -
Attachment is obsolete: true
Comment 33•11 years ago
|
||
If you're going to use SHA-1, there's already an implementation in mfbt (so, in mozglue).
Reporter | ||
Updated•11 years ago
|
Whiteboard: [qa-] → [qa-][parallel]
Assignee | ||
Comment 34•11 years ago
|
||
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)
Assignee | ||
Comment 35•11 years ago
|
||
Assignee | ||
Comment 36•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8355045 -
Attachment is obsolete: true
Assignee | ||
Comment 37•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Attachment #8378663 -
Attachment is obsolete: true
Assignee | ||
Comment 38•11 years ago
|
||
rnewman for compile changes, glandium for build. Similar to nalexander's part 2.
Attachment #8381791 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8381791 -
Flags: review?(rnewman)
Assignee | ||
Comment 39•11 years ago
|
||
Tests for sha256 are from nalexander's patch. I added SHA-1 tests.
Attachment #8381798 -
Flags: review?(rnewman)
Assignee | ||
Updated•11 years ago
|
Attachment #8355046 -
Attachment is obsolete: true
Reporter | ||
Comment 40•11 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•11 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•11 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+
Assignee | ||
Comment 43•11 years ago
|
||
(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)
Assignee | ||
Updated•11 years ago
|
Attachment #8381791 -
Attachment is obsolete: true
Attachment #8381791 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8382539 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 44•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Attachment #8381798 -
Attachment is obsolete: true
Assignee | ||
Comment 45•11 years ago
|
||
Comment on attachment 8382554 [details] [diff] [review]
Part 4: Test native crypto.
Move rnewman r+.
Attachment #8382554 -
Flags: review+
Assignee | ||
Comment 46•11 years ago
|
||
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
Comment 47•11 years ago
|
||
I still think "crypto" is a misleading name.
Assignee | ||
Comment 48•11 years ago
|
||
Do you have any alternative recommendations?
Comment 49•11 years ago
|
||
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•11 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•11 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.
Comment 52•11 years ago
|
||
(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•11 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•11 years ago
|
||
Though it's worth noting: "SHA-2 is a set of *crypto*graphic hash functions…".
Reporter | ||
Comment 55•11 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+
Assignee | ||
Comment 56•11 years ago
|
||
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)
Assignee | ||
Comment 57•11 years ago
|
||
Rebased.
Assignee | ||
Updated•11 years ago
|
Attachment #8381537 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8383364 -
Flags: review+
Assignee | ||
Comment 58•11 years ago
|
||
Rebased.
Assignee | ||
Updated•11 years ago
|
Attachment #8382539 -
Attachment is obsolete: true
Attachment #8382539 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 59•11 years ago
|
||
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)
Assignee | ||
Comment 60•11 years ago
|
||
Comment on attachment 8383367 [details] [diff] [review]
Part 2: Build native crypto into mozglue.
Move forward rnewman r+.
Attachment #8383367 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8382554 -
Attachment is obsolete: true
Attachment #8382554 -
Flags: review?(nalexander)
Assignee | ||
Comment 62•11 years ago
|
||
Comment on attachment 8383370 [details] [diff] [review]
Part 4: Test native crypto.
Move forward rnewman r+.
Attachment #8383370 -
Flags: review+
Comment 63•11 years ago
|
||
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-
Assignee | ||
Comment 64•11 years ago
|
||
Made the changes recommended in comment 63.
Attachment #8383881 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8383367 -
Attachment is obsolete: true
Assignee | ||
Comment 65•11 years ago
|
||
Moved NativeCrypto to omg.background.nativecode from part 3, as nalexander suggested, and updated these methods accordingly.
Attachment #8384014 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8383881 -
Attachment is obsolete: true
Attachment #8383881 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8383370 -
Attachment is obsolete: true
Attachment #8383370 -
Flags: review?(nalexander)
Assignee | ||
Comment 67•11 years ago
|
||
Sorry, I messed up the patches - working on an update. Hold off on a review until I'm finished please.
Assignee | ||
Comment 68•11 years ago
|
||
Correction: Moved sha256.* to mozglue/android/ (from m/a/hash/).
Assignee | ||
Updated•11 years ago
|
Attachment #8383364 -
Attachment is obsolete: true
Assignee | ||
Comment 69•11 years ago
|
||
Correction: Rebase on correction in part 1. Should be good to review.
Attachment #8384696 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8384014 -
Attachment is obsolete: true
Attachment #8384014 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8384691 -
Flags: review+
Comment 70•11 years ago
|
||
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 71•11 years ago
|
||
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+
Assignee | ||
Comment 72•11 years ago
|
||
(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 73•11 years ago
|
||
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 74•11 years ago
|
||
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+
Assignee | ||
Comment 75•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Attachment #8384691 -
Attachment is obsolete: true
Assignee | ||
Comment 76•11 years ago
|
||
Rebase with nits. I'll add a test in part 4 for the added exception handling.
Assignee | ||
Updated•11 years ago
|
Attachment #8384696 -
Attachment is obsolete: true
Comment 77•11 years ago
|
||
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.
Assignee | ||
Comment 78•11 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #77)
> You should probably return NULL right after doing this.
:)
Assignee | ||
Updated•11 years ago
|
Attachment #8386969 -
Attachment is obsolete: true
Assignee | ||
Comment 79•11 years ago
|
||
Rebase.
Assignee | ||
Updated•11 years ago
|
Attachment #8387009 -
Attachment description: Part 3: Add NativeCrypto Java interface. → Part 3: (import from github) Add NativeCrypto Java interface.
Assignee | ||
Comment 80•11 years ago
|
||
Rebase; add invalid arg test.
Assignee | ||
Updated•11 years ago
|
Attachment #8384017 -
Attachment is obsolete: true
Assignee | ||
Comment 81•11 years ago
|
||
Comment on attachment 8386960 [details] [diff] [review]
Part 1: Import sha files.
Moving r+'s.
Attachment #8386960 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8387006 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8387009 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8387017 -
Flags: review+
Assignee | ||
Comment 82•11 years ago
|
||
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.
Comment 83•11 years ago
|
||
s/NULL/nullptr/
Comment 84•11 years ago
|
||
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
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 85•11 years ago
|
||
(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?
Comment 86•11 years ago
|
||
See bug 784739. We don't use NULL is non third party code anymore.
Assignee | ||
Comment 87•11 years ago
|
||
Tests passed locally.
Attachment #8388700 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 88•11 years ago
|
||
Tests passed locally.
Attachment #8388706 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 89•11 years ago
|
||
Tests passed locally.
Attachment #8388716 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8388700 -
Attachment is obsolete: true
Attachment #8388700 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8388706 -
Attachment is obsolete: true
Attachment #8388706 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 90•11 years ago
|
||
Sorry for spam, but I don't think bzexport handled bug 981714 well.
Comment 91•11 years ago
|
||
This landed new tests on a-s that needed to be landed on m-c:
https://hg.mozilla.org/integration/fx-team/rev/ef0be52057a8
Comment 92•11 years ago
|
||
Reporter | ||
Comment 93•11 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
Comment 94•11 years ago
|
||
(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•11 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.
Assignee | ||
Comment 96•11 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #93)
> Is there a good reason not to uplift this
Not that I can tell.
Assignee | ||
Comment 97•11 years ago
|
||
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?
Updated•11 years ago
|
status-firefox29:
--- → affected
status-firefox30:
--- → fixed
Updated•11 years ago
|
Attachment #8386960 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 98•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Updated•11 years ago
|
Updated•11 years ago
|
Flags: needinfo?(sarentz)
Comment 99•10 years ago
|
||
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•10 years ago
|
||
Michael, wanna land the last bit? :D
Flags: needinfo?(dchan) → needinfo?(michael.l.comella)
Assignee | ||
Comment 101•10 years ago
|
||
Comment 103•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•