mozilla / scanjs Goto Github PK

Using .setAttribute should flag dangerous cases. We are missing the following examples:

var a=document.createElement("form"); a.setAttribute("action", "demo_form.asp"); document.body.appendChild(a);
var a = document.createElement("a"); a.setAttribute("href", "javascript:alert(0)"); document.body.appendChild(a);
iframe.setAttribute("mozapp", data.app);
var a = document.createElement("audio"); a.setAttribute("mozaudiochannel", data.app);

The following rules having pending tests relating to setAttribute:

• action attribute
• href attribute
• mozapp
• mozaudiochannel

This enhancement will be fixed post-1.0 release.

Currently results are printed by file, which is pretty unhelpful if you are actually trying to do a review. It would be better to sort by issue, or even better allow the user to sort the results.

Surely there is a neat bootstrap or angular thing that will allow adding this quickly.

realating to freddyb's changes to moving scan.js into a worker, we should display a throbber indicating if the scan is still running.

I find the mouseover less intuitive than the click. The mouseover doesn't work very well once you have a big app with a lot of results, because you line number and editor are far away from each other.

Another thing I have noticed is that setCursor focuses the CodeMirror instance (for the highlighted line to be actually visible), but that means the whole web page scrolls up..which is also annoying for big pages.

Example screenshot (click to enlarge):

It would be cool if we had different rule sets, depending on the target platform (nodejs, browser, ...)

I was looking into Cordova (aka PhoneGap) earlier today and realized most of the browser cases already cover this. There are some other APIs, but those highly rely on the app and which Cordova plugins are being used.

But this here lists a few items/APIs that aren't in the current rules yet:

• requestFileSystem
• standard HTML FileSystem API, not in Firefox as of Feb 2014 though
• FileReader
  • standard HTML File API, already in Firefox, not sure if interesting enough
• Media
  • for media playback
• navigator.contacts
  • probably shims mozContacts, but its more interesting to see the functions the developers uses, not her libraries ;)
• navigator.accelerometer

Right now, the File Upload functionality in the interface simply populates a CodeMirror HTML div.

This issue is to start the discussion about whether we should use localStorage.

We cannot use console.log inside a web worker, it causes console not defined errors. If we need to pass messages back and forth, we should use postMessage somehow.

Right now, scan.js uses console.log() pretty extensively. So we either need to add conditionals in there to only call console.log() when its being used NOT in a web worker, or we need to remove all console.logs from scan.js.

I went ahead and assigned @mozfreddyb, hope you don't mind :)

We should try to get this fixed ASAP, as master is broken.

freddyb, add me so i can assign/take issues etc!

Currently index.html just has a carefully crated list of JS files thrown into it. Proper module loading would be much better..

One could just restore the bits from before the commits c04df30 and ccbc81a, but I'm struggling to see how to lay out the condition that identifies whether the last results came from a JSON or a JS file.

See index.html

Syntax Errors lead to unscanned files. This should be transported to the UI with $scope.error assignments.

As previously discussed, we need to be able to ignore cases like:

var a = 1;
div.innerHTML = a;

Right now, if you run the server side scan.js after having already run a scan you get this error:

Error:output file or dir already exists (scanresults). Supply a different name using: -o [filename]

This feature request would create file/directories with timestamped dates so that the user can run multiple tests.

Relating to our action attribute rule, ScanJS misses:

var a=document.createElement("div");
a.innerHTML="<form action='demo.asp'></form>";
document.body.appendChild(a);

We do not flag this because innerHTML is assigned from a static string.

We should flag this because we are building a <form> with the action attribute.

Are you OK with AGPL, @pauljt @pwnetrationguru?

Handling files feels a bit clunky. Maybe there is a way to improve this..

For now we have a Controller for the rules and the Scanning parts, but the Scanning thing is already not really for scanning but for file uploads and other things.

Given that the web ui might become more separated and the scanning might once become async (if we use a Worker, see #7) the AngularJS Controller should probably be rearranged.

Also, AngularJS currently doesnt really handle file uploads, so there's already a workaround. But I think I'd lean towards keeping AngularJS as the framework for now

This does not seem completely reproducible.

1. Navigate to http://people.mozilla.org/~rfletcher/scanjs/client/
2. Click rules tab
3. select '.innerHTML'
4. Change rule_desc from: "Assignments to innerHTML with user input can lead to XSS." to: "Assignments to inner<img/src=a onerror=alert(0)>HTML with user input can lead to XSS."

The following error in console:

Error: current is not defined RuleListCtrl/$scope.saveRule@http://people.mozilla.org/~rfletcher/scanjs/client/js/rules.js:14 Za.prototype.functionCall/<@http://people.mozilla.org/~rfletcher/scanjs/client/js/lib/angular-1.2.13.min.js:164 Pc[c]</<.compile/</</<@http://people.mozilla.org/~rfletcher/scanjs/client/js/lib/angular-1.2.13.min.js:181 Cd/this.$get</h.prototype.$eval@http://people.mozilla.org/~rfletcher/scanjs/client/js/lib/angular-1.2.13.min.js:103 Cd/this.$get</h.prototype.$apply@http://people.mozilla.org/~rfletcher/scanjs/client/js/lib/angular-1.2.13.min.js:104 Pc[c]</<.compile/</<@http://people.mozilla.org/~rfletcher/scanjs/client/js/lib/angular-1.2.13.min.js:181 o.event.dispatch@http://people.mozilla.org/~rfletcher/scanjs/client/js/lib/jquery-2.1.0.min.js:3 o.event.add/r.handle@http://people.mozilla.org/~rfletcher/scanjs/client/js/lib/jquery-2.1.0.min.js:3


return a.apply(null,arguments)}

In general, we miss a case I've been calling 'variable indirection'.

If we use the example in the title of this issue, var b = alert; b(1);. We may wish to catch calls to alert(), but right now ScanJS misses the above case.

Failures:

var a = crypto; a.generate = crypto.generateCRMFRequest; a.generate("CN=0", 0, 0, null, "console.log(1)", 384, null, "rsa-dual-use");
var a = window.document; a.b = document.writeln; a.b("<h1>bad</h1>");
var a = window.document; a.b = document.writeln; a.b("<h1>bad</h1>");
var a = eval; a("alert(0);");
var a = Function; new a("alert(0)")();
var a = window.setInterval; a("console.log(4)", 300);
var a = window.setTimeout; a("console.log(4)", 300);
var o = window.open; o("http://www.mozilla.org", "name", {});

The following rules will need to be adjusted:

• generateCRMFrequest:
• document.write
• document.writeln
• eval
• new Function()
• setInterval
• setTimeout
• window.open

Currently, when running tests. We define the name of the rule we're looking for, say .innerHTML. Then we look up that rule in common/rules.js, and pass that single rule to ScanJS.scan().

This is limiting for a few reasons, but mainly, I could see use getting into a situation where we have multiple rules to ensure all dangerous uses are caught. For example, we want to catch var a = mozApps.mgmt and also var a = mozApps.mgmt.getAll().

Right now, I believe, this would require two different rules. If that is the case, then we would need two separate test files that find their specific rule and then run ScanJS.scan() twice with each rule. That violates keeping our code DRY and I could see that becoming more complicated to catch things.

Instead, if we pass the entire rules array each time, while certainly more inefficient since we go through all rules for each test file, we can have one test file that uses mozApps.mgmt in a bunch of ways and not be concerned with 'should this specific rule stop this behavior' and instead focus on 'these are bad patterns, these are good patterns, ScanJS.scan() should catch them'.

I feel pretty strongly about this, after working with tests for a while, but I definitely wanted to get @pauljt and @mozfreddyb opinions! 🍔

Let's just remove these things

Paul suggests checkboxes, so that people may exclude libraries.

A lot of our rules flag variables named the same as the object as dangerous, when the shouldn't.

Currently, we have the following failing test cases:

     CustomEvent -> good.CustomEvent = "CustomEvent";
     addEventListener -> var addEventListener = "variable with name";
     addIdleObserver -> addIdleObserver = "static string";
     addIdleObserver -> something.addIdleObserver = "static string";
     escapeHTML -> var escapeHTML = "just a string";
     localStorage -> var localStorage = "asdf";
     message -> var message = "static string";
     MozActivity -> var MozActivity = "static MozActivity";
     MozChromeEvent -> var mozChromeEvent = "string mozChromeEvent";
     mozPermissionSettings -> var mozPermissionSettings = "just a string, not mozPermissionSettings";
     mozPower ->  var mozPower = "just a string, window.navigator.mozPower;";
     mozSettings -> var mozSettings = "window.navigator.mozSettings;";
     mozSms -> var mozSms = "window.navigator.mozSms;";
     somethingNotNavigator.mozWifiManager;

The following rules need be updated:

• CustomEvent
• addEventListener
• addIdleObserver
• escapeHTML
• localStorage
• message
• MozActivity
• MozChromeEvent
• mozPermissionSettings
• mozPower
• mozSettings
• mozSms
• mozWifiManager

create a rule for crypto.

Related to generateCRMFRequest test:

var a = crypto;
a.generate = crypto.generateCRMFRequest;
a.generate("CN=0", 0, 0, null, "console.log(1)", 384, null, "rsa-dual-use");

rating would be low/medium/high/info, tag could be xss, info disclosure, api use, ...

createContextualFragment rule needs to be updated.

Currently, we miss this dangerous case:

var documentFragment = range.createContextualFragment("<h1>bad</h1>");

We need to switch to providing a lightweight HTTP server so a user can go to http://localhost:9999/scanjs/client vs file:///Users/rfletcher/work/scanjs/scanjs/client/index.html

Right now, in Chrome, we have cross-origin loading errors. More importantly, we use relative file paths throughout the code. This is presenting problems and isn't really how we should be doing it.

For example:
scanjs/client/index.html and scanjs/tests/run-tests.html include scanjs/client/js/initTern.js. scanjs/client/js/initTern.js attempts to load the file js/lib/tern/defs/browser.json.

This means index.html is looking for file js/lib/tern/defs/browser.json inside scanjs/client while run-tests.html is looking for js/lib/tern/defs/browser.json inside of scanjs/tests/

If we had a lightweight HTTP server, initTern.js could simply do a
getJSON('browser', '/scanjs/client/js/lib/tern/defs/browser.json');

As suggested by paul in #63.

We could also link to the relevant section in the FxOS Reviewer Guide

https://developer.mozilla.org/en-US/Apps/Security_guidelines#Firefox_OS_Reviewer_Guide

When uploading JSON results to the client interface for review, the line number is printed out but the line of code is not.

We should also save the line of code in the JSON object we create.

(Copied over from the old repo, originally filed by @pwnetrationguru)

I'm thinking it would be good to have some light tests that we run before merging in PRs.
I don't think this is too heavy handed and I have experience writing some light testing, but I wanted to get everyone's opinion.
@mozfreddyb and @pauljt

Some checks, like the one for .data leads to some very obvious false positives :(

I'd like to introduce a div tag to index.html that are styled to be well placed (i.e. top, center, overlay) to provide visual feedback for all sorts of UI interactions like "scan complete", "scan started", "file loading" etc.

This div tag shall have the alert and one of the alert-info (blue), alert-error (red) and alert-warning (yellow) classes attached to it whenever we call a function that triggers those visual clues.

Those classes should automatically be removed after a certain timeout, e.g. 1 second.
I guess it's unlikely that one would want to display more than one of those things - otherwise we could just include three tags instead of one that is repurposed all the time.

Any comments, @pwnetrationguru? Otherwise I'll take this soonish :)

Very large scripts give me the "Stop Script?" dialog, and rightfully so.
We could do the whole script scanning in a worker.

So far, CodeMirror has proven a little buggy for me.

I'm considering trying ace because it's suppose to be fast, looks awesome and used to be an old Mozilla project! :)

What do you think, @freddyb?

The following input:

var a = 'something not bad';
document.innerHTML = a;
var b = document.location;
document.innerHTML = b;

Produces:

 Line 2:
document.innerHTML = a;
Line 4:
document.innerHTML = b; 

Since we know a is a static string, it would be nice if line 2 did not produce a warning. This might be non-trivial.

Provide an interface that displays AST built from input to user.

Consider the following test for mozSettings rule. Specifically, pay attention to the second 'safe input' test that uses var mozSettings = "window.navigator.mozSettings;";

... Testing Safe Input
      var good = 'var a = "window.navigator.mozSettings;";';
      chai.expect(ScanJS.scan(good, [ScanJS.rules[i]], document.location.pathname)).to.be.empty;
      var good = 'var mozSettings = "window.navigator.mozSettings;";';
      chai.expect(ScanJS.scan(good, [ScanJS.rules[i]], document.location.pathname)).to.be.empty;
... Testing Unsafe Input
      var bad = 'var settings = window.navigator.mozSettings;';
      chai.expect(ScanJS.scan(bad, [ScanJS.rules[i]], document.location.pathname)).not.to.be.empty;});
...

Basically, the var mozSettings = "window.navigator.mozSettings;"; test is failing. I would think it should not since var mozSettings should be different from window.navigator.mozSettings.

I think this is the behavior with all identifier rules.

@pauljt and @mozfreddyb, what do you think? I've still yet to really dig into the guts of ScanJS.scan() so I don't know if this is possible or not.

(originally filed by @pwnetrationguru)

freddyb has the following URL so people can tinker with Scan.js:
http://freddyb.github.io/scanjs/client/
We should either enable that on our github or setup a development domain to point to.

We should probably make it CSP compliant, just to 'dog-food' :)

If we keep using angular.js, ngCsp seems relevant.

Otherwise, its just a matter of moving everything to externally sourced files. This is definitely super low priority but might be good to keep in mind for new features we implement.

It would be nice if one could upload multiple JS files at the same time.
This requires multiple codeMirror instances per file (or changing the content on the fly), some additional file navigation. Question is, would the "Output" part belong to a specific file tab or continue showing for all files?

(This surely wouldn't look nice if #4 isn't fixed though.)

This disallows scanning quite some Gaia and Gecko bits, because we're missing support for common stuff like let, const, yield, ...

This depends on acornjs/acorn#78.

This would help with libraries and is mandatory for app reviews. Viewing code appears pretty useless without this.

I think this should be an option and the user should be allowed to disable.

When scanning, I get:

arguments.join is not a function

postMessage({'type':'log', 'message': arguments.join(" ")});

This issue is to track potential changes to UX layout.

Currently, we have an Input section and an Output section. A better solution might be to simply render the entire JS file in one view and then highlight lines with errors. Also, provide functionality to 'jump' to errors inside files in case file is really large. This will clean up interface and allow best use of real estate.

The web client should support different ways to sort findings (by rule, by file, ..)

Creating new fields and hitting update would, I expect, create a new rule.

Right now it does nothing.

The description is hidden in the rule name's title attribute. It should be more obvious. We might tackle this in another issue, but I wanted this to be spelled out explicitly