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

Mark request as successful (short-circuit retry-https) if redirect chain fails due to SSL handshake #307

Closed
mzpqnxow opened this issue Mar 26, 2021 · 2 comments

Comments

@mzpqnxow
Copy link
Contributor

mzpqnxow commented Mar 26, 2021

Please see #300 for more context on this- it's not an SNI issue, but it was exposed as (in my opinion) undesirable (or incorrect) behavior as a result of the SNI issue.

This could be similar to the --redirects-succeed option that is already implemented. The --redirects-succeed option allows the request to be considered as a success, which short-circuits the --retry-https logic and allows capture of the responses from all of the requests leading up to exhaustion of the redirect maximum.

In this case, if there was a --redirects-sslerror-succeed, the invalid length error would not occur. I think if the first request gets a valid HTTP response back, there should be no retry as https- even if there is a handshake failure later on in the redirect response chain.

I thought that --redirects-succeed might already catch this case but I did a quick check and it does not seem to. It seems (as the help output suggests) that it only considers it successful when the cause is due to retry exhaustion, not some other network or protocol issue

I'm just not sure if this should be default behavior or added as an optional flag. It may be better as a general flag that applies to other failures that may occur in the response chain, e.g. if after two redirects a server with a closed port is encountered. The first two requests/responses should be loggable and the request should not be retried as HTTPS, I think. It only makes sense to me for the retry to happen if the first negotiation fails. That's my intuition at least, maybe there's a reason why that's not the default behavior (or maybe it was just an edge-case that was overlooked)

If not targeting SSL handshake failures specifically, it could be a generic option similar to --redirects-succeed, meaning if a valid HTTP redirect response was received on the original request, it should be considered successful

Originally posted by @mzpqnxow in #300 (comment)

@mzpqnxow

This comment has been minimized.

@mzpqnxow
Copy link
Contributor Author

mzpqnxow commented Apr 13, 2022

EDIT: I just realized that I don't seem to have submitted a PR for this... very, very sorry to bother you when there is really nothing to do at the moment- unless you want to take this on ;)

@zakird @dadrian bump on this one

To summarize clearly, the current (not ideal) behavior of a zgrab2 of an HTTPS service is as follows

Assume:

  1. User has set --retry-https
  2. "serviceA" speaks HTTP (plaintext) and redirects to an HTTPS service ("serviceB")
  3. "serviceB" speaks HTTPS, but for some reason zgrab2 is unable to complete the handshake (unsupported cipher-suite, invalid/unsupported certificate, sends early renegotiation as described in Zgrab2 + TLS CLient Cert Auth causes failure, but curl and openssl s_client succeed  #252 / Implement --negotiate-freely to address server renegotiation requests / "no renegotiation" error #334, or a small handful of other reasons)

In this case, zgrab2 knows:

  1. The initial service ("service1") is plaintext HTTP
  2. The redirected service ("service1") is HTTPS (but fails before the handshake fully completes)

Despite knowing these two things, an HTTPS request will be attempted to the initial service ("service1"), which will fail since it is expecting HTTP. This change just passes the error to the caller with more granularity, so it can make an educated decision to avoid this behavior. While it's technically (mostly) harmless, there are two issues that are of consequence to some users:

  1. Unnecessary requests are made (waste of bandwidth, additional total request seen by rate-limiting controls- things that are more likely to be impactful at scale)
  2. Data can be lost, similar to how data is lost completely when too many redirects occur

Issue 2 can currently be remediated via --redirects-succeeed, which captures the successful requests within the larger redirect chain, rather than marking the entire thing as a failure and returning no data at all. This proposed new behavior is similar to that, but for a somewhat different case (as described above)

Thanks, sorry for the persistence :)

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

No branches or pull requests

1 participant