Last Comment Bug 316931 - Allow chrome:// xforms to submit anywhere
: Allow chrome:// xforms to submit anywhere
Status: RESOLVED FIXED
: fixed1.8.0.2, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Doron Rosenberg (IBM)
: Stephen Pride
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-11-17 20:21 PST by alexander :surkov
Modified: 2016-07-15 14:46 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (856 bytes, patch)
2005-11-17 20:50 PST, alexander :surkov
doronr: review+
aaronr: review+
Details | Diff | Splinter Review
use nsIPrincipal (15.29 KB, patch)
2005-11-28 13:08 PST, Doron Rosenberg (IBM)
allan: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description alexander :surkov 2005-11-17 20:21:37 PST
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 2005-11-17 20:50:02 PST
Created attachment 203490 [details] [diff] [review]
patch
Comment 2 Doron Rosenberg (IBM) 2005-11-18 09:44:36 PST
Comment on attachment 203490 [details] [diff] [review]
patch

Looks ok.
Comment 3 Allan Beaufour 2005-11-24 06:25:16 PST
Checked into trunk.
Comment 4 Boris Zbarsky [:bz] (TPAC) 2005-11-26 10:21:30 PST
Er... WHY are we doing a protocol check, not a real security check?
Comment 5 Doron Rosenberg (IBM) 2005-11-27 08:09:01 PST
(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 [:bz] (TPAC) 2005-11-27 12:25:18 PST
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) 2005-11-27 16:08:04 PST
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 [:bz] (TPAC) 2005-11-27 16:13:29 PST
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 2005-11-28 02:06:12 PST
(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) 2005-11-28 06:46:04 PST
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) 2005-11-28 09:13:04 PST
(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 [:bz] (TPAC) 2005-11-28 12:18:56 PST
> 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) 2005-11-28 12:31:54 PST
(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 [:bz] (TPAC) 2005-11-28 12:43:58 PST
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) 2005-11-28 13:08:56 PST
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.
Comment 16 Doron Rosenberg (IBM) 2005-11-29 09:19:33 PST
Comment on attachment 204376 [details] [diff] [review]
use nsIPrincipal

bz, do you have time to sr?
Comment 17 Boris Zbarsky [:bz] (TPAC) 2005-11-29 09:31:52 PST
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) 2005-11-29 09:35:23 PST
Comment on attachment 204376 [details] [diff] [review]
use nsIPrincipal

allan, care to review the XForms changes?
Comment 19 Allan Beaufour 2006-01-11 01:40:56 PST
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) 2006-01-11 09:05:18 PST
checked in the nsIPrincipal patch.
Comment 21 aaronr 2006-02-02 17:26:31 PST
checked into MOZILLA_1_8_BRANCH via bug 323691.  Leaving open for now until it gets into 1.8.0

Note You need to log in before you can comment on or make changes to this bug.