Closed Bug 509500 Opened 15 years ago Closed 15 years ago

Validator should treat setTimeout/setInterval as potentially dangerous

Categories

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: eviljeff, Assigned: rjwalsh)

References

()

Details

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: 

setTimeout and setInterval both allow arbitrary code to be executed, like eval.  eval is checked for as warning function so setTimeout should be treated the same.

Reproducible: Didn't try
Was listed in bug 371210 comment 1.

Though, it would get very annoying if it just checked for any use and not just with a string parameter.
Summary: addon verification suite should treat settimeout as potentially dangerous → Validator should treat setTimeout/setInterval as potentially dangerous
Status: UNCONFIRMED → NEW
Ever confirmed: true
What would be a good regex to use to test?
Assignee: nobody → rjbuild1088
Target Milestone: --- → 5.0.9
We can probably use the same stuff as we're using for eval, namely something like:

/\b(setTimeout|setInterval)\s*\(/
(In reply to comment #1)
> Was listed in bug 371210 comment 1.
> 
> Though, it would get very annoying if it just checked for any use and not just
> with a string parameter.

I don't think the use can be determined pre-run time.
I use these things all over the place, but I'm not using strings. A warning will be fairly pointless if it hits too many people with a false positive. If you think it's worth it, then I guess my perfect validation score for Crash Report Helper just goes away. ;)

Frankly, to do this sanely we really need to run the JavaScript and then debug it.
CCing mfinkle:  do you have an opinion on this?
Only passing a string to setTimeout/setInterval is dangerous. Passing a true function is quite useful, sometimes required.
(In reply to comment #7)
> Only passing a string to setTimeout/setInterval is dangerous. Passing a true
> function is quite useful, sometimes required.

Do you think searching with a regex for setInterval( followed by a single or double quote would be useful?  Too many false positives?  Or a different regex?
(In reply to comment #8)
> Do you think searching with a regex for setInterval( followed by a single or
> double quote would be useful?  Too many false positives?  Or a different regex?

I was thinking the same thing, but I think that would get too many false negatives. If someone downloads some JS from a server somewhere then feeds it to setTimeout through a string variable, this wouldn't catch it at all. (which is the whole point of this test)
Dave’s comment 9 makes me wonder: Is it common to pass a string to setTimeout or setInterval in a “dangerous” manner (e.g. the string comes from the server or the user input)?  If not, I would suggest Wontfix because it will not be worth the time to devise a clever regexp trick to catch those rare insecure cases.

Checking for eval sounds good because (I guess) it is a common mistake to use eval insecurely.  But what about setTimeout and setInterval?

(Well, I do not think it a good coding practice to pass a string to setTimeout or setInterval anyway, but being insecure and being a bad practice are two different things.)
By the way, checking for setTimeout/setInterval with a string is fine with me as long as the reason for the check is because it is a potential source of bugs, not because it is potentially dangerous.
Well, we do have to remember that these are warnings, not full errors. If there's the potential for an abuse then it makes sense to detect it so that a person can go in and check further. I understand the argument that we should ignore rare things, but that's not what this test is for.

If we detect eval then we have to also detect setTimeout/setInterval because they can be used just like eval. setTimeout(string,0) is largely equivalent to eval(string), except that setTimeout will do things asynchronously which can be useful (though I've never done this, myself).

I think the solution here is to detect these but maybe handle their warnings better. Maybe put something to the effect of "this is only bad if you're parsing a string" up with it? Not sure; there's got to be a good way to do this.
By the way, banning string JS in chrome outright is bug 477380.
(In reply to comment #12)
> If we detect eval then we have to also detect setTimeout/setInterval because
> they can be used just like eval. setTimeout(string,0) is largely equivalent to
> eval(string), except that setTimeout will do things asynchronously which can be
> useful (though I've never done this, myself).

I know that setTimeout/setInterval can be used insecurely.  The question is, is that a common mistake?

If developers just want to suppress the warning about eval and do not want to fix the real issue, then I agree that setTimeout(string, 0) can be used as a workaround in many cases.  But I am pretty sure that such developers will find another way to suppress the new warning about setTimeout/setInterval as well.  We should not spend time playing a cat-and-mouse game with those developers, especially because the game will end up with adding (probably many) false positives.

I agree that a better presentation of the warnings may help developers learn and fix the real issue.

(In reply to comment #13)
> By the way, banning string JS in chrome outright is bug 477380.

Wow.  I am not sure if that is a good idea, but thanks for the information.
(In reply to comment #9)
> (In reply to comment #8)
> > Do you think searching with a regex for setInterval( followed by a single or
> > double quote would be useful?  Too many false positives?  Or a different regex?
> 
> I was thinking the same thing, but I think that would get too many false
> negatives. If someone downloads some JS from a server somewhere then feeds it
> to setTimeout through a string variable, this wouldn't catch it at all. (which
> is the whole point of this test)

calling setTimeout with just a hardcoded string { setTimeout("alert()",0) } is actually safe as we know what it will do consistently and can check it accordingly.  The normal usage is the dangerous one as its difficult pre-runtime to know what the variable from just the syntax. { setTimeout(oo,0); } 'oo' could be either a 'safe' function or an 'unsafe' string (http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSTimeoutHandler.cpp#224).
setTimeout/setInterval are necessary for many add-ons. I know I use it very often. I think checking for its use will produce too much noise and will make the check almost useless.

I think in this case it's better to check for the use of double or single quotes in the function call. There will be some false negatives, but at least it will be useful.
(In reply to comment #16)
> I think in this case it's better to check for the use of double or single
> quotes in the function call. 

why?  A hard coded string is pre-determined.  Its variables that aren't.
(In reply to comment #17)
> (In reply to comment #16)
> > I think in this case it's better to check for the use of double or single
> > quotes in the function call. 
> 
> why?  A hard coded string is pre-determined.  Its variables that aren't.

I think the general practice of passing strings to these functions should be avoided. And I think that is the only detection that can be made without making it useless due to false positives.
I don't think that it would be a good idea to have setInterval/setTimeout produce warnings. Better introduce a "notice" class and provide some meaningful help.

There is the setTimeout-to-let-other-stuff-do-updates-before-we-continue pattern is pretty common I guess, and would therefore generate a lot of noise.
This is used often when updating some UI stuff and/or to partition long computations that would otherwise block the UI.

If you decided to implement some rules, here are some things that are in widespread use that pose no risk in themselves when passed to setTimeout/setInterval:
|function()|, i.e. both |function() { expr; }| and |function() expr|
|new Function("hardcoded")|
(+ additional brackets around)

Another note (that crossed my mind when I tried to imagine a regex):
|setTimeout("head" + something_evil + "tail",0)| which might be unsafe
as opposed to |setTimeout("alert(\" + hardcoded + \")",0)| which is OK.
(In reply to comment #18)
> I think the general practice of passing strings to these functions should be
> avoided. And I think that is the only detection that can be made without making
> it useless due to false positives.
Why?  A constant string is *safe* and it doesn't do anything bad.  The worst that happens is that the JS engine has to parse the string, and it can't be traced.  This isn't terrible - just a small performance hit that all JS developers probably already know about.
(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > I think in this case it's better to check for the use of double or single
> > > quotes in the function call. 
> > 
> > why?  A hard coded string is pre-determined.  Its variables that aren't.
> 
> I think the general practice of passing strings to these functions should be
> avoided. And I think that is the only detection that can be made without making
> it useless due to false positives.

It may be the only detection that can be made easily but its the safest use as we know what a constant string will do!  A variable holding a String is bad but as I mentioned earlier up JS doesn't decide whether it form of the function used is the String version or the Function version based on syntax - its determined in the C++ implementation by checking the object type at runtime.

Most of the uses are probably valid and safe but we can't determine so we either decide they're all potentially unsafe or all safe.  (Or some new semi-warning situation)
As I wrote in comment 10 and comment 11, there are two issues about passing a string to setTimeout/setInterval.
(1) It is insecure if the string cannot be trusted.
(2) It is easy to get it wrong, hard to debug, etc.

To check for (1), we have to create a new framework for validation.  While it might be interesting, it is not what we can do soon.  In addition, I do not think that it is a common mistake, as I wrote in comment 10 and comment 14.

To check for (2), probably checking for /\bset(?:Timeout|Interval)\s*\("/ will suffice in most cases.  (If any of you are an editor of AMO, can you enlighten me about whether this estimate is correct?)

So the action to take depends on the reason why we want to check for setTimeout/setInterval with a string argument.
Something like /\bset(?:Timeout|Interval)\s*\(\s*(?:"|')/ is more robust.

As for the argument to use it or not, I still think that flagging all usages will yield too many false positives. Detecting for strings and not variables is probably as good as it gets without the noise, but there are also good arguments that it will also be pretty pointless. I personally think it's not a good practice to hide your code in a string, given that it's trivially easy to avoid it.
What is so hard about using setTimeout and setInterval?  I think the likelihood of false positives here far outweighs the benefits of checking for this.
Fundamentally, if we're checking for eval and looking for insecure string JS execution, then we need to check for these too. It's a big hole in what is being attempted if we don't. I think the only real issue here is how we do this without being a nuisance. I see two options here:

1) Downplay warnings like this in some way as to not make them a problem.
   This could be done either by adding a message to explain the situation
   (see comment 12) or by adding a lower level of severity below "warning".
   (see comment 19)
2) Postpone dealing with this until we have a test that can actually test
   what we're trying to test. Regexp just doesn't cut it for this.
(In reply to comment #25)
> 1) Downplay warnings like this in some way as to not make them a problem.
>    This could be done either by adding a message to explain the situation
>    (see comment 12) or by adding a lower level of severity below "warning".
>    (see comment 19)
And that's a great way to get this warnings ignored by add-on authors.

> 2) Postpone dealing with this until we have a test that can actually test
>    what we're trying to test. Regexp just doesn't cut it for this.
Maybe, but editors still have to look at these add-ons.  These verification tests aren't going to change that.  If somebody wants to do something malicious, they can:
window["eval"](stringToExecute) works and we really cannot check for that because there are lots of ways to obfuscate it.

I'm saying that it's not worth the effort to try and check for malicious uses of setTimeout or setInterval because we are more likely to flag someobody trying to use it in a safe manner than we are to catch someone trying to beat us.
(In reply to comment #26)
> (In reply to comment #25)
> > 1) Downplay warnings like this in some way as to not make them a problem.
> >    This could be done either by adding a message to explain the situation
> >    (see comment 12) or by adding a lower level of severity below "warning".
> >    (see comment 19)
> And that's a great way to get this warnings ignored by add-on authors.

I'd read all the messages it gives me, personally. But yeah, it's a problem.

> > 2) Postpone dealing with this until we have a test that can actually test
> >    what we're trying to test. Regexp just doesn't cut it for this.
> Maybe, but editors still have to look at these add-ons.  These verification
> tests aren't going to change that.  If somebody wants to do something
> malicious, they can:
> window["eval"](stringToExecute) works and we really cannot check for that
> because there are lots of ways to obfuscate it.
> 
> I'm saying that it's not worth the effort to try and check for malicious uses
> of setTimeout or setInterval because we are more likely to flag someobody
> trying to use it in a safe manner than we are to catch someone trying to beat
> us.

These tests are NOT to catch something fully malicious; they're to catch accidentally insecure code. Clearly, if someone is doing something malicious here it'd be quite easy to avoid the tests. Yes, an editor is always going to have to look at this stuff. These tests are just the first line of defense to catch general mistakes.

Ditching the abstractness and going with an actual example: An extension needs to run a user/remote script and dumps it into eval, which could be unsafe. An extension could also do this by dumping it into setTimeout to do it later or now but just asynchronously. I'd like the developer to be informed right away that this is a bad idea. With this system we're just trying to catch these things faster than waiting for an editor review, and with less workload for the editors.
In order to even attempt to scan for possibly obfuscated malicious code, we'd need something more in depth than regexp. Is there a bug sitting around here to request actually running and debugging the code yet? Though, such a thing may be just a pipe dream, but it'd be nice. :)
(In reply to comment #28)
> In order to even attempt to scan for possibly obfuscated malicious code, we'd
> need something more in depth than regexp. Is there a bug sitting around here to
> request actually running and debugging the code yet? Though, such a thing may
> be just a pipe dream, but it'd be nice. :)

No.  We have JSHydra on the servers, which isn't the same but may be able to assist us a bit more than regex.  However, we're not talkinga bout malicious code here, we're talking about common mistakes authors may make and helping them out.
I see lots of grumbling in this bug, but I don't know why. Everyone seems to believe eval should be treated as dangerous. IMO, setTimeout/setInterval should be treated like eval, _if_ a string is passed in as the first parameter. To not do so would be hypocritical.

Also, I do not believe add-on authors (an JS developers in general) understand the performance penalty of passing a string into setTimeout/setInterval. I have seen far too many:

setTimeout("myObj.myFunction()", x);
(In reply to comment #30)
> I have seen far too many:
> 
> setTimeout("myObj.myFunction()", x);

If I get you right, this implies that even this (safe but slow) use of setTimeout makes sense to be notified about, as we could point out that it *may* be unsafe, but it *definitely* performs badly. Right?
(In reply to comment #30)
> I see lots of grumbling in this bug, but I don't know why. Everyone seems to
> believe eval should be treated as dangerous. IMO, setTimeout/setInterval should
> be treated like eval, _if_ a string is passed in as the first parameter. To not
> do so would be hypocritical.
I agree with you if it's a string passed in via a variable.  How, exactly, is a constant string unsafe?  It's not evaluating random code from somewhere else...
RJ: Let's add some regex's similar to:

setTimeout\("
setInterval\("

as well as a brief blurb on https://addons.mozilla.org/en-US/firefox/pages/validation.  We can mention performance impact too if that's relevant (comment 30).
Plus unit tests!  Let me know if we need to tweak the wording on pages/validation
Attachment #395928 - Flags: review?(clouserw)
Attachment #395928 - Flags: review?(clouserw) → review+
Fixed in r49747
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
I don't think this is really fixed at all.

The dangerous case that we would want to catch is {setTimeout(stringfromwebsite,0);}.

Hard-coded strings are *SAFE*, variables that contains strings aren't - we're testing for the wrong condition.  If the general consensus/conclusion is that we don't want the false positives of variables containing functions then okay but this fix isn't a fix.  Might as well roll it back and mark it WONTFIX.
(In reply to comment #36)
> The dangerous case that we would want to catch is
> {setTimeout(stringfromwebsite,0);}.
> 
> Hard-coded strings are *SAFE*, variables that contains strings aren't - we're
> testing for the wrong condition.  If the general consensus/conclusion is that
> we don't want the false positives of variables containing functions then okay
> but this fix isn't a fix.  Might as well roll it back and mark it WONTFIX.
I agree.
Alright, let's whack the " off the end and treat it the same way as eval( and see how many false positives we have.
Attached patch Everything bug strings (obsolete) — Splinter Review
Attachment #396246 - Flags: review?(clouserw)
I don't think it's worth ignoring hardcoded strings. I think just checking for setTimeout/setInterval by itself should be fine.
Comment on attachment 396246 [details] [diff] [review]
Everything bug strings

Agreed.  We want to treat it just like eval(, so just check for an open parenthesis
Attachment #396246 - Flags: review?(clouserw) → review-
(In reply to comment #40)
> I don't think it's worth ignoring hardcoded strings. I think just checking for
> setTimeout/setInterval by itself should be fine.
Why?  It's *safe* to use just a string.
(In reply to comment #42)
> (In reply to comment #40)
> > I don't think it's worth ignoring hardcoded strings. I think just checking for
> > setTimeout/setInterval by itself should be fine.
> Why?  It's *safe* to use just a string.

There's the case noted in comment 19 of setTimeout("head" + something_evil + "tail",0) which could get missed if filtering out due to leading quotes. If the unsafe string was pieced together in some way like this it'd be missed. Yeah, probably not a common case, but nonetheless true.
Attachment #396246 - Attachment is obsolete: true
Attachment #396283 - Flags: review?(clouserw)
(In reply to comment #42)
> (In reply to comment #40)
> > I don't think it's worth ignoring hardcoded strings. I think just checking for
> > setTimeout/setInterval by itself should be fine.
> Why?  It's *safe* to use just a string.

It's not safe under 100% of the situations, so we have to be conservative and
flag any usage. If we had better parsing tools, we could check for variables
being passed to setTimeout/setInterval.

I'd rather have false positives than miss evil uses.
Attachment #396283 - Flags: review?(clouserw) → review+
Yay, 8 warnings from this in one extension. I'd call that noisy. ;)

Nonetheless, this is working so I'll mark this verified.
Status: RESOLVED → VERIFIED
How about allowing only - setTimeout( function() {
},delay);  usage , this will handle cases of passing the string through a variable  or a direct string argument. This can be validated with a regexp easily. 

As of now the Addon validator is giving lot of false positives , there is no way a setTimout can avoid warnings.
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: