Skip to content

Lowered the timeout from one minute to a default of 20 seconds - #5

Open
Gijutsu wants to merge 1 commit into
FiloSottile:masterfrom
Gijutsu:master
Open

Lowered the timeout from one minute to a default of 20 seconds#5
Gijutsu wants to merge 1 commit into
FiloSottile:masterfrom
Gijutsu:master

Conversation

@Gijutsu

@Gijutsu Gijutsu commented May 10, 2016

Copy link
Copy Markdown

which in my tests proved to be around 10 times the required time to get a response from a patched or vulnerable server - but it of course depends on your network latency.

Comment thread server/main.go Outdated
s := &http.Server{
Addr: os.Args[1],
ReadTimeout: 10 * time.Second,
ReadTimeout: 1 * time.Second,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why change these? It's totally reasonable for a connection to stall for 1 second.

@Gijutsu Gijutsu May 13, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree, this change is not necessary to lower the timeout when running against a host that is not responding. The only change that is actually needed is "deadline := time.Now().Add(2 * time.Second)" I'll amend my pull request to reflect this.

@benjojo

benjojo commented May 13, 2016

Copy link
Copy Markdown
Contributor

Cool, Could you squash? It may be a good idea to add this as a flag, since 2 seconds is in the realms of a TCP connection missing something, and in some cases you really want to avoid a false positive, and other times you are less caring :)

@FiloSottile

Copy link
Copy Markdown
Owner

I'm unlikely to put in a default lower than 20 seconds. Some of us have crappy connections :) Please make this a flag if you want to be able to configure something lower.

by adding the command line flags timeout and address while keeping
backwards compability by making the usage of the flags optional.
@Gijutsu Gijutsu changed the title Lowered the timeout from one minute to a maximum of 2 seconds, Lowered the timeout from one minute to a default of 20 seconds May 13, 2016
@Gijutsu

Gijutsu commented May 13, 2016

Copy link
Copy Markdown
Author

Thanks for the feedback. I can see the benefit of making this a flag, so I have converted the PR to use flags while keeping backwards compability by making the usage of the flags optional.

Example usage:
./CVE-2016-2107 --help
Usage of ./CVE-2016-2107:
-address string
Server to test (default "127.0.0.1")
-timeout int
Connection timeout in seconds (default 20)

time ./CVE-2016-2107 8.8.8.8
2016/05/13 23:19:55 dial tcp 8.8.8.8:443: i/o timeout

real 0m20.002s
user 0m0.000s
sys 0m0.000s

./CVE-2016-2107 -address filippo.io -timeout 2
2016/05/13 23:22:06 Vulnerable: false

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.

3 participants