614116 - (CVE-2011-0052) Insecure sites may modify existing secure items in globalStorage when in PB or SO-cookies mode

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

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.

Comment 1

[Honza Bambas (:mayhemer)]

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.

Comment 2

[(no longer active)]

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?

Comment 3

[(no longer active)]

Attachment: Patch (v1)

Comment 4

[Honza Bambas (: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

Comment 5

[(no longer active)]

Attachment: For check-in

Moved the test, but otherwise left it as it was.

Comment 6

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/1f53f85ddfb5

Comment 7

[(no longer active)]

Do we also need this for branches?

Comment 8

[Honza Bambas (:mayhemer)]

(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.

Comment 9

[(no longer active)]

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.

Comment 10

[(no longer active)]

Attachment: Patch (v2)

Honza, can you please review the change?

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 493792 [details] [diff] [review]
Patch (v2)

r=honzab.

Comment 12

[(no longer active)]

Relanded:

http://hg.mozilla.org/mozilla-central/rev/8855a25370b3

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 493792 [details] [diff] [review]
Patch (v2)

Approved for 1.9.2.14 and 1.9.1.17, a=dveditz for release-drivers

Comment 14

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a63fd7a33fc7
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/340d74cbb09f

Comment 15

[Steven Michaud [:smichaud] (Retired)]

The test for this bug has been failing continuously (with a timeout) on the branches since it landed there.

See bug 616219.

Comment 16

[Daniel Veditz [:dveditz]]

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.