Closed Bug 31818 Opened 24 years ago Closed 24 years ago

bad referring URI for javascript:

Categories

(Core :: Networking, defect, P3)

Product:
Core
Component:
Networking
Platform:
x86
Windows NT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: warrensomebody, Assigned: security-bugs)

References

Details

(Whiteboard: [dogfood+][nsbeta2-])

Attachments

(2 files)

Patch for partial solution created by Norris
18.84 KB, patch
Details | Diff | Splinter Review
Complete Patch - Please Review
15.08 KB, patch
Details | Diff | Splinter Review 
I just noticed the following code in the javascript: protocol handler:

    // Get principal
    nsCOMPtr<nsIPrincipal> principal;
    nsCOMPtr<nsIURI> referringUri;
    if (originalURI) {
      referringUri = originalURI;
    } else {
      nsCOMPtr<nsIDocShell> docShell;
      docShell = do_QueryInterface(globalOwner);
      if (!docShell)
        return NS_ERROR_FAILURE;
      if (NS_FAILED(docShell->GetCurrentURI(getter_AddRefs(referringUri))))
        return NS_ERROR_FAILURE;
    }

The "original URI" is absolutely not the referring URI. An original URI is when 
you start with resource:foo and resolve it to file:.../foo (the resource: is 
the original, the file: is the current URI). The referrer is a distinctly 
http-specific thing, so you need to QI to nsIHTTPChannel and use its interface 
to get a referrer.
Well, in this case the originalURI is the referrer. This code from 
nsWebShell.cpp sets it up this way:

      rv = pNetService->NewChannelFromURI(aCommand, aUri, loadGroup, requestor,
                                          aType, referrer /* referring uri */, 
                                          0, 0,
                                          getter_AddRefs(pChannel));

At any rate, we need some way to pass along the URI that is responsible for 
loading the channel (for http: and other protocols). This is the way nisheeth 
set it up. I suppose we could use the nsIChannel's owner attribute for this 
instead.

Thoughts?
I just remembered that I also filed bug 29831 about that issue (that the 
webshell does it wrong too).
Depends on: 29831
Warren,

Can I add a parameter for the owner to NewChannelFromURI? Setting the owner 
after the channel is created is too late for javascript: and data: URIs, which 
need the owner in order to be able to run scripts under the appropriate 
principals.
Status: NEW → ASSIGNED
Target Milestone: M15
Subject: 
            Re: original/referring URI issue
       Date: 
            Fri, 17 Mar 2000 12:13:06 -0800
      From: 
            Warren Harris <warren@netscape.com>
        To: 
            Norris Boyd <norris@netscape.com>
 References: 
            1




Norris, 

I'm in the process of doing major surgury on nsIChannel -- making many of the 
initialization parameters become
attributes. In the process of doing this, I'm fixing the js protocol to not 
evaluate until AsyncRead/OpenInputStream is
called (this is the way it used to be some time ago, and it was changed to fix a 
bug, but we have a better fix now). I'll let
you know when I get this in -- hopefully tonight. 

Warren 

Norris Boyd wrote: 

  This is from bugs 31818 and 28387. 

  Can I add a parameter for the owner to NewChannelFromURI? Setting the owner 
  after the channel is created is too late for javascript: and data: URIs, which 
  need the owner in order to be able to run scripts under the appropriate 
  principals. Since the owner is already an attribute of the channel, it seems 
like a reasonable 
  extension. 

  Then I can remove the instances where the current URI is confused with the 
referrer. 

  Thanks, 
  Norris




Norris: The javascript: protocol evaluation now occurs when 
AsyncRead/OpenInputStream is called, so you should be able to construct the 
channel, and then SetOwner before the evaluation occurs.
Turns out this blocks my fix for 34769 which requires me to stop setting the
referrer as the original uri on the channel in nsWebShell.

I broke one of the smoketests I think because the JS protocol handler expects to
call GetOriginalURI on the passed in channel and it wants to get the referrer.
Now that I'm not setting it, this breaks and bad things happen.

Looks like we need to set the owner on the channel on the JS side in this case?
Blocks: 34769
Norris is out this week (till Monday 4/10).  If this is blocking, and is needed 
for an M15 stopper, then we need some external assistance (mscott??).  If this 
is not blocking an M15 stopper, this needs to be pushed to M16 (so that we can 
branch for M15 checkpoint).
A fix for this (along with bugs 33803 and 28387) is forthcoming, but it isn't 
ready yet. Marking as M16.
Target Milestone: M15 → M16
Bulk reassigning most of norris's bugs to mstoltz.
Assignee: norris → mstoltz
Status: ASSIGNED → NEW
Blocks: 28387
Problems with the trust boundary for javscript: URLs. Marking M17.
Status: NEW → ASSIGNED
Target Milestone: M16 → M17
promoting because of dependency from 39089.
Blocks: 39089
Keywords: dogfood, nsbeta2
Severity: normal → critical
mstoltz, what's the plan for M17, and can I help?

/be
Brendan,
   Yes...thanks for the offer. I have a patch created by Norris (I'll post it 
here) which apparently fixes this problem for most cases, but it's not complete. 
Could you take a look at this patch and see what else is required?

Putting on [nsbeta2+][6/01] radar.  This work must be done by 06/01 or we may 
pull this for PR2. Please work with brendan to get in ASAP.  Thanks!
Whiteboard: [nsbeta2+][6/01]
Assessment: Providing backward compatibility with DOM0 JS 1.1 code on the web is 
a critical goal for the browser to be a viable product. javascript: URLs 
are fairly widely used even by beginning JS programmers and are common in 
the JS 1.1 code that's predominant on the web. Until we are executing the 
JavaScript code in javascript: URLs on legacy web pages, we won't even be 
detecting the other backward compatibility bugs we must have. We must fix this 
for nsbeta2 if we are to have any hope of finding the other b.c. bugs that will 
be exposed by executing the code in <A HREF="javascript: ..."> URLs in time to 
fix them for FCS.

Clearing [nsbeta2+][6/01] to trigger re-evaluation. Recommend [nsbeta2+] 
stopper.
Whiteboard: [nsbeta2+][6/01]
Blocks: 38537
Blocks: 34667
Blocks: 36745
Putting on [nsbeta2+][dogfood-] radar.  Does not need a fix ASAP for daily work, 
but we should fix this for beta2.
Whiteboard: [nsbeta2+][dogfood-]
I haven't had the opportunity to work on this, so Norris's patch is the most 
recent solution I have. Thanks for looking at this.

I haven't had the opportunity to work on this, so Norris's patch is the most 

recent solution I have. Thanks for looking at this.


*** Bug 38537 has been marked as a duplicate of this bug. ***
*** Bug 34667 has been marked as a duplicate of this bug. ***
*** Bug 36745 has been marked as a duplicate of this bug. ***
*** Bug 40396 has been marked as a duplicate of this bug. ***
Blocks: 26041
*** Bug 41395 has been marked as a duplicate of this bug. ***
Clearing [dogfood-] for reevaluation. This bug prevents me from searching
talkback, and that's dogfood for me.
Whiteboard: [nsbeta2+][dogfood-] → [nsbeta2+]
Putting on [dogfood+] radar.
Whiteboard: [nsbeta2+] → [dogfood+][nsbeta2-]
ETA is 6/9 (tomorrow).
Whiteboard: [dogfood+][nsbeta2-] → [dogfood+][nsbeta2-] ETA: 6/9
*** Bug 41919 has been marked as a duplicate of this bug. ***
*** Bug 42002 has been marked as a duplicate of this bug. ***
Fix in hand; will check in on Monday.
Whiteboard: [dogfood+][nsbeta2-] ETA: 6/9 → [dogfood+][nsbeta2-] Fix in hand.
Mitch, can you attach the complete fix?  Thanks,

/be
Yeah, I'd like to see this before you commit it.
*** Bug 33940 has been marked as a duplicate of this bug. ***
+ 
+ NS_IMETHODIMP nsDocShell::GetCurrentDocumentOwner(nsISupports** aOwner)
+ {
+     nsresult rv;
+     *aOwner = nsnull;
+     nsCOMPtr<nsIDocumentViewer> docv(do_QueryInterface(mContentViewer));
+     if (!docv) return NS_ERROR_FAILURE;
+     nsIDocument* doc;
+     rv = docv->GetDocument(doc);

Why not

    nsCOMPtr<nsIDocument> doc;
    rv = docv->GetDocument(getter_AddRefs(doc));

instead of manually ref-counting doc?

+     if (NS_FAILED(rv) || !doc) return NS_ERROR_FAILURE;
+     nsCOMPtr<nsIPrincipal> principal;
+     rv = doc->GetPrincipal(getter_AddRefs(principal));
+     NS_IF_RELEASE(doc);

I'd use an nsCOMPtr for doc, but if you don't, then this should be

    NS_RELEASE(doc);

because you've already tested for !doc and returned error above.

+       else
+       {
+           nsCOMPtr<nsIStreamIOChannel> ioChannel(do_QueryInterface(channel));
+           if(ioChannel) // Might be a javascript: URL load, need to set owner
+           {
+               static const char jsSchemeName[] = "javascript";
+               char* scheme;
+               aURI->GetScheme(&scheme);
+               if (PL_strcasecmp(scheme, jsSchemeName) == 0)
+                   channel->SetOwner(aOwner);
+           }
+           else
+           { // Also set owner for data: URLs
+               nsCOMPtr<nsIDataChannel> 
dataChannel(do_QueryInterface(channel));
+               if (dataChannel)
+                   channel->SetOwner(aOwner);
+           }
+       }

Ick -- why not always set owner here, whether or not the scheme is "javascript"?

Can we not unify cases here and just always call channel->SetOwner?  What goes 
wrong with other channel impls if we do (I don't see any error propagation here, 
so it must be unwanted side effects)?

! 
!     /*
!       The owner of the load, that is, the entity responsible for 
!       causing the load to occur. This should be a nsIPrincipal typically.
!     */
!     attribute nsISupports owner;
! };

Not your file, I know -- and you're no doubt imitating the prevailing comment 
style (which is The Right Thing, aka When In Rome), but I thought I'd plunk for 
/** ... */ doc comments here, in case you're up for reforming that file, or 
bugging its owner to use javadoc comments.  Thanks, every bit helps.

With another few rounds of review, and if this tests out well, I think it should 
go in M16.  Cc'ing leaf.

/be
what's the eta? this looks like something i'd want applied to a clean pull of
the branch and tested thoroughly before checking in, if at all. I want to
release this week.
Brendan,
   In response to your comments in the bug: 

>Why not
>
>    nsCOMPtr<nsIDocument> doc;
>    rv = docv->GetDocument(getter_AddRefs(doc));
> instead of manually ref-counting doc?

nsIDocumentViewer::GetDocument() takes a nsIDocument*& parameter, not
nsIDocument**. nsCOMPtr 
didn't seem to work here. Is there a way to pass a getter_AddRefs(nsCOMPtr<...>)
as a nsIDocument*& ?

As for the ugly protocol-specific code which strcmps for "javascript," here's my
dilemma: The vast majority of 
protocols should not inherit their owner from the loading page, they should
initially derive an owner from their own 
URL using GetCodebasePrincipal. This currently happens in
nsDocument::GetPrincipal() for cases where a 
document does not get an owner from the channel that loaded it. If I were to set
owner for all channels in 
DocShell, every page would inherit its owner from its referring page (in the
case of link clicks anyway) and this is 
incorrect; only javascript: and data: URLs should do this right now. The other
way to accomplish this that I could 
think of was to add a function to nsIChannel, something like
"SetReferringOwner()" which would set the owner 
for javascript: and data: and do nothing for all other channel types. I wanted
to avoid changing nsIChannel since 
this interface is used all over and I'm trying to "tread lightly" on the code at
this stage of the game. I agree that 
protocol-specific code in DocShell is ugly, but it vastly reduces the number of
files which need to be changed, and 
anyway we're already doing that sort of thing here in DocShell with http URLs.
Can you think of an alternate 
solution?
Yes, there is a way to use |nsCOMPtr| in this case.  Apply the |*| operator, like 
so

  nsCOMPtr<nsIDocument> doc;
  nsresult rv = docv->GetDocument(*getter_AddRefs(doc));

See the examples at
  <http://www.mozilla.org/projects/xpcom/nsCOMPtr.html#ref_getter_AddRefs>

The first code box there shows some declarations.  |GetFoo2| illustrates your 
situation.  The side-by-side boxes below show raw-pointer versus |nsCOMPtr| 
clients of those interfaces.

I was thinking that there would be a default SetOwner impl that did nothing, and 
specialized overriding methods in javascript: and data: channel impls that did 
set mOwner.  Then the default codebase-principals computation would use mOwner 
if set, otherwise it would use the document's URL.  But this is no doubt too 
stupidly simple to work.  What am I missing?

/be
Subject: 
             Re: Bug 31818
       Date: 
             Mon, 12 Jun 2000 17:48:51 -0700
       From: 
             Brendan Eich <brendan@meer.net>
         To: 
             Mitchell Stoltz <mstoltz@netscape.com>
         CC: 
             vidur@netscape.com, warren@netscape.com
 References: 
             1 , 2 , 3




   I will take Vidur's suggestion and use 
   rv = docv->GetDocument(*getter_AddRefs(doc));

Sorry I forgot about this, it's obvious in hindsight (if & works at all, it must 
work for *p such that &(*p) is p, but as a reference
[never null]). 

       Vidur and I considered the way you suggested, that is, making 
   setOwner() a no-op for every other type of channel. There are two 
   problems with this: the chrome protocol handler uses setOwner() to 
   assign the system principal to chrome. This happens below the call to 
   NS_NewURI() in docshell, which means that any subsequent setOwner() 
   calls overwrite the system principal for chrome, and the whole world 
   ends.

Well, shoot.  That means we're overloading the ambiguous term "owner".  I suspect 
you're right, and we now need yet another
channel method, say SetOriginatingURL or (your suggestion, but is it really 
specific to referrers?) SetReferringOwner. 

   That's the only legitimate existing use of setOwner(), but since 
   the chrome protocol creates various different kinds of channels, the 
   assignment of the system principal can't be pushed any lower. So I think 
   that way is out.

Another crazy idea.  At most one SetOwner per channel call is respected, after 
which the first URI set as the owner "sticks", and
subsequent sets fail.  This allows chrome to "preset" a trump card, even there is 
to be a later SetOwner call just in case a javascript: or
data: link was clicked, or src=javascript: attribute set.  But for content (as 
opposed to chrome) channels, the first and sticky set is the
one from the docshell. 

   The alternative, as I mentioned, is adding another 
   function to nsIChannel. Do you think I should do this now, or stick to 
   the ugly protocol-specific code in docshell?

As I wrote, we should do the "right thing" in due course, not to fix this bug for 
m16, or even m17.  For the short run, we should do
the expedient thing.  Your special cases for javascript: and data: are fine in 
that timeframe. 

One other problem: There's no javascript: channel. the JS protocol creates a 
StreamIOChannel; that's why I had to use a strcmp in docshell. Is it worth 
creating an nsJSChannel merely to override SetOwner()?

Let's do the expedient thing (special case GetScheme strcasecmp) for now, but I
would say for the long run, either implement nsIChannel for javascript: URLs, or
enhance StreamIOChannel so its setOwner and getOwner call into some
sub-interface that nsJSProtocolHandler.cpp can implement cheaply (i.e., without
having to implement all of the channel methods).

/be
*** Bug 37702 has been marked as a duplicate of this bug. ***
mstoltz, didn't you just check in the fix for this?
Fix is in (on the tip), marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Whiteboard: [dogfood+][nsbeta2-] Fix in hand. → [dogfood+][nsbeta2-]
concerns that this may not be completely fixed bug 44022
Discussed with mstoltz.  He pointed out bug 30915 as a passing test case for the 
work done here.  There are problems with javascript urls from the location bar 
but they should be filed in separate bugs.

verified: WinNT 2000070508
Status: RESOLVED → VERIFIED
*** Bug 33940 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: