Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add response read timeout flag #4944

Merged
merged 6 commits into from
Apr 24, 2024
Merged

Conversation

dogancanbakir
Copy link
Member

Proposed changes

Closes #4942.

test_template.yaml:

id: fuzz-dirs

info:
  name: fuzz-dirs
  author: savik
  severity: info

requests:
  - method: GET
    path:
      - "{{BaseURL}}"
    skip-variables-check: true
    matchers:
      - type: status
        condition: or
        status:
          - 200
          - 201
          - 204
          - 206
          - 304
          - 500
$ go run . -t test_template.yaml -u https://download.binance.com/electron-desktop/mac/production/binance.dmg -rsr 324009984 -rrt 10

                     __     _
   ____  __  _______/ /__  (_)
  / __ \/ / / / ___/ / _ \/ /
 / / / / /_/ / /__/ /  __/ /
/_/ /_/\__,_/\___/_/\___/_/   v3.2.2

                projectdiscovery.io

[WRN] Found 1 templates loaded with deprecated protocol syntax, update before v3 for continued support.
[INF] Current nuclei version: v3.2.2 (latest)
[INF] Current nuclei-templates version: v9.8.0 (latest)
[WRN] Scan results upload to cloud is disabled.
[INF] New templates added in latest release: 85
[INF] Templates loaded for current scan: 1
[WRN] Loaded 1 unsigned templates for scan. Use with caution.
[INF] Targets loaded for current scan: 1
[fuzz-dirs] [http] [info] https://download.binance.com/electron-desktop/mac/production/binance.dmg

Checklist

  • Pull request is created against the dev branch
  • All checks passed (lint, unit/integration/regression tests etc.) with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@dogancanbakir dogancanbakir requested a review from ehsandeep March 25, 2024 22:06
@dogancanbakir dogancanbakir self-assigned this Mar 25, 2024
@dogancanbakir dogancanbakir requested a review from Mzack9999 March 26, 2024 14:58
Copy link
Member

@tarunKoyalwar tarunKoyalwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think max-http-body-size is meant to be handled gracefully[i.e non-fatal/error] ( without error and only use first 4MB) . but currently it errors which is not expected behaviour

$ ./nuclei -t fuzz.yaml  -target https://download.binance.com/electron-desktop/mac/production/binance.dmg --debug

                     __     _
   ____  __  _______/ /__  (_)
  / __ \/ / / / ___/ / _ \/ /
 / / / / /_/ / /__/ /  __/ /
/_/ /_/\__,_/\___/_/\___/_/   v3.2.2

		projectdiscovery.io

[WRN] Found 1 templates loaded with deprecated protocol syntax, update before v3 for continued support.
[INF] Current nuclei version: v3.2.2 (latest)
[INF] Current nuclei-templates version: v9.8.0 (latest)
[WRN] Scan results upload to cloud is disabled.
[INF] New templates added in latest release: 85
[INF] Templates loaded for current scan: 1
[WRN] Loaded 1 unsigned templates for scan. Use with caution.
[INF] Targets loaded for current scan: 1
[INF] [fuzz-dirs] Dumped HTTP request for https://download.binance.com/electron-desktop/mac/production/binance.dmg

GET /electron-desktop/mac/production/binance.dmg HTTP/1.1
Host: download.binance.com
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:109.0) Gecko/20100101 Firefox/122.0
Connection: close
Accept: */*
Accept-Language: en
Accept-Encoding: gzip

[WRN] [fuzz-dirs] Could not execute request for https://download.binance.com/electron-desktop/mac/production/binance.dmg: [:RUNTIME] got err while executing https://download.binance.com/electron-desktop/mac/production/binance.dmg <- could not generate response chain: error reading response body: could not read response body: http: request body too large
[INF] No results found. Better luck next time!

keeping the 4MB limit , i think we should handle errors returned by responseChain in protocols/http/http.go more appropriately i.e in this case , we can warn user and use the first 4MB instead of erroring out

@dogancanbakir
Copy link
Member Author

depends on projectdiscovery/utils#378

@dogancanbakir
Copy link
Member Author

go run . -t test_template.yaml -u https://download.binance.com/electron-desktop/mac/production/binance.dmg 

                     __     _
   ____  __  _______/ /__  (_)
  / __ \/ / / / ___/ / _ \/ /
 / / / / /_/ / /__/ /  __/ /
/_/ /_/\__,_/\___/_/\___/_/   v3.2.2

                projectdiscovery.io

[WRN] Found 1 templates loaded with deprecated protocol syntax, update before v3 for continued support.
[INF] Current nuclei version: v3.2.2 (latest)
[INF] Current nuclei-templates version: v9.8.0 (latest)
[WRN] Scan results upload to cloud is disabled.
[INF] New templates added in latest release: 85
[INF] Templates loaded for current scan: 1
[WRN] Loaded 1 unsigned templates for scan. Use with caution.
[INF] Targets loaded for current scan: 1
[fuzz-dirs] [http] [info] https://download.binance.com/electron-desktop/mac/production/binance.dmg

Copy link
Member

@tarunKoyalwar tarunKoyalwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm ! implementation wise

$  ./nuclei -t a.yaml  -target https://download.binance.com/electron-desktop/mac/production/binance.dmg --debug-req -v -svd             

                     __     _
   ____  __  _______/ /__  (_)
  / __ \/ / / / ___/ / _ \/ /
 / / / / /_/ / /__/ /  __/ /
/_/ /_/\__,_/\___/_/\___/_/   v3.2.2

		projectdiscovery.io

[VER] Started metrics server at localhost:9092
[WRN] Found 1 templates loaded with deprecated protocol syntax, update before v3 for continued support.
[INF] Current nuclei version: v3.2.2 (outdated)
[INF] Current nuclei-templates version: v9.8.1 (latest)
[WRN] Scan results upload to cloud is disabled.
[INF] New templates added in latest release: 77
[INF] Templates loaded for current scan: 1
[WRN] Loaded 1 unsigned templates for scan. Use with caution.
[INF] Targets loaded for current scan: 1
[DBG] HTTP Protocol request variables: 
	1. BaseURL => https://download.binance. .... ac/production/binance.dmg
	2. DN => binance
	3. FQDN => download.binance.com
	4. File => binance.dmg
	5. Host => download.binance.com
	6. Hostname => download.binance.com
	7. Input => https://download.binance. .... ac/production/binance.dmg
	8. Path => /electron-desktop/mac/production
	9. Port => 443
	10. RDN => binance.com
	11. RootURL => https://download.binance.com
	12. SD => download
	13. Scheme => https
	14. TLD => com
	15. ip => 

[INF] [fuzz-dirs] Dumped HTTP request for https://download.binance.com/electron-desktop/mac/production/binance.dmg

GET /electron-desktop/mac/production/binance.dmg HTTP/1.1
Host: download.binance.com
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.4 Mobile/15E148 Safari/604.1
Connection: close
Accept: */*
Accept-Language: en
Accept-Encoding: gzip

[VER] [fuzz-dirs] Sent HTTP request to https://download.binance.com/electron-desktop/mac/production/binance.dmg
[DBG] Http Protocol response variables: 
	1. BaseURL => https://download.binance. .... ac/production/binance.dmg
	...
	44. x_nuclei_ignore_error => http: request body too large

[fuzz-dirs:body] [http] [info] https://download.binance.com/electron-desktop/mac/production/binance.dmg [1.048576e+07]
  1. x_nuclei_ignore_error => http: request body too large

More

  • requesting changes in context of
    • merge conflict
    • integration test failure ( most likely flaky test but we should confirm just in case )

@tarunKoyalwar
Copy link
Member

minor update

  • due to incorrect time unit response-read-timeout value was set to 5 ns instead of 5 sec which caused network tests to fail
 ./nuclei -h |& grep read-timeout
   -rrt, -response-read-timeout value    response read timeout in seconds (default 5ns)
   -irt, -input-read-timeout value  timeout on input read (default 3m0s)

also we have asnmap <-> nuclei unit test , looks like we need to add env secret with pdcp key cc: @ehsandeep @dogancanbakir

@dogancanbakir
Copy link
Member Author

oh yes!

Copy link
Member

@tarunKoyalwar tarunKoyalwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

the failing test seems to be specific to windows environment for scan-all-ips feature , i think we can track and fix it in follow-up pr since it doesn't seem to be related to this and is specific to windows

cc: @dogancanbakir @ehsandeep

@ehsandeep ehsandeep merged commit 8676cb6 into dev Apr 24, 2024
9 of 12 checks passed
@ehsandeep ehsandeep deleted the add_response_read_timeout_flag branch April 24, 2024 07:35
@savushkin-yauheni
Copy link
Contributor

savushkin-yauheni commented Apr 24, 2024

Hi @dogancanbakir @ehsandeep
I'm the author of #4942

Right now we have a bug in final calculation of maxBodylimit:

	maxBodylimit := MaxBodyRead // 10MB

	gologger.Info().Msgf("1: %s", maxBodylimit)
	if request.MaxSize > 0 {
		maxBodylimit = int64(request.MaxSize)
		gologger.Info().Msgf("2: %s", maxBodylimit)

	}
	if request.options.Options.ResponseReadSize != 0 {
		maxBodylimit = int64(request.options.Options.ResponseReadSize)
		gologger.Info().Msgf("3: %s", maxBodylimit)

	}
[INF] 1: %!s(int64=4194304)
[INF] 2: %!s(int64=20485760)
[INF] 3: %!s(int64=10485760)

So even if I set upmax-size in template it would be overwritten by such code ( I set up 20mb but instead we got 10mb):

if request.options.Options.ResponseReadSize != 0 {
		maxBodylimit = int64(request.options.Options.ResponseReadSize)		
}

My command:

nuclei -t fuzz-dirs.yaml -target https://download.binance.com/electron-desktop/mac/production/binance.dmg

I don't use ResponseReadSize option. so it shouldn't affect the calculation

Regards

@dogancanbakir
Copy link
Member Author

dogancanbakir commented Apr 24, 2024

Hi @savushkin-yauheni,
The request.options.Options.ResponseReadSize option has a default value, which is why it overrides the max limit. Ref https://github.com/projectdiscovery/nuclei/blob/dev/cmd/nuclei/main.go#L299

@savushkin-yauheni
Copy link
Contributor

@dogancanbakir
But it doesn't make sense. does it mean that we can't control max-size through templates?
If so, why do we need it at all.

@dogancanbakir
Copy link
Member Author

Yes; creating another issue to work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

invalid behaviour for big http responses (files)
4 participants