737451 - enhance updateFromStream in crypto hashing to support 64-bit sized input

Status: NEW

(Core :: Security, defect)

Comment 1

[Makoto Kato [:m_kato]]

Attachment: fix

testing on try server

Comment 2

[Kai Engert [:KaiE:]]

Comment on attachment 607579 [details] [diff] [review]
fix

I propose Honza reviews this code first, as he wrote it initially.

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 607579 [details] [diff] [review]
fix

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

According bug 215450 comment 87, followup means something to create and land after the main thing.  I don't think this change is good to do before.  This obsoletes the security patch in that bug anyway.  Lot of things in this patch are not necessary for bug 215450 so I don't see much reason to land it first when the claim the API offers cannot be satisfied anyway.

::: browser/app/profile/extensions/[e-mail]/modules/jar-code-store.js
@@ +108,5 @@
>                     .createInstance(Ci.nsICryptoHash);
>      // Use SHA256, it's more secure than MD5:
>      ch.init(ch.SHA256);
>      // this tells updateFromStream to read the entire file
> +    ch.updateFromStream(istream, istream.available());

Why not to pass 0xffffffffffffffff ?  PR_Available64 that istram.available() calls may be expensive.

Why did -1 (in other places of calls) didn't work?

::: security/manager/ssl/src/nsStreamCipher.cpp
@@ +152,5 @@
> +    aLen = PR_UINT32_MAX;
> +
> +  // NS_ConsumeStream doesn't allow over 4GB length
> +  if (aLen > PR_UINT32_MAX)
> +    return NS_ERROR_FILE_TOO_BIG;

r- for this:

Sorry, but this appears to me strange.

You are changing the API to allow 64 bit length, but actually don't support it in the implementation.

Also NS_ConsumeStream won't accept 64 bits for maxCount in your xpcom patch from bug 215450.

::: toolkit/mozapps/extensions/PluginProvider.jsm
@@ +69,5 @@
>    hasher.init(Ci.nsICryptoHash.MD5);
>    let stringStream = Cc["@mozilla.org/io/string-input-stream;1"].
>                       createInstance(Ci.nsIStringInputStream);
>                       stringStream.data = aStr ? aStr : "null";
> +  hasher.updateFromStream(stringStream, stringStream.available());

As above.

::: toolkit/mozapps/plugins/content/pluginInstallerService.js
@@ +65,5 @@
>  
>      var hasher = Components.classes['@mozilla.org/security/hash;1'].
>        createInstance(Components.interfaces.nsICryptoHash);
>      hasher.initWithString(method);
> +    hasher.updateFromStream(fis, fis.available());

As above.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +2579,5 @@
>        var hashFunction = Ci.nsICryptoHash[this._patch.hashFunction.toUpperCase()];
>        if (hashFunction == undefined)
>          throw Cr.NS_ERROR_UNEXPECTED;
>        hash.init(hashFunction);
> +      hash.updateFromStream(fileStream, fileStream.available());

As above.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 607579 [details] [diff] [review]
fix

Double r-:

https://bugzilla.mozilla.org/show_bug.cgi?id=215450#c107