aidansteele commented 1 year ago

Hi,

Here is the draft PR that I mentioned in #439. It is incomplete and only a draft right now, but I am sharing it early to get your feedback.

Which HAR package to use?

I looked at https://github.com/vvakame/go-harlog and some other packages (I found other packages by going through the results of this GitHub search). There were some issues with the other projects that already existed, so I created my own by forking github.com/vvakame/go-harlog. Those issues were:

Their design required that you use one http.RoundTripper per HAR archive. Dalfox creates a http.RoundTripper in a few different places and I didn't want to make big changes to Dalfox.
Their design would store the HAR archive in memory and serialize to JSON at the end. Dalfox can make a lot of requests and I didn't want Dalfox to use lots of memory, so I wanted something that would write the file incrementally during the scan.
Their design would only allow standard HAR files to be written, without modification. I wanted to be able to add a _messageId property to the HAR entries to correlate with Dalfox PoCs like ZAP does.

So that's why I created a new HAR package. The package does the same thing Dalfox does in JSON mode: it writes the [ first, then each JSON object, then commas, then the closing ]. It also allows multiple http.RoundTripper objects (in this case, *har.RoundTripper) to write to a single shared har.Writer.

Question: should it be a standalone package that Dalfox imports in its go.mod or would you prefer it to be contained entirely in Dalfox, e.g. github.com/hahwul/dalfox/pkg/har? I think I prefer it to be a Dalfox package, but I don't know how much code you want in the Dalfox repo. Let me know what you prefer.

Integration into Dalfox

What do you think of the changes to Dalfox so far? Is the design ok? Are there any changes you would like me to make? Right now the code works correctly and generates a HAR archive. I will add another commit that adds a _messageId property to the HAR entries and the Dalfox PoC JSON soon. Maybe in a couple of days.

codecov-commenter commented 1 year ago

Codecov Report

Merging #440 (c3a2e50) into main (f78a653) will not change coverage. The diff coverage is n/a.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main     #440   +/-   ##
=======================================
  Coverage   82.88%   82.88%           
=======================================
  Files          11       11           
  Lines         590      590           
=======================================
  Hits          489      489           
  Misses         79       79           
  Partials       22       22

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

hahwul commented 1 year ago

Hello again @aidansteele First, Thank you so much for cool idea and PR :D

The answer to the question is I don't care. (I like both) Of course, I prefer to be included package in Dalfox as you say. The reason is the advantage of fast maintenance and low-dependency. But I don't think it necessarily needs to be changed.

And yes I like the changes so far. I'm looking forward to the next commit as well :D

Dalfox has a separate Options Interface for importing and using Dalfox from other Go packages. and I think we need to write additional code for this part. I'll take care of it. (this interface was my stupid code. haha...😭)

aidansteele commented 1 year ago

@hahwul thank you for your feedback. I agree that it is easier to maintain if the code is in this repo, so I moved the package here. I've now implemented the code to add the "message IDs" to HAR entries and Dalfox output. Here is some demo of the changes:

Output (pretty-printed) from running url --no-spinner --no-color --output-all --follow-redirects --silence --format json http://testphp.vulnweb.com/listproducts.php?cat=2 --har-file-path dump.har:

Dalfox output (click to expand)

```json [ { "type": "G", "inject_type": "BUILTIN", "poc_type": "plain", "method": "GET", "data": "http://testphp.vulnweb.com/listproducts.php?cat=%2F%2F%2Fwww.google.com%2F%252f%252e%252e", "param": "", "payload": "toOpenRedirecting", "evidence": "", "cwe": "", "severity": "Low", "message_id": 13 }, { "type": "R", "inject_type": "inHTML-URL", "poc_type": "plain", "method": "GET", "data": "http://testphp.vulnweb.com/listproducts.php?cat=2%22%3E%3Csvg%2FOnLoad%3D%22%60%24%7Bprompt%60%60%7D%60%22%3E", "param": "cat", "payload": "\">", "evidence": "48 line: syntax to use near ''>' at line 1", "cwe": "CWE-79", "severity": "Medium", "message_id": 342 }, { "type": "R", "inject_type": "inHTML-URL", "poc_type": "plain", "method": "GET", "data": "http://testphp.vulnweb.com/listproducts.php?cat=2%22%3E%3Cscript+y%3D%22%3E%3C%22%3E%2F%2A%3Cscript%2A+%2A%2Fprompt%28%29%3C%2Fscript", "param": "cat", "payload": "\">\n\n\n \n

\n \n \n\n\n\n

\n\tError: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ''\"> \n\n \n

\n
\n

\n

Warning: This is not a real shop. This is an example PHP application, which is intentionally vulnerable to web attacks. It is intended to help you test Acunetix. It also helps you understand how developer errors and bad configuration may let someone break into your website. You can use it to test other tools and your manual hacking skills as well. Tip: Look for potential SQL Injections, Cross-site Scripting (XSS), and Cross-site Request Forgery (CSRF), and more.

\n

\n\n\n" }, "redirectURL": "", "headersSize": -1, "bodySize": -1 }, "cache": null, "timings": { "blocked": 0.001791, "dns": 0.775084, "connect": 190.115667, "send": 0.016708, "wait": 222.568834, "receive": 0.80275, "ssl": -1 }, "_messageId": 322 } ] ```

And here's an example of the HAR file being loaded in to Chrome dev tools:

chrome screenshot

aidansteele commented 1 year ago

I tried to fix all the Codacy issues but I don't know what to do with these ones. I have already added a package-level comment to round_tripper.go, so I don't think these other files need comments.

CleanShot 2023-02-13 at 14 49 16@2x

aidansteele commented 1 year ago

I also just force-pushed a squashed commit, so there won't be lots of fix-commits if this is merged 😂

aidansteele commented 1 year ago

@hahwul thank you for reviewing it! I don't think there are any more changes that I need to make. So it can be merged now. Of course if you find a bug after merging then let me know and I will be happy to try fix it ASAP. But I have been using my fork for the last week and it seems to work :)

Also if you want me to help look at your PR for the interface thing then I would be happy to!

Thanks 💯

hahwul / dalfox

Add HAR file writing support (#439) #440

Which HAR package to use?

Integration into Dalfox

Codecov Report

$\"Acunetix$

TEST and Demonstration site for Acunetix Web Vulnerability Scanner

Links