search

luh2 / DetectDynamicJS

The DetectDynamicJS Burp Extension provides an additional passive scanner that tries to find differing content in JavaScript files and aid in finding user/session data.
GNU General Public License v3.0
65 stars 19 forks source link

request changes #1

Closed 1lastBr3ath closed 8 years ago

1lastBr3ath commented 8 years ago

Hi,

I'm planning to improve your extension, along with your help, by implementing the checks I listed above. If you have any questions or problems, please ping me back. We may need to work along.

Please go through the changes I've done, and files I've added.

Thanks, Prakash

luh2 commented 8 years ago

Dear Prakash, Thank you for your contribution! I really like the effort and ideas! I'm sorry it took me almost two weeks to answer.

Some ideas:

  1. I like to add more ambient authority removals for checks. Not quite sure if I always want it to be removed or as extra check yet though.
  2. The pull request as it is now is too big. I need them to be smaller to accept them. One pull request for one change (e.g. check for status code) About all your points:
  3. StatusCode is a good idea, but it has to be checked not only on the authenticated request, but also on the response. If one of the two is a valid script from the 200-family, detection in an attack scenario will work
  4. For right now I'd like to leave the detection of the filetype, because this is also coupled with x-content-type-options and the content-type and the detection mechanism of Burp occasionally has given me false negatives.
  5. I really like the idea of implementing that. If you have a pull request for this one, that'd be great. 4 & 5. I like the idea of using burp suites, pull request welcome
  6. yes for ii) and iiii) (please separate pull requests) , not sure about iii) and iv) does not need to happen, because if a ressource hasn't been modified that's fine too (just need to implement that when comparing responses in compareResponses

Your comment:

The second request, unauthenticated one, sends previously received responseBody as its body params, which most of the time results in error

I think I fixed that one in Version 0.6, but if you find errors you might also open issues.

The test you're doing only addresses "DYNAMIC JAVASCRIPT DETECTION", while the test I'm doing addresses XSSI in general.

That is not true, it's covering category 2 and 3 of XSSI vulnerabilities. Please read my article to get a better idea of the concept. This article will also explain, as to why you can't exclude [ as first character, which I saw you did.

The print messages are for debugging purpose only.

I don't want debugging output production code. I use debugging information offline, but this repository should stay free of that information.

On a quick note: My name is not Jules Winnfield, it's a character from Pulp Fiction as are all other versionnames. :)

Looking forward to including your work!

1lastBr3ath commented 8 years ago

Hi,

Sorry, I didn't quite get your first idea.

As you've said, I'm making pull requests for every single changes I make. I've added "statusCode check" (#1) for now, please check it and verify it from your end too.

About #2, For efficiency reasons, you should first check the inferredMimeType, if it is NOT script, I doubt there's any point in moving forward. If it's script, you can check further to verify false negatives. And, as I said, Content-Type or X-Content-Type-Options don't seem to have any effects here (I'm searching on it). About #3, Yes! Sure, I'll send a pull request for it too. It may require some improvements though.

About #4 & #5 I was working on it, and had managed to make it work, but it made things really slow. May be, it needs to spawn a separate thread for doing so. I'm having hard time getting things done here.

About #6

not sure about iii) and iv) does not need to happen, because if a ressource hasn't been modified that's fine too (just need to implement that when comparing responses in compareResponses

I guess, removing non-standard headers is vital, because modern web applications make heavy use of these. For example, instagram uses X-CSRFToken as an anti-csrf token, which if removed, results in error. Also, Twitter uses Referer header as protection against CSRF. About 304 responses, we could just compare the statusCode and flag it as vulnerable.

I think I fixed that one in Version 0.6, but if you find errors you might also open issues.

Yes, it's fixed now.

This article will also explain, as to why you can't exclude [ as first character, which I saw you did.

I already read your article. I removed [, because JavaScript no longer allows overriding array constructors. I also included < as I've found applications implementing <json> or similar invalid tags to protect against XSSI, because browsers stop parsing as soon as it encounters < (if not string).

I've removed all changes so far, including debugging messages. Just including the statusCode check. I didn't know about the versionname :)

luh2 commented 8 years ago

About #1: Way more the way I like it! Please fix according to comments. About #2: If it is JSON, starting with [, it is possible to escape in IE and Edge (not using construction overriding, but UTF-7), so I want to know about it. inferredScript (also there is statedScript) will miss that! Maybe we can have a way to label requests where the inferredScript type is not "script" and if the other checks are still returning true, it can be reported as Information. This way I don't loose the information. About #6: I'm all for X-. When it comes to Referer I want to investigate beforehand. Generally, I'm more interested in having a few more false-p, sometimes strange browser behaviors make them worth it. And If I exclude those findings from the beginning on would be a shame. Maybe something similar can be done as for #2. About #7: I'm fine with excluding < as first char.