Changes to check for response other than 200 OK

[amitamathkar]

Improvement to the plugin checks the status code of the response and does not report the issue for responses other than 200 OK (e.g. 401 Unauthorized) if the response is not a dynamic script (contribution by Synopsys).

[fenceposterror]

If I understand your first pull request correctly, all it needs in doPassiveScan is to add a or not self.hasScriptContent(baseRequestResponse) into the second if statement of the function. Not saying I haven't missing something. If you disagree, please explain what else it is. I'm just trying to make it understandable again.

[amitamathkar]

If I understand your first pull request correctly, all it needs in doPassiveScan is to add a or not self.hasScriptContent(baseRequestResponse) into the second if statement of the function. Not saying I haven't missing something. If you disagree, please explain what else it is. I'm just trying to make it understandable again.

I think or not self.hasScriptContent(baseRequestResponse) is not needed in second second if statement. As this check is already done in first if statement. What missing was a check that the response of unauthorised request sent by Burp, is scannable and has some dynamic script content or not. That's why the condition not (self.isScannableRequest(newRequestResponse) is added in the second if statement.

[luh2]

You get a login oracle if the second request is not a scannable request. You want to lose that information? Maybe it would be good to introduce a new finding. Take it out for now, and I'll open an issue about that.

[luh2]

So please take it out, and we'll fix that in a separate commit. It's true, we can stop earlier testing, but this is mixing up things, and is still a finding

<script src="https://some.host.com/app/js/ua.js"></script>
<script>
try {
  detectBrowser();
  console.log("Logged in");
  // do something with it
 }
 catch(err) {
 console.log("Not logged in");
 }
</script>

[ksdmitrieva]

@luh2, you are making a good point about the login oracle. I know Sebastian mentions "login state" in the paper, but how would you protect data otherwise. Having an access control where "if the user is logged in, the user can access X", and "if the user is not logged it, respond with Unauthorized" is a standard practice.

I find that when these issues are flagged by DetectDynamicJS, they are marked as false positives in real assessments and are treated as noise. My understanding is that this whole pull request is targeting exactly that scenario, when the unauthenticated response doesn't return cookies, 200 Ok, or any script in the body.

And as you suggest, taking out the condition of checking if the unauthenticated response contains any script basically makes the whole pull request unnecessary.

[luh2]

You could make js files not require authentication. Fairly simple. I guess most apps prefer to just leak their login state, which doesn't make me less want to know about them. But I see your point in, that if it is returned as Dynamic JavaScript it might be misleading and also your point about this pull request. I do want to continue knowing about login oracles, as they can be of value to my work. But reducing the noise sounds good. So why not turn https://github.com/luh2/DetectDynamicJS/blob/master/DetectDynamicJS.py#L86 into a negated if and return a different finding "Login Oracle" or something along those lines, and then after that continuing normally. And now that I finally start to see what this confusing original commit tried to accomplish - why not improve the isScript method instead of adding another one? And I think I'm happy to add ˙[˙ to the ichars, which is probably the one part that actively reduces most of the fp.Not sure it then really still needs the regex, but that is then worth trying to out to see what happens. The rest of that function is anyway more or less the isScript.

[luh2]

Just merged another pull request - that might solve lots of what you were trying to achieve already. https://github.com/luh2/DetectDynamicJS/pull/16