mozilla / scanjs

[DEPRECATED] Static analysis tool for javascript code.
Other
428 stars 39 forks source link

[testing][false negative] - document.writeln #80

Closed pwnetrationguru closed 9 years ago

pwnetrationguru commented 10 years ago

Our document.writeln rule currently misses the following dangerous example:

window.document.writeln("Hellow World");
zombie commented 9 years ago

i tried the both this and the .write() example from issue #79, and both match (testhit) window.document.writeln() format.. in fact, they match anything.document.writeln() (as they should imho).

but reading how the scanjs rules are (supposed to) work, there should be a separate $_any.document.writeln() rule for this to work.

which, if i'm right, looks like a bigger issue!

pauljt commented 9 years ago

So this specific issue should be closed (actually a while ago). But you are also correct that this is a bigger issue - or rather a known limitation of scanjs rules.

Basically scanjs checks only work on one AST node, which means you can only look at one object name and property at a time. So the rule for document.write() actually matches all of the following cases:

document.write() foo.document.write() document.write().foo

The point of the $_any object name is so that you can find properties on any object, for example anything in the global namespace like eval because eval can be called like this: window.eval() top.eval()

However because it is global, eval can also be called like eval()

So we actually have two separate rules for eval. This is a little confusing and counterintuitive, so i am actually looking rewriting the rule format (again) at the moment to make it more explicit. While its convenient to be able to write rules as javascript, its confusing since we only support matching a very limited subset of js. I leaning to making explicit rules with well defined arguments. Ill create a seperate issue for this.

Im going to close this issue though since as I mentioned its actually resolved.

pauljt commented 9 years ago

To further illustrate the point above, the following is also matched by the rule "document.write()":

foo.bar.document.write().foo