316931 - Allow chrome:// xforms to submit anywhere

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

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

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Doron Rosenberg (IBM)]

Comment on attachment 203490 [details] [diff] [review]
patch

Looks ok.

Comment 3

[Allan Beaufour]

Checked into trunk.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Er... WHY are we doing a protocol check, not a real security check?

Comment 5

[Doron Rosenberg (IBM)]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Doron Rosenberg (IBM)]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

[Allan Beaufour]

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

Comment 10

[Doron Rosenberg (IBM)]

Yeah, I usually ask darin to review such changes, made a mistake by not doing this  this time.  Going to take a look.

Comment 11

[Doron Rosenberg (IBM)]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Doron Rosenberg (IBM)]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

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?

Comment 15

[Doron Rosenberg (IBM)]

Attachment: 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.

Comment 16

[Doron Rosenberg (IBM)]

Comment on attachment 204376 [details] [diff] [review]
use nsIPrincipal

bz, do you have time to sr?

Comment 17

[Boris Zbarsky [: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.

Comment 18

[Doron Rosenberg (IBM)]

Comment on attachment 204376 [details] [diff] [review]
use nsIPrincipal

allan, care to review the XForms changes?

Comment 19

[Allan Beaufour]

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

Comment 20

[Doron Rosenberg (IBM)]

checked in the nsIPrincipal patch.

Comment 21

[aaronr]

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