Open
Bug 737451
Opened 12 years ago
Updated 2 years ago
enhance updateFromStream in crypto hashing to support 64-bit sized input
Categories
(Core :: Security, defect)
Tracking
()
NEW
People
(Reporter: m_kato, Assigned: m_kato)
References
Details
Attachments
(1 file)
15.87 KB,
patch
|
mayhemer
:
review-
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 1•12 years ago
|
||
testing on try server
Assignee | ||
Updated•12 years ago
|
Attachment #607579 -
Flags: review?(kaie)
Updated•12 years ago
|
Summary: updateFromStream makes 64-bit → enhance updateFromStream in crypto hashing to support 64-bit sized input
Comment 2•12 years ago
|
||
Comment on attachment 607579 [details] [diff] [review] fix I propose Honza reviews this code first, as he wrote it initially.
Attachment #607579 -
Flags: superreview?(kaie)
Attachment #607579 -
Flags: review?(kaie)
Attachment #607579 -
Flags: review?(honzab.moz)
Comment 3•12 years ago
|
||
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/testpilot@labs.mozilla.com/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.
Attachment #607579 -
Flags: review?(honzab.moz) → review-
Comment 4•12 years ago
|
||
Comment on attachment 607579 [details] [diff] [review] fix Double r-: https://bugzilla.mozilla.org/show_bug.cgi?id=215450#c107
Updated•12 years ago
|
Attachment #607579 -
Flags: superreview?(kaie)
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•