Work in Progress - Unintended assignment

[codelion]

Trying to implement rules to check for unintended assignment in if/while etc.

[codelion]

Thanks @pauljt for your comments. I removed the semi colon. If you think this way of checking for unintended assignment is okay, I can go ahead and add the rules for while/for/switch etc.

[pauljt]

Hey @codelion thanks for getting involved! Really cool. Sorry I meant to leave more useful feedback but got sidetracked testing your code. I was a little unclear about what the ifstatement logic was trying to do. I’m offline on a train at the moment, but I’ll have a look over the weekend.

Currently scanjs doesn’t really support nested statements - IIUC you are trying to detect whenever there is an assignment expression inside the attribute of an ifStatement. But in order to do this you will need to walk down the AST checking all of the nodes within the test to make sure that none of them are an AssignmentExpression. Maybe this is what your code tries to do, i wasn’t sure.

I found that this pattern didn’t seem to get caught though:

if(foo && a=b){blah}

What I think we need is a template function which does “ensure none of the nodes in this tree are of type XXX”. Then you could use this in your unintended assignment expression. This would also be useful for innerHTML assignment (e.g. check that node of the node on the right hand side of an assignment to innerHTML are identifiers)

You might be able to use acorn.walk for this, not sure.

On 6 Jun 2014, at 4:44 am, Asankhaya Sharma notifications@github.com wrote:

Thanks @pauljt for your comments. I removed the semi colon. If you think this way of checking for unintended assignment is okay, I can go ahead and add the rules for while/for/switch etc.

— Reply to this email directly or view it on GitHub.

[codelion]

Thanks for your comments and feedback so far @pauljt. I added the check that looks recursively for the AssignmentExpression in the AST using the findNodeAt function from acorn.walk. However, what I found was that the following example you gave still cannot be handled.

if(foo && a=b){blah}

I looked into it by logging just after acorn.parse function is called. It seems like above is not parsed by acorn.parse so it does not even reach the code for checking rules in scanJS.

[pauljt]

Hmm ok, I wonder if that is actually valid js then - I think it should be, but I am not sure.

[pauljt]

So this seems to work now as expected - good stuff. I'm confused as to why you check:

testNode.test.type == "AssignmentExpression"

That means that this template will only return true, if the test rule is something like if(foo=bar). This means this ifstatement rule isn't very flexible currently.

I think the challenge here is that while the concept is simple, its complex to express in the rule language that we currently have. Basically, we want to write a rule like: if(contains AssignmentExpression)

But rules have to be valid js, since we use acorn to parse them. So what about if we wrote the rules something like:

if($_contains("AssignmentExpression"))

Then we add two new templates, something like:

$_contains:function(node,typestring){
       var foundnode = acorn.walk.findNodeAt(node.test,null,null,typestring);
      return typeof foundnode != 'undefined'
},
ifstatement:{
     nodeType: "IfStatement",
     test: function(testNode, node) {
          //contains support
          if(testNode.test.type=="CallExpression"&&testNode.test.callee.name=="$_contains"){
                  templateRules.$_contains(testNode.test.arguments[0])
           }
     }
}

Or something like that. We kind of need to add nested statement support in order to support this properly..... like if(foo==bar); won't match an ifstatement with foo==bar as it's test. But im not sure that we really need this for security rules so maybe just do the unintended assignment bit first.

What do you think?

[pauljt]

PS see #148 for more deatils on why $_contains might be useful.

[codelion]

I think it is a good idea to support matching inside the AST using $_contains, I tried to use it for unintended assignment based on your suggestion above.

[pauljt]

@codelion thanks for this! I finally got time to test it properly. I made a coupe minor edits and added your rule to the main rule spreadsheet here (https://docs.google.com/a/mozilla.com/spreadsheets/d/1T14mvMMAspnvhKXxPNRBiV0x6QKZsXr8_b7eXJvEf_A/edit?alt=json#gid=1178582613).

I also added support for $_unsafe keyword in assignments, using your contains method.

[codelion]

@pauljt Thank you for adding in the changes. For further development around Scan.JS, do you think it is a good idea to spend some time to figure out how to include type information that can help us avoid false positives and in future may lead to doing some kind of taint analysis of user inputs. If you have some other plans or ideas for contributions please do share.