Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
942123004:
Use sha hashes of extension ids to whitelist. (Closed)
DescriptionUse sha hashes of extension ids to whitelist.
Currently if an app needs to whitelist with feedback, it needs to put its
original ID into the event_handler JS. We want apps to be able to keep their
ids private and still be able to whitelist themselves. Hence we now check
the sha-1 hash of the app id instead of the app id itself.
R=arv@chromium.org
BUG=453587
Committed: https://crrev.com/cca7ccbc8cb9b8a3d59c35bff112632e4edab95d
Cr-Commit-Position: refs/heads/master@{#317679}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Messages
Total messages: 15 (4 generated)
Any chance this can be tested? https://codereview.chromium.org/942123004/diff/1/chrome/browser/resources/fee... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/942123004/diff/1/chrome/browser/resources/fee... chrome/browser/resources/feedback/js/event_handler.js:63: var buffer = new ArrayBuffer(str.length); Use TextEncoder instead. https://codereview.chromium.org/942123004/diff/1/chrome/browser/resources/fee... chrome/browser/resources/feedback/js/event_handler.js:77: * @param {function} startFeedbackCallback The callback function that will Function https://codereview.chromium.org/942123004/diff/1/chrome/browser/resources/fee... chrome/browser/resources/feedback/js/event_handler.js:86: var hashView = new Uint8Array(hashBuffer); and TextDecoder here.
Currently testing this manually. I really want to add some tests for this code, but it would have to be post branch. https://codereview.chromium.org/942123004/diff/1/chrome/browser/resources/fee... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/942123004/diff/1/chrome/browser/resources/fee... chrome/browser/resources/feedback/js/event_handler.js:63: var buffer = new ArrayBuffer(str.length); On 2015/02/20 22:05:07, arv wrote: > Use TextEncoder instead. Done. https://codereview.chromium.org/942123004/diff/1/chrome/browser/resources/fee... chrome/browser/resources/feedback/js/event_handler.js:77: * @param {function} startFeedbackCallback The callback function that will On 2015/02/20 22:05:07, arv wrote: > Function Done. https://codereview.chromium.org/942123004/diff/1/chrome/browser/resources/fee... chrome/browser/resources/feedback/js/event_handler.js:86: var hashView = new Uint8Array(hashBuffer); On 2015/02/20 22:05:07, arv wrote: > and TextDecoder here. I am not converting this back to a string. This needs to be converted into a hash string (with padding). Is that possible with TextDecoder?
A few more ideas https://codereview.chromium.org/942123004/diff/20001/chrome/browser/resources... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/942123004/diff/20001/chrome/browser/resources... chrome/browser/resources/feedback/js/event_handler.js:68: crypto.subtle.digest('SHA-256', (new TextEncoder).encode(id)).then( new TextEncoder().encode(id) https://codereview.chromium.org/942123004/diff/20001/chrome/browser/resources... chrome/browser/resources/feedback/js/event_handler.js:74: hex = '0'.substr(0, 2 - hex.length) + hex; No need to muck with substr here. var n = hashView[i]; hashString += n < 0x10 ? '0' : n.toString(16);
mednik@chromium.org changed reviewers: + mednik@chromium.org
https://codereview.chromium.org/942123004/diff/20001/chrome/browser/resources... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/942123004/diff/20001/chrome/browser/resources... chrome/browser/resources/feedback/js/event_handler.js:22: '0eeefaa87e292cd986dd0528e90f9128a936b1c4e1c53e7faa0244594c69df94', // QuickOffice This format looks different from that used to whitelist hashes of extensions for APIs: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... It would be really nice if these were the same so app developers who get whitelisted in that file can just dump the same hashes in here.
https://codereview.chromium.org/942123004/diff/20001/chrome/browser/resources... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/942123004/diff/20001/chrome/browser/resources... chrome/browser/resources/feedback/js/event_handler.js:22: '0eeefaa87e292cd986dd0528e90f9128a936b1c4e1c53e7faa0244594c69df94', // QuickOffice On 2015/02/20 23:48:03, mednik wrote: > This format looks different from that used to whitelist hashes of extensions for > APIs: > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... > > It would be really nice if these were the same so app developers who get > whitelisted in that file can just dump the same hashes in here. The current extensions system uses SHA1. I decided to use SHA-256 due to Google eventually phasing out SHA1 due to insecurity. I spoke with the extension team about this and it seems that we get enough security with SHA1. If someone really wants to spend the time to break SHA1 on these IDs, we don't really care that much. So the decision is to use SHA1. Done. https://codereview.chromium.org/942123004/diff/20001/chrome/browser/resources... chrome/browser/resources/feedback/js/event_handler.js:68: crypto.subtle.digest('SHA-256', (new TextEncoder).encode(id)).then( On 2015/02/20 23:28:20, arv wrote: > new TextEncoder().encode(id) Throws an exception when I remove the parenthesis. "Error in event handler for runtime.onMessageExternal: TypeError: undefined is not a function at senderWhitelisted (chrome-extension://gfdkimpbcpahaombhbimeihdjnejgicl/js/event_handler.js:69:32)" https://codereview.chromium.org/942123004/diff/20001/chrome/browser/resources... chrome/browser/resources/feedback/js/event_handler.js:74: hex = '0'.substr(0, 2 - hex.length) + hex; On 2015/02/20 23:28:20, arv wrote: > No need to muck with substr here. > > var n = hashView[i]; > hashString += n < 0x10 ? '0' : n.toString(16); Done (with the fixed version).
LGTM https://codereview.chromium.org/942123004/diff/40001/chrome/browser/resources... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/942123004/diff/40001/chrome/browser/resources... chrome/browser/resources/feedback/js/event_handler.js:69: crypto.subtle.digest('SHA-1', (new TextEncoder).encode(id)).then( crypto.subtle.digest('SHA-1', new TextEncoder().encode(id)).then( Should work.
https://codereview.chromium.org/942123004/diff/40001/chrome/browser/resources... File chrome/browser/resources/feedback/js/event_handler.js (right): https://codereview.chromium.org/942123004/diff/40001/chrome/browser/resources... chrome/browser/resources/feedback/js/event_handler.js:69: crypto.subtle.digest('SHA-1', (new TextEncoder).encode(id)).then( On 2015/02/23 20:25:11, arv wrote: > crypto.subtle.digest('SHA-1', new TextEncoder().encode(id)).then( > > Should work. Hmm, not sure what I was doing wrong before, but it does work now. Done.
The CQ bit was checked by rkc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org Link to the patchset: https://codereview.chromium.org/942123004/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/942123004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/cca7ccbc8cb9b8a3d59c35bff112632e4edab95d Cr-Commit-Position: refs/heads/master@{#317679}
Message was sent while issue was closed.
Description was changed from ========== Use sha hashes of extension ids to whitelist. Currently if an app needs to whitelist with feedback, it needs to put its original ID into the event_handler JS. We want apps to be able to keep their ids private and still be able to whitelist themselves. Hence we now check the sha-256 hash of the app id instead of the app id itself. R=arv@chromium.org BUG=453587 Committed: https://crrev.com/cca7ccbc8cb9b8a3d59c35bff112632e4edab95d Cr-Commit-Position: refs/heads/master@{#317679} ========== to ========== Use sha hashes of extension ids to whitelist. Currently if an app needs to whitelist with feedback, it needs to put its original ID into the event_handler JS. We want apps to be able to keep their ids private and still be able to whitelist themselves. Hence we now check the sha-1 hash of the app id instead of the app id itself. R=arv@chromium.org BUG=453587 Committed: https://crrev.com/cca7ccbc8cb9b8a3d59c35bff112632e4edab95d Cr-Commit-Position: refs/heads/master@{#317679} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
