Open Bug 344890 Opened 18 years ago Updated 2 years ago

<img src="javascript:alert(...);"> gives "Permission denied to get property Window.alert"

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Platform:
PowerPC
macOS
Type:
defect

Tracking

()

People

(Reporter: jruderman, Unassigned)

References

(Depends on 1 open bug)

Details

(Keywords: testcase)

Attachments

(2 files)

testcase
116 bytes, text/html
Details
testcase 2
625 bytes, text/html
Details 
Error: uncaught exception: Permission denied to get property Window.alert
Attached file testcase — 

    
    

    
  
The strange thing is that scripts there can do some things only indirectly, by calling page-defined functions.

    
    

    
  

      
      
      
      

        Attached file
          
        testcase 2
        — 
      

    


  
Demonstrates that a javascript: img src (at least one set by JS) can call a function that calls alert, but can't call alert directly.

JavaScript error: , line 0: uncaught exception: Permission denied to get property Window.alert

    
    

    
  
This isn't a recent regression.  Firefox 1.5.0.x shows the bug too, as do trunk builds from December.  (Mostly testing with testcase 2.)

    
    

    
  
I bet mrbkap can figure out what's going wrong here.

    
    

    
  
The script that we're running (which is trying to call alert) has the null principal, so it doesn't have access to touch window.alert. Boris, was this intentional? I was expecting us to run this code with the principals of the surrounding page.

    
    

    
  
The owner of the channel is null, so we get the object principal for the global object, then do the paranoia check at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp&rev=1.130#240 (which fails, since this is not about:blank), so we create a null principal.

This looks correct to me -- in general we don't give the page's permissions to javascript: URIs except those loaded in the docshell (which get an owner set).  Everything else gets the null principal (and has for years in Gecko; before we had null principals it got a principal based on the javascript: URI).

I feel that this is an important protection from XSS script injection.  Otherwise, for example, stylesheets could run arbitrary script on a window, which would lead to cookie-stealing attacks on sites that allow users to post stylesheets...

I think we should wontfix this unless someone has a _really_ good reason to change the behavior here.

    
    

    
  
<Jesse_> bz: so how do you explain comment 3?
<Jesse_> bz: it's different for javascript: src set by JS rather than just in the page?
<Jesse_> bz: i'm pretty sure the testcase in comment 3 demonstrates *a* bug, i guess not the one you're suggesting should be wontfix
<bz> we should fail to get the "foo" property off the global
<bz> imo...
<Jesse_> even in the foo.src="javascript:" case?
<bz> imo, yes
<Jesse_> ok
<bz> unless we want to allow general access to everything in the page from script embedded via images, stylesheets, etc
<bz> Which I don't think we want to do.
<bz> I bet our security check optimizations in classinfo are messing us up here...

    
    

    
  
I'm curious whether making that change would fix bug 344996 and bug 344874 by making it impossible for scripts in javascript: img URLs to modify the page, or if pages could somehow collude so that page scripts execute as a result of the javascript: img URL executing.

    
    

    
  
(In reply to comment #7)
> The owner of the channel is null,

Under what conditions is the owner null?

It seems to me we should always be able to propagate "taint" here.  In the classic codebase, every JS URL had its origin tracked.  A javascript: URL written verbatim into markup loaded from a server (as a href= or src= attribute value, e.g.) would have as its origin the containing page's origin.  A javascript: URL set via script would have as its origin the subject principals.

We have a bug on tracking URL origin, IIRC.  I know we aren't poised to fix it, but I wonder if we shouldn't blaze a trail for JS URLs.  I'm concerned that if we just use null principals because channel owner is null, we will fail in some valid cases that worked in the old days, and that may work in other browsers.  Perhaps it's not important to work in these case (if IE doesn't support 'em, say).  OTOH it would be good to be sure that null channel owner means "use null principals".

Any clues welcome.

/be

    
    

    
  
> Under what conditions is the owner null?

Always, except when the javascript: load is being performed in a docshell (i.e. setting an iframe src or a window's location to a javascript: URL).

> It seems to me we should always be able to propagate "taint" here.

We can try, but it'd be a whack-a-mole of finding every single place where we load them, followed by API changes to propagate the info down into image loaders, plugin loaders, etc, etc.

Further, I think it would be counterproductive, as I said in comment 7.

> A javascript: URL written verbatim into markup loaded from a server
....
> A javascript: URL set via script would have as its origin the subject
> principals.

We can't really tell those two cases apart very well.  See comment at <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsFrameLoader.cpp&rev=1.68#150> .  I just realized that some of my reasoning for that comment never made it into the comment or bug and I've added it in the bug.  See bug 327078 comment 30 and in particular note cases I, II, III that I describe and the issues with telling who the "real caller" is in them.

> OTOH it would be good to be sure that null channel owner means "use null
> principals".

I'm not sure what you mean here.

    
    

    
  
(In reply to comment #11)
> > It seems to me we should always be able to propagate "taint" here.
> 
> We can try, but it'd be a whack-a-mole of finding every single place where we
> load them, followed by API changes to propagate the info down into image
> loaders, plugin loaders, etc, etc.

Think of it as a dynamically scoped variable.  The principal who set this JS URL in a place where it might be loaded.  It should be implementable without API changes and passing any extra actual argument. XPConnect has to keep track of dynamic scope using the thread context stack.  Could use that, maybe -- the point is that we need to be sure of who is stuffing JS URLs into the DOM, or I think we will break valid use cases (they may have been broken for years in Gecko, but are they really broken in IE)?

> Further, I think it would be counterproductive, as I said in comment 7.

Injection would carry the attacker's taint, so would result in a stronger defense than null principals do.  No?

> > A javascript: URL written verbatim into markup loaded from a server
> ....
> > A javascript: URL set via script would have as its origin the subject
> > principals.
> 
> We can't really tell those two cases apart very well.  See comment at
> <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsFrameLoader.cpp&rev=1.68#150>
> .  I just realized that some of my reasoning for that comment never made it
> into the comment or bug and I've added it in the bug.  See bug 327078 comment
> 30 and in particular note cases I, II, III that I describe and the issues with
> telling who the "real caller" is in them.

I'm in favor of fixing our design so we can tell.  I've talked about doing this with you but not necessarily in 1.9.  So here, I'm mainly interested in what can be done for 1.9.  We can spin this out into a followup bug for sure, but it seemed worth some discussion before punting.

> > OTOH it would be good to be sure that null channel owner means "use null
> > principals".
> 
> I'm not sure what you mean here.

I'm suggesting that if the current architecture makes null owner ambiguous (could be evil injection, could be innocent scripting of one's own DOM), then the answer is not "use null principals" -- it's "disambiguate".

I'm motivated to help fix this, after I get back from Oslo next week.

/be

    
    

    
  
(In reply to comment #12)
> > Further, I think it would be counterproductive, as I said in comment 7.
> 
> Injection would carry the attacker's taint, so would result in a stronger
> defense than null principals do.  No?

Or at least a better diagnostic showing who was attacking.

/be


    
    

    
  
> or I think we will break valid use cases

I'm not buying that some of these use cases (e.g. javascript: backgrounds in stylesheets that modify the page DOM) are valid.

> so would result in a stronger defense than null principals do

No.  Null principals are not allowed to access any web page.  Any other principal will be able to access something.  So in fact, null principals give a stronger defense.

The fundamental problem here is that if a user uploads a stylesheet to a site, then that stylesheet should have the user's principal, not the site's.  Probably.  Unless the site says otherwise.  But we have no way to tell where this stylesheet came from.

Same issue with a site that allows a user to point to an image hosted on some other server.

> I'm in favor of fixing our design so we can tell.

In my opinion, the stylesheet problem (and the image problem with images, as mentioned above) is fundamentally insoluble on our end.  We'd need to have the server tell us where each piece of content came from where to really solve it.

> Or at least a better diagnostic showing who was attacking.

That, yes.

For what it's worth, we _do_ still have a problem that we should, imo, focus this bug on.  The javascript: URL can call foo() even though it can't access window.alert.  This means we're doing inconsistent security checks.  We need to figure out why that's happening and fix it to be at least consistent.  Then we can start worrying about what that consistent state should be.

    
    

    
  
(In reply to comment #14)
> > or I think we will break valid use cases
> 
> I'm not buying that some of these use cases (e.g. javascript: backgrounds in
> stylesheets that modify the page DOM) are valid.

That's fine, but Gecko as it stands and with your patch is not invalidating those cases only -- it's breaking scripted cases that are not attacks, cases that should work.

> > so would result in a stronger defense than null principals do
> 
> No.  Null principals are not allowed to access any web page.  Any other
> principal will be able to access something.  So in fact, null principals give a
> stronger defense.

They break the benign case where there is no attack.

> The fundamental problem here is that if a user uploads a stylesheet to a site,
> then that stylesheet should have the user's principal, not the site's. 
> Probably.  Unless the site says otherwise.  But we have no way to tell where
> this stylesheet came from.

Let's not mix up two separate cases:

1.  User-generated content, hosted and sanitized poorly, where the we might as a matter of policy say that CSS background properties that are JS URLs run with the null principals.

2.  Safe scripted assignment of a JS URL to an href or src property.

> Same issue with a site that allows a user to point to an image hosted on some
> other server.

That case can't be distinguished from safe scripting with the current architecture, which I call an ambiguity and a bug.  Again I'm not insisting on a fix right now.  But I am arguing with your claim that all cases must be treated as exploits or "bad" cases.

> > I'm in favor of fixing our design so we can tell.
> 
> In my opinion, the stylesheet problem (and the image problem with images, as
> mentioned above) is fundamentally insoluble on our end.  We'd need to have the
> server tell us where each piece of content came from where to really solve it.

No, we either trust the served content as a matter of policy -- which we do for static <img src="javascript:..."> but wrongly do not for safely scripted JS URL assignments to img.src etc. -- or we pragmatically ban it in hard cases that have bit us in the ass (badly hosted/sanitized user generated content).

I think it's important not to mix these cases up.

> For what it's worth, we _do_ still have a problem that we should, imo, focus
> this bug on.  The javascript: URL can call foo() even though it can't access
> window.alert.  This means we're doing inconsistent security checks.  We need to
> figure out why that's happening and fix it to be at least consistent.  Then we
> can start worrying about what that consistent state should be.

What I am proposing is consistent.

/be


    
    

    
  
> which we do for static <img src="javascript:..."> but wrongly do not for safely
> scripted JS URL assignments to img.src etc.

At the moment, static vs dynamic does the same thing here.  Jesse's testcases happen to set .src in script, but he'd get the same errors (or lack thereof) if he were setting the attr via the parser.

    
    

    
  
(In reply to comment #16)
> > which we do for static <img src="javascript:..."> but wrongly do not for safely
> > scripted JS URL assignments to img.src etc.
> 
> At the moment, static vs dynamic does the same thing here.  Jesse's testcases
> happen to set .src in script, but he'd get the same errors (or lack thereof) if
> he were setting the attr via the parser.

Sorry, I'm tired and jetlagged -- his testcase 2's foo2 button fails, but foo1 works for me.  Why?

/be

    
    

    
  
foo2 calls alert().

foo1 calls a function named "foo", which calls alert()

Both use setAttribute('src', 'javascript:...'), so both are dynamic.

foo2 is not allowed to access alert, because XPConnect does a security check for that access and denies it.

foo1 is allowed to access window.foo (because we're not doing a security check there for some reason), then foo() is allowed to call alert, because it has the same principal as the window, being a function defined on the window.  The fact that window.foo is accessible while window.alert is not is a bug that needs to be fixed independently of what else we do.

    
    

    
  
The setTimeout(foopy, 200) from onload is irrelevant, right?

Why does the img src having a different origin from the page containing the img matter for same-origin checking purposes?  The img element is scoped by the page's window, so in the same origin.

I see no threat in testcase 2.

/be

    
    

    
  
(In reply to comment #18)
> foo1 is allowed to access window.foo (because we're not doing a security check
> there for some reason),

Can you set a breakpoint in needsSecurityCheck? I bet that being able to access window.foo1 from the button onclick handler is caching the inner window and the given context, giving us access to window.foo from foo1 or something. Perhaps it's the onload handler accessing foopy?

    
  
Blocks: 343940

    
    

    
  
> The setTimeout(foopy, 200) from onload is irrelevant, right?

Yes.

> The img element is scoped by the page's window, so in the same origin.

We've already covered this part earlier in the bug, no?

> I see no threat in testcase 2.

The threat is that in our codebase we fundamentally cannot tell apart testcase 2 from other cases which _are_ threats.  I'm happy to try to address this issue, but as you pointed out that's not happening for 1.9.  The question is what we do about the inconsistent security check bug for 1.9, as well as for 1.8.1 and 1.8.0.6.  If it would make you happier to either spin that bug off into a new bug or move the javascript: futures discussion to a new bug, we should do that.  I frankly don't have the brainprint  to redesign our security architecture this week.

Blake, I'll try to debug this once I'm done with the things that are currently tying up all my builds, but if you get there first, go for it.  ETA for me is 2-3 days before I can look.
Blocks: 179793
Depends on: 350433

    
    

    
  
Bug 350433 filed on what I feel is the must-fix problem here.
Depends on: 355365

    
    

    
  
Seems that bug 167475 is related here

    
    

    
  
It's not relate, no.  javascript: is not an external protocol.

    
    

    
  
But it's very similar, javascript: protocol also has no sense in <ANYTAG SRC="...">  reference. Maybe, summary of bug 167475 should be edited to add internal protocols such as javascript:, irc: etc., isn't it?

    
    

    
  
> javascript: protocol also has no sense

Sorry, not true.  Simple test:

  <iframe src="javascript:'This makes sense'"></iframe>


    
    

    
  
I still guess that thus doesn't make sense. To show text in iframe we can use data protocol (RFC 2397):

<iframe src="data:This%20makes%20sense"></iframe>

BTW it works even if javascript is disabled, instead of your example.

And if we want to generate or change data in iframe dynamically, we can do it via DOM or manipulations with src attribute.

Why do we need <iframe src="javascript:..."> or more so <img src="javascript:...>? I still couldn't understand.

    
    

    
  
> To show text in iframe we can use data protocol 

Not in IE.  So if you want to do something cross-browser, you can't use it.

There aren't any reasons one _needs_ the javascript: at this point, really, if browsers implemented all the stuff that they should.  But they don't, and it's used by web sites, so we need to support it.

    
    

    
  
Manko: javascript: URLs are a de-facto web standard. They should be codified by the WHAT-WG if not the W3C. They're not going away.

/be

    
    

    
  
Now both "testcase" and "testcase 2" trigger:

WARNING: No principal to execute JS with: file /Users/jruderman/trunk/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp, line 175

and neither button in "testcase 2" triggers an alert.

    
    

    
  
That's correct.  I disabled execution of javascript: with null origin principals, since we can't even tell whether script is enabled for the origin in that case.

Bug 377092 or equivalent imagelib changes is needed to make it work for <img> again.

    
  
Depends on: 377092
Assignee: dveditz → nobody

    
  
Depends on: 434522

    
  
Severity: normal → S3

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: