nsDOMStorage::CanAccessSystem is broken

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
RESOLVED FIXED
10 years ago
2 months ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.2a1
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

if (NS_SUCCEEDED(ssm->IsSystemPrincipal(aPrincipal, &isSystem) && isSystem))

probably wants to be:

if (NS_SUCCEEDED(ssm->IsSystemPrincipal(aPrincipal, &isSystem)) && isSystem)

This means that nsDOMStorage::CanAccessSystem always returns true, which seems bad.

msvc warned about this:
c:/moz/mozilla-central/dom/src/storage/nsDOMStorage.cpp(1340) : warning C4806: '&' : unsafe operation: no value of type 'bool' promoted to type 'unsigned int' can equal the given constant
Flags: blocking1.9.1?
Yeah, indeed.  Please fix!
Gavin, please reassign to bz (who can also reassign if he wants) if you don't have time to fix this. Can't accept unowned blockers at this point.
Assignee: nobody → gavin.sharp
Flags: blocking1.9.1? → blocking1.9.1+
Posted patch patchSplinter Review
I also changed the behavior in the unlikely failed-to-get-ssm case to default to false rather than true.

I don't know how to test this offhand, but perhaps we shouldn't block on that at this point.
Attachment #379311 - Flags: superreview?(bzbarsky)
Attachment #379311 - Flags: review?(bzbarsky)
Attachment #379311 - Flags: superreview?(bzbarsky)
Attachment #379311 - Flags: superreview+
Attachment #379311 - Flags: review?(bzbarsky)
Attachment #379311 - Flags: review+
Comment on attachment 379311 [details] [diff] [review]
patch

Looks good; I'd check with dcamp and/or mayhemer on testing this.
Whiteboard: [needs landing m-c]
This affects only globalStorage.
To create a test we probably need to obtain the object of globalStorage["examle.com"] in e.g. an iframe and pass it to a parent that is from a different origin, e.g. "http://localhost:8888". This parent than should try to access an item of that storage and we should check it succeeds w/o the patch and fails w/ the patch.
For considering this is a test to check we are not simple able to access storage objects from a different origins. It is protected on level of DOM and NOT in the specific storage code.
There's a typo in one of those tests: sessoinStorage.

Was it intentional that those tests avoid using globalStorage, given comment 5? I'm really not familiar with the various different storage objects...
(In reply to comment #8)
> There's a typo in one of those tests: sessoinStorage.
> 

Thanks.

> Was it intentional that those tests avoid using globalStorage, given comment 5?
> I'm really not familiar with the various different storage objects...

I wanted to check that new and speced features are ok. globalStorage is obsolete at the moment. Here is a new test including globalStorage. I cannot check the DOM storage C code, I cannot access the window property on level of DOM. Maybe there is another way to access properties of a different-origin content?
Attachment #379356 - Attachment is obsolete: true
Not sure if this is relevant, but netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect") will let you access properties in other origins in mochitest, but it will not directly let you control the principal of the calling code, so I'm not sure it'll help here.
Pushed this to try just to be sure, all green apart from some known-sporadic failures (bug 487750, bug 494113, bug 492575, bug 494420).
Still would love a testcase here, but I've landed the patch:
http://hg.mozilla.org/mozilla-central/rev/ed23428195e2
https://hg.mozilla.org/releases/mozilla-1.9.1/rev/c0f12f88f0e0
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Keywords: fixed1.9.1
Resolution: --- → FIXED
Whiteboard: [needs landing m-c]
Target Milestone: --- → mozilla1.9.2a1
How will check-ins of tests for blocking1.9.1 bugs be handled? Will they get checked in after the tree is open again?
Group: core-security
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.