Bug 614116 (CVE-2011-0052)

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

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
DOM
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: mayhemer, Assigned: Ehsan)

Tracking

({privacy, regression})

Trunk
mozilla2.0b8
privacy, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+, status1.9.2 .14-fixed, status1.9.1 .17-fixed)

Details

(Whiteboard: [sg:low])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
blocking2.0: --- → ?
(Reporter)

Comment 1

8 years ago
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

Updated

8 years ago
blocking2.0: ? → betaN+
Whiteboard: [sg:moderate]
(Assignee)

Comment 2

8 years ago
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)

Updated

8 years ago
Assignee: nobody → ehsan
(Assignee)

Comment 3

8 years ago
Created attachment 493605 [details] [diff] [review]
Patch (v1)
Attachment #493605 - Flags: review?(honzab.moz)
(Assignee)

Updated

8 years ago
Whiteboard: [sg:moderate] → [sg:moderate][has patch][needs review mayhemer]
(Reporter)

Comment 4

8 years ago
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+
(Assignee)

Comment 5

8 years ago
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
(Assignee)

Comment 6

8 years ago
http://hg.mozilla.org/mozilla-central/rev/1f53f85ddfb5
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [sg:moderate][has patch][needs review mayhemer] → [sg:moderate]
Target Milestone: --- → mozilla2.0b8
(Assignee)

Comment 7

8 years ago
Do we also need this for branches?
(Reporter)

Comment 8

8 years ago
(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.
(Assignee)

Comment 9

8 years ago
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 → ---
(Assignee)

Comment 10

8 years ago
Created attachment 493792 [details] [diff] [review]
Patch (v2)

Honza, can you please review the change?
Attachment #493730 - Attachment is obsolete: true
Attachment #493792 - Flags: review?(honzab.moz)
(Reporter)

Comment 11

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

r=honzab.
Attachment #493792 - Flags: review?(honzab.moz) → review+
(Assignee)

Updated

8 years ago
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
(Assignee)

Updated

8 years ago
Attachment #493792 - Flags: approval1.9.2.14?
Attachment #493792 - Flags: approval1.9.1.17?
Blocks: 487695
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 1.9.2.14 and 1.9.1.17, 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
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.