Closed Bug 614116 (CVE-2011-0052) Opened 11 years ago Closed 11 years ago

Insecure sites may modify existing secure items in globalStorage when in PB or SO-cookies mode


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
blocking2.0 --- betaN+
status1.9.2 --- .14-fixed
status1.9.1 --- .17-fixed


(Reporter: mayhemer, Assigned: ehsan.akhgari)



(Keywords: privacy, regression, Whiteboard: [sg:low])


(1 file, 2 obsolete files)

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.
blocking2.0: --- → ?
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
blocking2.0: ? → betaN+
Whiteboard: [sg:moderate]
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?
Assignee: nobody → ehsan
Attached patch Patch (v1) (obsolete) — Splinter Review
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+
Attached patch For check-in (obsolete) — Splinter Review
Moved the test, but otherwise left it as it was.
Attachment #493605 - Attachment is obsolete: true
Closed: 11 years ago
Flags: in-testsuite+
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.
Resolution: FIXED → ---
Attached patch Patch (v2)Splinter Review
Honza, can you please review the change?
Attachment #493730 - Attachment is obsolete: true
Attachment #493792 - Flags: review?(honzab.moz)
Comment on attachment 493792 [details] [diff] [review]
Patch (v2)

Attachment #493792 - Flags: review?(honzab.moz) → review+
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
Attachment #493792 - Flags: approval1.9.2.14?
Attachment #493792 - Flags: approval1.9.1.17?
Blocks: 487695
blocking1.9.1: ? → ---
blocking1.9.2: ? → ---
Comment on attachment 493792 [details] [diff] [review]
Patch (v2)

Approved for and, a=dveditz for release-drivers
Attachment #493792 - Flags: approval1.9.2.14?
Attachment #493792 - Flags: approval1.9.2.14+
Attachment #493792 - Flags: approval1.9.1.17?
Attachment #493792 - Flags: approval1.9.1.17+
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.
Keywords: privacy
Whiteboard: [sg:moderate] → [sg:low]
Alias: CVE-2011-0052
Group: core-security
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.