Closed Bug 528950 Opened 15 years ago Closed 14 years ago

Disallow eval in chrome

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: bzbarsky, Assigned: mrbkap)

References

Details

(Whiteboard: [sg:want]) 

This seems like a gun that's too easy to misuse and we have safe options (sandboxes, window.JSON, etc) for people who really need the functionality.  I propose we make eval throw if you try to use it with the system principal.

See the "eval vs lookup" thread in m.d.platform, which is what prompted the thought.

Opinions?
If we had done this long ago, our lives would have been much easier.

We're going to have to announce this far and wide and give people time to fix their extensions, but I support this 1,000% (comma added to show that I didn't accidentally type one too many 0s).
We'd absolutely have to announce this.  But we have time before 1.9.3, right?  How about we do this now-ish and start the announcing?
blocking2.0: --- → ?
(In reply to comment #0)
> This seems like a gun that's too easy to misuse and we have safe options
> (sandboxes, window.JSON, etc) for people who really need the functionality. 

I don't know about window.JSON, but we have investigated sandbox option extensively and it's lack of documentation makes it not a viable alternative. We are unable to determine what it does.
Feel free to ask questions in the newsgroups, esp. with pointers to the unclear docs, if any.  I assume you've read https://developer.mozilla.org/En/Components.utils.evalInSandbox
And just as a note, there's strictly more documentation for evalInSandbox than there is for eval.
Umm. What if I just want to run JS input from the user in a chrome environment? ChatZilla and Venkman and, to my knowledge, the JS console, all do this (and I bet chromebug does, too). I don't see any good reason not to allow that. I understand that there may be security issues, but these usecases are devoid of these issues (unless the user themselves chooses to shoot themselves in the foot, which we're not going to be able to prevent anyway).

I guess I could workaround by writing user input to a temp file and then using the subscriptloader, but that sounds so ridiculous that I almost wouldn't have put it in this comment. :-)
In that case, we should give a specific and explicit way to do so: evalWithFullPrivileges or such.  That will at least keep it from being inadvertently used, or used as easily as part of an escalation attack.

(Those use cases are only devoid of such issues to the extent that there is no way to trick the code in question into running a string that the user didn't intend to run that way.  Such bugs are not unheard-of.)
> and, to my knowledge, the JS console

Note that the JS console does this in a clean environment, not against its window scope.  Therefore it doesn't actually use eval.

But yes, we could expose an API for this if it's _really_ needed.  Heck, you could use evalInSandbox, no?
(In reply to comment #4)
> Feel free to ask questions in the newsgroups, esp. with pointers to the unclear
> docs, if any.  I assume you've read
> https://developer.mozilla.org/En/Components.utils.evalInSandbox

Yes, not that this helps much. The documentation has two problems, 1) it does not tell you how to cause the environment of the sandbox to completely mimic the environment of the web page, 2) most of the article is devoted to obscure and complex discussions of security problems with evalInSandbox, so it does not solve the foot-shooting problem after all.

evalInSandbox is fine for toy problems, but when we tried it in Firebug we encountered a stream of problems and we were not able to find anyone who could help us sort them out.
Dup of bug 477380?
(In reply to comment #10)
> Dup of bug 477380?

That bug includes setTimeout. While Firebug does not use eval(), it does rely on extensive use of setTimeout.
With a script-string, or with a function?  (I guess more importantly: in a way that doesn't work with a function rather than source string?)
> Dup of bug 477380?

Er.. looks like it.  Dup in your preferred direction.

> it does not tell you how to cause the environment of the sandbox to completely
> mimic the environment of the web page

If you're using eval() in a way that this bug proposes to disallow you're about 3 astronomical units from "mimic the environment of the web page".

> security problems with evalInSandbox

I believe SJOW fixes those; not sure whether we automatically use it for sandbox yet.  Blake would know.

> we were not able to find anyone who could help us sort them out

I don't recall you ever asking me, or in any forum I follow (which is most of our public developer-oriented newsgroups).  Let's take this to .platform and get it sorted out.  I'd welcome specific examples of issues you had with evalInSandbox and how you're using eval() to solve them.
(In reply to comment #12)
> With a script-string, or with a function?  (I guess more importantly: in a way
> that doesn't work with a function rather than source string?)

Always a function, as far as I know. We don't need setTimeout(), we need what
setTimeout() does when you send it a function. 

We could make the same statement for eval(), but evalInSandbox is not the
function.
(In reply to comment #13)

> > we were not able to find anyone who could help us sort them out
> 
> I don't recall you ever asking me, or in any forum I follow (which is most of
> our public developer-oriented newsgroups).  Let's take this to .platform and
> get it sorted out.  I'd welcome specific examples of issues you had with
> evalInSandbox and how you're using eval() to solve them.

The closest I can find is Bug 421593. 

But I think you are mis-interpreting me here. I am not advocating for 'eval()' in extension code (yet). I am trying to make it clear why evalInSandbox is not an effective alternative to eval().  The reason is that evalInSandbox very clearly and by design has some differences from eval(), but we cannot tell what those differences are. I really spent a lot of time trying it.
> The closest I can find is Bug 421593. 

In terms of behavior, evalInSandbox is no different from a content-privileged eval for purposes of that bug (which is why you get the behavior you observe: content script can't touch that object).

> I am trying to make it clear why evalInSandbox is not an effective alternative
> to eval()

It sure is, for eval() invocations that are actually safe to start with.

> but we cannot tell what those differences are.

Evaluating the string with a principal other than the system principal.  The rest is commentary.
(In reply to comment #16)
> > The closest I can find is Bug 421593. 
> 
> In terms of behavior, evalInSandbox is no different from a content-privileged
> eval for purposes of that bug (which is why you get the behavior you observe:
> content script can't touch that object).
> 
> > I am trying to make it clear why evalInSandbox is not an effective alternative
> > to eval()
> 
> It sure is, for eval() invocations that are actually safe to start with.
> 
> > but we cannot tell what those differences are.
> 
> Evaluating the string with a principal other than the system principal.  The
> rest is commentary.

Perhaps that seems simple to you, but for me its answering a question with a question. I don't know what to do with a function that has that property.

But by now I manage to misdirect this bug completely, sorry. 

I think content and chrome code should support dynamic evaluation. I don't care if its via eval().
(In reply to comment #13)
... 
> > it does not tell you how to cause the environment of the sandbox to completely
> > mimic the environment of the web page
> 
> If you're using eval() in a way that this bug proposes to disallow you're about
> 3 astronomical units from "mimic the environment of the web page".

But the two use cases for eval() in chrome are 
  1) for chrome purposes (see Bug 529079), so you need the system principal.
  2) for content purposes from chrome calls. Both eval() and evalInSandbox() are 3AU distant. I've wandered in that quadrant and wish to forget the experience. 
This combination is why evalInSandbox is not helpful. I hope this is clearer.
> evalInSandbox() are 3AU distant.

evalInSandbox should be able to give a pretty good equivalent of content eval.  If it doesn't, it's just a bug.
(In reply to comment #19)
> > evalInSandbox() are 3AU distant.
> 
> evalInSandbox should be able to give a pretty good equivalent of content eval. 
> If it doesn't, it's just a bug.

I think it would be very helpful to all extension developers who might be affected by this bug if someone would change the MDC page to explain how to use evalInSandbox to get the equivalent to content eval but called in extension scope. The ingredients in a good explanation would include just what is needed to safely accomplish this goal, and nothing more. Specifically. the explanation should avoid discussions of "principals" because these are not things we can do anything about.  The current instructions are a complete mystery because they talk about URLs and principals but we have nsIDOMWindows. Similarly the Security section of the current documentation is very troubling, since the whole point of useing evalInSandbox is to avoid exactly the kind of analysis it seems to suggest is required.
Whiteboard: [sg:want]
(In reply to comment #7)
> In that case, we should give a specific and explicit way to do so:
> evalWithFullPrivileges or such.

I think that's a bad idea. How long until this function shows up in the forums as the easy way to make your extension compatible with the next version of Firefox? That will pretty much defeat the purpose of the change. evalInSandbox should do for all of the few extensions that actually need to run user-supplied code. And - sure, we would need documentation outlining straightforward ways to convert the common uses of eval() (I wrote down some of it in https://adblockplus.org/blog/five-wrong-reasons-to-use-eval-in-an-extension but that's an opinion piece and not documentation).
(In reply to comment #22)
> (In reply to comment #7)
> > In that case, we should give a specific and explicit way to do so:
> > evalWithFullPrivileges or such.
> 
> I think that's a bad idea. How long until this function shows up in the forums
> as the easy way to make your extension compatible with the next version of
> Firefox? That will pretty much defeat the purpose of the change.

If you don't want to lose functionality, there is necessarily going to be an equivalent way of achieving what eval() does now. So, fundamentally, this problem cannot be avoided. I agree it sucks, but that's how it is...


> evalInSandbox
> should do for all of the few extensions that actually need to run user-supplied
> code.
Uh, no. evalInSandbox runs the user-supplied code with reduced privileges. There is no way that I can tell to make it run with elevated privileges (indeed, if there was that might be a security problem), thus making it impossible to use it for the same purpose as such extensions use eval().
Is this really the right bug to fix now?

We once had eval |in| every object (via Object.prototype.eval). That was a pre-ECMA Netscape JS1 thing, we got rid of it. That definitely caused trouble.

We had bugs figuring out the principals to use; fixed also.

What's left? If you get rid of eval, do you also get rid of Function? It would not make sense to remove one but not the other. CSP allows site policy to do this but for chrome, do we really benefit by forcing a JS subset?

I think the horses have left the barn, and we have fixed the door and hinges as much as we can. If there's a hazardous API left, as Gijs rightly suggests will be needed and as shaver allowed, then what good have we done?

The problem is not eval at this point, it is that add-ons run with full effective user-id of the Firefox process. Let's focus on that.

/be
See also bug 529474 evalInGlobalScope as a secure alternative to eval
I think for alpha1, or at least an early alpha, we need to decide what to do here, we either prevent eval, or we close this bug and focus on what brendan suggests in comment 24. Blake, thoughts?
Assignee: nobody → mrbkap
blocking2.0: ? → alpha1
I don't think this needs to block alpha 1, but we should decide one way or another here soon.
blocking2.0: alpha1 → beta1
eval is just a way to compile Javascript. It's not secure or insecure. Removing it won't make code more or less secure. 

Extension developers that create security problems do so because they cannot understand the implications of some operations. The problem here is very simple: the security mechanisms in the platform are too difficult to understand from the available information. You can solve this problem by lopping off parts of the API until extensions can't do anything interesting, but I urge you to address the design and documentation of the security system instead.

Since I guess evalInSandbox is the simplest (but not only) alternative to eval() available, that is a good place to start. Extension developers would use evalInSandbox if anyone understood how to use it.
(In reply to comment #28)
> eval is just a way to compile Javascript. It's not secure or insecure. Removing
> it won't make code more or less secure. 

Well, eval itself might not be inherently insecure, but it makes it much easier to write an insecure extension. Any time you're injecting code that could be influenced by unknown attackers you're walking into dangerous territory.

> from the available information. You can solve this problem by lopping off parts
> of the API until extensions can't do anything interesting, but I urge you to
> address the design and documentation of the security system instead.

Well, the question is really whether the API that we offer is the right one for the job. eval is a pretty big hammer that makes it hard to any one thing correctly. More targeted APIs (like evalInSandbox) are the way to go, but we definitely (as you mention) need to flesh them out.

As brendan said in comment 24, the eval ship has sailed (I guess I'll mark this WONTFIX), but hopefully, we'll be able to move extensions off of 'eval' and onto something like evalInSandbox which can offer APIs that make it harder to shoot oneself in the foot.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
Removing blocking flag then. Feel free to reopen/renominate if you disagree with the decision here.
blocking2.0: beta1 → ---
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.