Regression from bug 501423. Before an non-secure page modifies an item in globalStorage, we first have to check the item is not present or is not secure (not stored by an https page). This is simple to fix by adding a check to nsDOMStorage::SetItem. SetDBValue just updates w/o any checks, expecting those are made by callers. Love to have a test for this as well.
This is valid only in session-only cookies or PB mode. The check is actually missing in nsDOMStorageMemoryDB::SetKey. nsDOMStorage::SetDBValue is not responsible for secure flag checking. nsDOMStoragePersistentDB::SetKey (called from SetDBValue) does it for persistent storages. This can potentially be used to detect PB or SO mode. Not sure if this necessarily needs to be a security bug any more.
Summary: Insecure sites may modify existing secure items in globalStorage → Insecure sites may modify existing secure items in globalStorage when in PB or SO-cookies mode
If I read comment 1 correctly, our only concern is that the previously stored item in the memory DB is not from a secure site when nsDOMStorageMemoryDB::SetKey is called for a non-secure site, right?
Created attachment 493605 [details] [diff] [review] Patch (v1)
Attachment #493605 - Flags: review?(honzab.moz)
Whiteboard: [sg:moderate] → [sg:moderate][has patch][needs review mayhemer]
Comment on attachment 493605 [details] [diff] [review] Patch (v1) That's exactly what we need, thanks. Few details: - please move the test to dom/tests/mochitest/globalstorage (there will be soon this directory) - if you are willing to simplify the test a bit: there is a framework to write inter origin tests, see test_localStorageOriginsSchemaDiffs.html + frameMasterNotEqual.html + frameSlaveNotEqual.html, the test is using interOriginTest2.js and frames are using interOriginFrame.js ; it makes the tests much more readable and easier to review r=honzab with the first change and optionally the second one as well
Attachment #493605 - Flags: review?(honzab.moz) → review+
Created attachment 493730 [details] [diff] [review] For check-in Moved the test, but otherwise left it as it was.
Attachment #493605 - Attachment is obsolete: true
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [sg:moderate][has patch][needs review mayhemer] → [sg:moderate]
Target Milestone: --- → mozilla2.0b8
Do we also need this for branches?
(In reply to comment #7) > Do we also need this for branches? We do, this bug is present since 1.9.1. Introduced in bug 487695.
This broke a XUL file test in editor/. It turns out that calling pm.removeAll will remove the permission for allowXULXBL, which makes us not able to load the XUL file for that test. I have a patch to fix that.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 493792 [details] [diff] [review] Patch (v2) Honza, can you please review the change?
Comment on attachment 493792 [details] [diff] [review] Patch (v2) r=honzab.
Attachment #493792 - Flags: review?(honzab.moz) → review+
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago → 8 years ago
Resolution: --- → FIXED
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking1.9.1: ? → ---
blocking1.9.2: ? → ---
status1.9.1: --- → wanted
status1.9.2: --- → wanted
Comment on attachment 493792 [details] [diff] [review] Patch (v2) Approved for 22.214.171.124 and 126.96.36.199, a=dveditz for release-drivers
status1.9.1: wanted → .17-fixed
status1.9.2: wanted → .14-fixed
The test for this bug has been failing continuously (with a timeout) on the branches since it landed there. See bug 616219.
sg:moderate overstates the problem. You can't read secure data, only write, and only when the potential victim is in session-only/private mode. It's not hard to imagine scenarios where there's some website set up to do off-line processing and maybe has terrible things to overwrite if the MITM knew you were on that site, but in practice I can't think of any real cases.
Whiteboard: [sg:moderate] → [sg:low]
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.