Address false negatives via non-({...}) callbacks.

[jjarmoc]

I noticed there's a false negative condition in the current version. This PR is an effort to address that, and thus identify more vulnerable scenarios.

Previously, only callbacks which a method({... formatted call were flagged as vulnerable. This PR changes occurrences of ({ to ( for broader detection.

• callbackMethod('...')
• callbackMethod("...")
• callbackMethod(...)

For testing purposes, @BenHayak's SOME demo exhibits the false negative case.

Callbacks from the colorpicker there generate an exploitable response containing:

<script>opener.changeColor('#414141')</script>

The extension was missing it, and now it's not. :)

[nwalsh1995]

Hi @jjarmoc, thanks for your PR. Originally I checked for the curly braces to reduce FP as the plugin was noisy. Now, with my PR #3 we could go with this approach to make it broader.

I will pull these changes and test locally.

[jjarmoc]

Thanks!

I've been running with this change for a while, and FPs haven't been too bad, though that'll vary a bit depending on the application.

My largest false-positive case right now is due to short request param values being seen as controlling a callback, when it's really just a common suffix. They aren't super common, but it's less than ideal to see High severity issues w/ Certain confidence that wind up being FPs. Maybe it's worth a bit of discussion...

These stem from requests with params like:

• param=1
• param=on
• param=data

being matched to JS in responses such as;

• Method1(...)
• MethodCondition(...)
• MethodProcessData(...)
• Random image or binary files that happen to have an 0x3128 sequence (that's 1( ASCII)

I've been thinking about how this could be accounted for without risking additional FNs. It probably wouldn't be appropriate to require a minimum length globally, and I'm not sure how we could easily distinguish complete method names from substrings.

My best idea right now is to adjust the reporting issue so that the confidence is adjusted depending on the length, with something like 5+ chars being the minimum for 'Certain'.

[BenHayak]

Hey @jjarmoc thanks for contributing to this project! Regarding the FP you guys discussed, I liked the idea of the severity change based on certainty.

In addition though, looking at your examples of FPs I think there might be more to consider... I've never seen a callback case with a parameter value being reflected directly as a suffix to a fixed string (even though it may theoretically happen its not worth the FPs) therefore, I would change the searching of javascript according to a definite match of the parameter value or a match with a leading dot each of which followed by an opening of either parenthsis or ES6 backticks for example:

• param= on
  on( //match
  .on( //match
  on` //match
  .on` //match
  stringon( //not match!
  stringon` //not match!

//cc @nwalsh1995