Allow chrome:// xforms to submit anywhere

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
9 months ago

People

(Reporter: surkov, Assigned: Doron Rosenberg (IBM))

Tracking

({fixed1.8.0.2, fixed1.8.1})

Trunk
fixed1.8.0.2, fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051116 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051116 Firefox/1.6a1

The bug like bug 296231. If you set up application then you trust it almost extremely. There is a question what is "the instance replacing" as it is mentioned in a patch for bug 296231?

Reproducible: Always
(Reporter)

Comment 1

12 years ago
Created attachment 203490 [details] [diff] [review]
patch
Attachment #203490 - Flags: review?(doronr)
(Assignee)

Comment 2

12 years ago
Comment on attachment 203490 [details] [diff] [review]
patch

Looks ok.
Attachment #203490 - Flags: review?(doronr) → review+

Updated

12 years ago
Assignee: aaronr → surkov
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Updated

12 years ago
Attachment #203490 - Flags: review?(aaronr)

Updated

12 years ago
Attachment #203490 - Flags: review?(aaronr) → review+

Comment 3

12 years ago
Checked into trunk.
Whiteboard: xf-to-branch
Er... WHY are we doing a protocol check, not a real security check?
(Assignee)

Comment 5

12 years ago
(In reply to comment #4)
> Er... WHY are we doing a protocol check, not a real security check?
> 

Don't you need a JSContext for real security checks to work?
No.  As long as you have the nsIPrincipal involved (which you had better!), you can just call IsCapabilityEnabled() on it with whatever capability it is you have here.  You'll have to pass null for the annotation, and at the moment that will make things equivalent to checking for the system principal, but that's what you want here, I assume.  I certainly _hope_ that non-system chrome (eg themes) is not meant to get extra privileges here.
(Assignee)

Comment 7

12 years ago
I guess we'll have to figure how to get a nsIPrincipal.

cc: some more people.  Perhaps we can use nsIScriptSecurityManager to get at it, not sure.
nsIDocument::GetPrincipal() should do what you want, I suspect.

Also, imo, nsXFormsUtils::CheckSameOrigin should take an nsIPrincipal for the first arg and call checkSameOriginPrincipal, not checkSameOriginURI.

In general any time you're working with URIs directly for security purposes you have a bug.

Comment 9

12 years ago
(In reply to comment #8)
> nsIDocument::GetPrincipal() should do what you want, I suspect.
> 
> Also, imo, nsXFormsUtils::CheckSameOrigin should take an nsIPrincipal for the
> first arg and call checkSameOriginPrincipal, not checkSameOriginURI.
> 
> In general any time you're working with URIs directly for security purposes you
> have a bug.
> 

Ouch. I was feeling a bit uneasy about this... but apparently it did not stop me from checking it in. We have some security issues then...
(Assignee)

Comment 10

12 years ago
Yeah, I usually ask darin to review such changes, made a mistake by not doing this  this time.  Going to take a look.
(Assignee)

Comment 11

12 years ago
(In reply to comment #8)
> nsIDocument::GetPrincipal() should do what you want, I suspect.
> 
> Also, imo, nsXFormsUtils::CheckSameOrigin should take an nsIPrincipal for the
> first arg and call checkSameOriginPrincipal, not checkSameOriginURI.
> 
> In general any time you're working with URIs directly for security purposes you
> have a bug.
> 

Shouldn't I use checkLoadURIWithPrincipal and pass in the principal of the document and the URI I want to load?  Wouldn't that allow "true" chrome to access anything?  Which would mean I can skip the IsCapabilityEnabled() check and just use nsXFormsUtils::CheckSameOrigin?
> Shouldn't I use checkLoadURIWithPrincipal and pass in the principal of the
> document and the URI I want to load?

Depends on the model you want.  checkLoadURIWithPrincipal allows any http: page to load any other http: page, which is not what you want here, last I checked.
(Assignee)

Comment 13

12 years ago
(In reply to comment #12)
> > Shouldn't I use checkLoadURIWithPrincipal and pass in the principal of the
> > document and the URI I want to load?
> 
> Depends on the model you want.  checkLoadURIWithPrincipal allows any http: page
> to load any other http: page, which is not what you want here, last I checked.
> 

I got it mostly working, what is the correct way to allow file:// to load anything?
Um....  There's no such way via the security manager that I know of.  If we need one (and I suspect we do), file a bug?
(Assignee)

Comment 15

12 years ago
Created attachment 204376 [details] [diff] [review]
use nsIPrincipal

This seems to work.

I haven't tried chrome://, can you try with this Alexander?  I am checking for the "UniversalBrowserRead" capability, which should work.
(Assignee)

Comment 16

12 years ago
Comment on attachment 204376 [details] [diff] [review]
use nsIPrincipal

bz, do you have time to sr?
Attachment #204376 - Flags: superreview?(bzbarsky)
Comment on attachment 204376 [details] [diff] [review]
use nsIPrincipal

>Index: extensions/xforms/nsXFormsSubmissionElement.cpp
>+nsXFormsSubmissionElement::CheckSameOrigin(nsIDocument 
>+      baseURI->SchemeIs("file", &allowSubmission);

For what it's worth, this should be checking the rv -- any extension can add new protocols, and you can't trust them to not throw on SchemeIs.

>Index: extensions/xforms/nsXFormsUtils.cpp
>+    // check the security manager and do a same original check on the principal

s/original/origin/

>+        rv = secMan->CheckSameOriginPrincipal(aBaseDocument->GetPrincipal(),

Just pass in basePrincipal?

sr=bzbarsky with those fixed.
Attachment #204376 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 18

12 years ago
Comment on attachment 204376 [details] [diff] [review]
use nsIPrincipal

allan, care to review the XForms changes?
Attachment #204376 - Flags: review?(allan)
(Assignee)

Updated

12 years ago
Assignee: surkov → doronr

Comment 19

11 years ago
Comment on attachment 204376 [details] [diff] [review]
use nsIPrincipal

(In reply to comment #18)
> (From update of attachment 204376 [details] [diff] [review] [edit])
> allan, care to review the XForms changes?

Yes, sorry that it took so long.

r=me
Attachment #204376 - Flags: review?(allan) → review+
(Assignee)

Comment 20

11 years ago
checked in the nsIPrincipal patch.
Status: NEW → ASSIGNED

Comment 21

11 years ago
checked into MOZILLA_1_8_BRANCH via bug 323691.  Leaving open for now until it gets into 1.8.0

Updated

11 years ago
Whiteboard: xf-to-branch

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.0.2
Resolution: --- → FIXED

Updated

11 years ago
Keywords: fixed1.8.1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.