421224 - Give content policies an official way to change the URI

Status: NEW

(Core :: Security, defect)

Description

[Alex Vincent [:WeirdAl]]

A quick look at nsContentPolicy.cpp shows this master policy assumes child policies will not change the URI arguments, in particular contentLocation.  If policy # 4 changes the location, policies 1, 2, and 3 are not called to check the location again.  According to bz, it would also bypass CheckLoadURI, which doesn't sound so good either.

I see two different ways to fix this, and bz has a third:
(1) Check the URI after each policy to see if it's changed, and if it has, reset.
(2) Forcibly ensure the URI passed to each policy is the same as the one initially passed in, possibly by cloning the URI and passing in the clone each time.
(3) Make sure all URI's are immutable.

I'm not sure which one is the correct answer, if any.  In the second and third cases, we would prevent content policies from doing their own redirects (which is somewhat sane - a policy might not be the right way to do that).

I'm willing to implement a fix, if someone will offer guidance.  My initial thought was to go with plan 1, using GetSpec(), but bz says that's expensive.

Comment 1

[Boris Zbarsky [:bzbarsky]]

We should make URIs immutable.  We should also have a way for people to redirect loads, either as part of content policy, or as a separate previous pass.

I don't think the fact that privileged code can bypass content policies and checkloadURI is something to worry about.

Comment 2

[Alex Vincent [:WeirdAl]]

Clarification:  GetSpec() isn't expensive, but neither is it right.  Instead, that route would mean cloning the URI's via the Clone() method, and then calling Equals()... which in nsStandardURL.cpp *is* expensive.  :(

Factor in the number of times nsContentPolicy::ShouldLoad() is called thanks to a content load, times the number of policies shipped by default (I counted five), and this can get ugly fast.

Comment 3

[Wladimir Palant]

Some URIs passed to content policies are already immutable (in particular nsImageLoadingContent and nsObjectLoadingContent.cpp call NS_TryToSetImmutable on the URIs they create). However, we probably don't want the callers to take care of that - rather have nsContentPolicyUtils create an immutable copy of the original URI. An option would adding the following line to CHECK_CONTENT_POLICY macro:

nsComPtr<nsIURI> contentLocationImmutable = NS_TryToMakeImmutable(contentLocation);

And passing contentLocationImmutable to content policies then.

I am more interested in an "official" way to redirect requests however. How could the interface for that look like - an out param in ShouldLoad? nsContentPolicy would need to restart checking if this out param (or whatever) is set, how can endless loops be avoided here?

Comment 4

[Alex Vincent [:WeirdAl]]

Well, I was wrong, in the case that mattered to me.

I was trying to change a chrome DTD URL to a virtual:// protocol one, and I hit the ENSURE_MUTABLE abort hard.  So much for that hack! :)

If I submitted a patch with tests, is there any reasonable way that this would make 1.9 at this late stage in the game?

Comment 5

[Alex Vincent [:WeirdAl]]

mfinkle had a truly great idea for this:  change "in nsIURI aContentLocation" to "inout nsIURI aContentLocation".  I like it, bz likes it, I'll spin out a patch this weekend to see if we can get it in for 1.9.

Smaug: this should be a relatively trivial patch to review, do you think you'll have time?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I haven't seen the "trivial" patch yet ;)

Comment 7

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

The good news is this patch appears to work - it includes a xpcshell testcase which passes.  jst-review simulacrum seems relatively happy, too.

The bad news is xpcshell crashes on shutdown.  :-)  I don't know why; gdb isn't forthcoming with a useful stack, but it's crashing on this line:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1208551728 (LWP 6993)]
0x00bd1ce4 in XPCJSRuntime::GCCallback (cx=0x82250a0, status=JSGC_END) at /home/ajvincent/trunk/mozilla/js/src/xpconnect/src/xpcjsruntime.cpp:816
816                         NS_RELEASE(obj);


Despite that, I think I can ask for a preliminary review to see if I'm making any obvious, stupid, or no-way-in-the-world-is-this-getting-in-the-tree mistakes.  In fact, I'd appreciate any help that could come up from reviewers and people who can debug this to see where the crash is coming from.

The fact that it works until shutdown and the NS_RELEASE in xpcshell point to ref counting getting messed up somewhere.  I did add a bit to nsCOMPtr.h at the top of this patch, but I don't see how that affects XPConnect crashing.  I also note we're entering some relatively new territory.  There hasn't been much use of inout nsIAnything in our source tree - mostly inout PRInt32 or some other primitive type.  It could be we've hit some heretofore untested piece of code in XPConnect.

This was run on a trunk build from source a couple days old.

Comment 8

[timeless]

Comment on attachment 309717 [details] [diff] [review]
patch, v1

>Index: browser/base/content/browser.js
>+            var uriValue = { value: uri };
>             if (contentPolicy.shouldLoad(Ci.nsIContentPolicy.TYPE_IMAGE,
>+                                         uriValue, targetDoc.documentURIObject,

>Index: xpfe/global/resources/content/bindings/tabbrowser.xml
>+                var uriValue = { value: uri };
>                 if (contentPolicy.shouldLoad(nsIContentPolicy.TYPE_IMAGE,
>                                              uri, origURI, event.target,

you didn't change this

> nsContentBlocker::ShouldLoad(PRUint32          aContentType,
>+                             nsIURI           **aContentLocationValue,

>+  // We don't change the content location.
>+  nsIURI* aContentLocation = *aContentLocationValue;

>   if (!aContentLocation)
>     return NS_OK;

I need to check the rules for inouts, but i don't think this is right....

>Index: suite/browser/tabbrowser.xml
>+              var uriWrapper = { value: uri };
>               if (contentPolicy.shouldLoad(nsIContentPolicy.TYPE_IMAGE,
>+                                           uriWrapper, origURI, event.target,
>+                // A content policy may have changed the URI.
>+                href = uriValue.value.spec;

uriValue != uriWrapper.

>Index: content/base/src/nsDataDocumentContentPolicy.cpp
>@@ -83,11 +83,11 @@ NS_IMETHODIMP
> nsDataDocumentContentPolicy::ShouldProcess(PRUint32 aContentType,
>                                            nsIURI *aContentLocation,
> {
>-  return ShouldLoad(aContentType, aContentLocation, aRequestingLocation,
>+  return ShouldLoad(aContentType, &aContentLocation, aRequestingLocation,
>                     aRequestingContext, aMimeGuess, aExtra, aDecision);

This change seems unsafe. You're NS_RELEASE()ing a pointer [possibly a *copy*] you didn't own and stashing someone else's pointer in its place.

Comment 9

[Alex Vincent [:WeirdAl]]

(In reply to comment #8)
> > nsContentBlocker::ShouldLoad(PRUint32          aContentType,
> >+                             nsIURI           **aContentLocationValue,
> 
> >+  // We don't change the content location.
> >+  nsIURI* aContentLocation = *aContentLocationValue;
> 
> >   if (!aContentLocation)
> >     return NS_OK;
> 
> I need to check the rules for inouts, but i don't think this is right....

How is this wrong?  First, the IDL compiler changed it to nsIURI** (which is why I sadly had to junk the function pointer).  Second, I am getting the nsIURI through this, according to MSVC.

Unless you mean to imply that one shouldn't *set* the value this way, I don't quite understand your objection here.


> >Index: content/base/src/nsDataDocumentContentPolicy.cpp
> >@@ -83,11 +83,11 @@ NS_IMETHODIMP
> > nsDataDocumentContentPolicy::ShouldProcess(PRUint32 aContentType,
> >                                            nsIURI *aContentLocation,
> > {
> >-  return ShouldLoad(aContentType, aContentLocation, aRequestingLocation,
> >+  return ShouldLoad(aContentType, &aContentLocation, aRequestingLocation,
> >                     aRequestingContext, aMimeGuess, aExtra, aDecision);
> 
> This change seems unsafe. You're NS_RELEASE()ing a pointer [possibly a *copy*]
> you didn't own and stashing someone else's pointer in its place.

That might explain the crash, but I don't follow the logic.  How is this releasing a pointer?  ShouldProcess takes nsIURI*, ShouldLoad takes nsIURI**.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 309717 [details] [diff] [review]
patch, v1

>+template <class T>
>+class nsGetterInOut
Do you really need this? I'm not the right person to review xpcom/

Comment 11

[timeless]

ShouldProcess takes an unowned reference to a pointer.
ShouldLoad takes an owned reference to a pointer.

you probably need this:
+  nsCOMPtr contentLocation(aContentLocation);
+  return ShouldLoad(aContentType, getter_InOut(contentLocation), aRequestingLocation,

in your code ShouldProcess passes a reference it doesn't own to ShouldLoad which releases it (the reference it didn't own). ShouldLoad quasi-returns a new object reference to ShouldProcess which then leaks it (because the pointer it got was an in variable so no one will have a pointer to it).

I guess for nsISupports you don't have as many problems leaving variables alone as normal arrays.

Comment 12

[Alex Vincent [:WeirdAl]]

timeless: thanks for your comments.  I appreciate the explanation.

Smaug: I think I do.  Simply put, there's no good way to handle in/out for nsIFoo in XPCOM.  Since I ended up using this code in a bunch of places, I decided to create a little helper.  However, I'm pretty sure that I did it wrong, since I never touched ShouldProcess in my test, and I still crashed.

I'll post a new patch later tonight.

Comment 13

[Mike Shaver (:shaver emeritus)]

(In reply to comment #1)
> I don't think the fact that privileged code can bypass content policies and
> checkloadURI is something to worry about.

I think that they can do it unintentionally by using a hook that's ostensibly for _restriction_ of operations is something to worry about, actually.  An extension that rewrites a URL to be the "you can't load this" page packaged in their chrome is probably not expecting that they give content a way to load chrome, for example.

I think it's worth restarting the CP consultation, and would prefer that we did.

Comment 14

[Mike Shaver (:shaver emeritus)]

We also need test coverage so that we know the effects of such rewrites on
- our built-in loads of things like the blocklist, app and add-on updates, the URL classifier
- calls during startup and shutdown
- coverage of the different call sites getting rewrites, rewrite-to-same-URI, rewrite-to-bogus-URI, rewrite-to-privileged-URI, error-during-rewrite

Free drive-by:

+      if ((cLocation)) {                                                      \

doesn't need the overparenthesization of cLocation, since there's no risk of semantic change from adjacent tokens.

Comment 15

[Alex Vincent [:WeirdAl]]

Despite my efforts, this bug isn't going to make 1.9.  It was worth trying for it, though, in my opinion.

Comment 16

[Alex Vincent [:WeirdAl]]

I just noticed bug 286159 - is this a duplicate of that bug?

Comment 17

[Alex Vincent [:WeirdAl]]

re comment 14 (from Mossop)
Extensions, blocklists:  toolkit/mozapps/extensions
App/extension updates: toolkit/mozapps/updates
URL classifier: toolkit/components/url-classifier

startup/shutdown, I don't quite grok what you have in mind.  Nor for call site coverage.

I think I'll post a new patch, but I'll need help with the extra tests so I know what I'm supposed to do.

Comment 18

[Alex Vincent [:WeirdAl]]

Attachment: patch, v2 (work-in-progress)

This does not yet answer shaver's concerns in comment 14 - I'm posting this so I don't lose any work.

Comment 19

[Alex Vincent [:WeirdAl]]

I need advice, please.

CHECK_PRINCIPAL in nsContentPolicyUtils.h is a macro I left alone, and a XMLHttpRequest from xpcshell does not redirect because of it.  This was introduced in bug 388597 (bz, sicking, in particular, if you'd weigh in...).

Backing out the fix in that bug just to enable redirects seems very, very wrong.  Bug 388597 comment 0 cites 20% time spent in content policy checks.

Would it be all right to say that if CHECK_PRINCIPAL passes, no content policy redirects will be allowed?

Comment 22

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.