Changes to check for response other than 200 OK by amitamathkar · Pull Request #15 · luh2/DetectDynamicJS

amitamathkar · 2018-09-20T19:16:47Z

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

Thanks for your pull request! I started reviewing the code. Please improve the pull request according to the comments.

fenceposterror · 2018-09-26T08:57:58Z

DetectDynamicJS.py

+            scan_issues.append(issue)
+            return scan_issues
+        else:
+            if(self.hasScriptContent(newRequestResponse)):


small change - please check if it does not have script content - return None -> less indentation for the rest. :)

fenceposterror · 2018-09-26T08:58:51Z

DetectDynamicJS.py

+
+    def hasScriptContent(self,requestResponse):
+        """
+        Checks if the response of the request contains the scipt content


typo "script"

Typo is fixed.

somehow this did not make it all the way to your commit

Typo is fixed in recent commit.

fenceposterror · 2018-09-26T09:05:15Z

DetectDynamicJS.py

+                                                   secondRequestResponse)
+            scan_issues.append(issue)
+            return scan_issues
+        else:


It looks to me like this else is not necessary. safe an indent. safe a little cats life. Makes the code more readable.

This is fixed as per comments.

fenceposterror · 2018-09-26T09:07:21Z

DetectDynamicJS.py

-                                               secondRequestResponse)
-        scan_issues.append(issue)
-        return scan_issues
+        if(self.isScannableRequest(newRequestResponse)):


Looks like this could be flattened out by an early return as well.

This is modified by changing the if conditions, the code is more flattened out in recent pull commit.

DetectDynamicJS.py

fenceposterror · 2018-10-08T14:28:46Z

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 · 2018-11-29T01:21:52Z

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 · 2018-12-04T16:01:33Z

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 · 2018-12-05T15:00:07Z

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 · 2018-12-05T19:28:49Z

@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 · 2018-12-05T20:31:57Z

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 · 2018-12-17T13:47:31Z

Just merged another pull request - that might solve lots of what you were trying to achieve already. #16

Changes to check for response other than 200 OK

9590420

fenceposterror reviewed Sep 26, 2018

View reviewed changes

Changes as per comments given in previous pull request

1365145

mathkar added 2 commits October 8, 2018 12:09

Add ScriptContent check

0a49b68

Typo fixed

6820415

Conversation

amitamathkar commented Sep 20, 2018

Uh oh!

fenceposterror left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fenceposterror commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amitamathkar commented Nov 29, 2018

Uh oh!

luh2 commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luh2 commented Dec 5, 2018

Uh oh!

ksdmitrieva commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luh2 commented Dec 5, 2018

Uh oh!

luh2 commented Dec 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fenceposterror commented Oct 8, 2018 •

edited

Loading

luh2 commented Dec 4, 2018 •

edited

Loading

ksdmitrieva commented Dec 5, 2018 •

edited

Loading