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

Implement --negotiate-freely to address server renegotiation requests / "no renegotiation" error #334

Closed
wants to merge 19 commits into from

Conversation

mzpqnxow
Copy link
Contributor

Feature as requested in #252, which is required for some endpoints. Without this, negotiation fails with:

"error":"local error: no renegotiation"

How to Test

To reproduce a failure, you'll need to find an endpoint that has the behavior of requesting a renegotiation during the handshake. I have an example here that I found by grepping for "tls: no renegotiation" in a sample dataset I had handy:

$ echo mx-stg-mbr-migration-websvc-scus.azurewebsites.net  | ./zgrab2 http --use-https -p 443                                                               
INFO[0000] started grab at 2021-11-16T10:24:12-05:00    
{"domain":"mx-stg-mbr-migration-websvc-scus.azurewebsites.net","data":{"http":{"status":"unknown-error","protocol":"http","result":{},"timestamp":"2021-11-16T10:24:12-05:00","error":"local error: tls: no renegotiation"}}}

With the patch applied, and with the --renegotiate-freely flag, it works as expected:

$ echo mx-stg-mbr-migration-websvc-scus.azurewebsites.net  | ./zgrab2 http --use-https -p 443 --renegotiate-freely
INFO[0000] started grab at 2021-11-16T10:24:54-05:00    
{"domain":"mx-stg-mbr-migration-websvc-scus.azurewebsites.net","data":{"http":{"status":"success","protocol":"http","result":{"response":{"status_line":"403 Forbidden","status_code":403,"protocol":{"name":"HTTP/1.1","major":1,"minor":1},"headers":{"content_length":["1976"],"content_type":["text/html"],"date":["Tue, 16 Nov 2021 15:24:54 GMT"],"server":
...

Notes & Caveats

I think it may be best to remove the flag and make this a default setting rather than a flag. There may be potential security concerns by allowing unlimited renegotiation requests, but the goal of zgrab2 (and zcrypto) is to be as compatible as possible, not to negotiate unbreakable TLS sessions, obviously :)

In addition, I'll add that there are three settings for this behavior in golang tls- disallow, allow once, or allow freely- as many times as the server requests. I think it's best to either allow it as many times as the server wants or not at all

Issue Tracking

#252

@mzpqnxow
Copy link
Contributor Author

Also- ughh, I'm sorry the commits are so messy, I must have done some unusual merging to get my fork up to date

@mzpqnxow
Copy link
Contributor Author

I have some high level data on how common this configuration is and where it tends to exist, for those interested

Based on the input I have, most of the sites with this renegotiation behavior are in Azure

  1. Azure Traffic Manager (trafficmanager.net)
  2. Azure Cloud App / Cloud Gateway (azuregateway-*.cloudapp.net)
  3. Azure Websites (azurewebsites.net)

I see a very small handful of hosts that do not have Azure names or are not in Azure,, but the majority are

It's a relatively small number of hosts in my scope, only about a half a percent across ~75k endpoints

@mzpqnxow
Copy link
Contributor Author

mzpqnxow commented Feb 15, 2022

@dadrian @zakird any interest in meeting this one? The implementation is straightforward, I just need to fix the merge conflicts that have popped up since I sent it

Thanks!

EDIT: also would appreciate any thoughts on whether this should remain as opt-in or be default behavior

@mzpqnxow
Copy link
Contributor Author

mzpqnxow commented Apr 13, 2022

@dadrian @zakird sorry to be persistent, just a nudge for this one

More than any of the other things I've sent along, I think this feature is critical in for comprehensive coverage with the zgrab2 HTTPS captures. Configurations with this behavior may not comprise the majority of HTTPS services out there, but they're non-zero in my scope of assets, which adds up at scale

Thanks as always for your patience and consideration with PRs :)

EDIT: I can fix the merge conflicts, just looking for a yay/nay on whether you would accept this

@dadrian
Copy link
Member

dadrian commented Apr 13, 2022

Sorry for the delay. Do feel free to @ me on general, it's more likely I'll notice. This seems like a reasonable idea. I can do a review once merge cleanup is fixed, please @ me again.

@mzpqnxow
Copy link
Contributor Author

Sorry for the delay. Do feel free to @ me on general, it's more likely I'll notice. This seems like a reasonable idea. I can do a review once merge cleanup is fixed, please @ me again.

Terrific, thanks! I generally hesitate to poke people but I will keep this in mind for issues like this

@mzpqnxow
Copy link
Contributor Author

I'm closing this but will reenter a new one, I got all mixed up with the changes to the heartbleed option ...

Seanstoppable added a commit to Seanstoppable/zgrab2 that referenced this pull request Oct 14, 2023
This is driven by  zmap#378, zmap/zcrypto#364 and
zmap#334

This allows a number of scans to actually succeed, rather than fail out
when parsing the certificate

Example without permissive parsing:

```
echo FAILING_IP | ./zgrab2 http -p 443 --use-https
INFO[0000] started grab at 2023-09-21T21:25:29-05:00
{"ip":"FAILING_IP","data":{"http":{"status":"unknown-error","protocol":"http","result":{},"timestamp":"2023-09-21T21:25:29-05:00","error":"tls: failed to parse certificate from server: asn1: structure error: explicitly tagged member didn't match"}}}
INFO[0001] finished grab at 2023-09-21T21:25:29-05:00
{"statuses":{"http":{"successes":0,"failures":1}},"start":"2023-09-21T21:25:29-05:00","end":"2023-09-21T21:25:29-05:00","duration":"987.606886ms"}
```

With Permissive parsing:

```
echo FAILING_IP | ./zgrab2 http -p 443 --use-https --permissive-parsing
INFO[0000] started grab at 2023-09-21T21:25:34-05:00
{"ip":"FAILING_UP","data":{"http":{"status":"application-error","protocol":"http","result":{"response":{"status_line":"302 Found","status_code":302,"protocol":{"name":"HTTP/1.1","major":1,"minor":1},"headers":{"content_length":["0"],
... all the HTTP and TLS handshake log data
```
zakird pushed a commit that referenced this pull request Feb 18, 2024
* Add permissive parsing TLS option

This is driven by  #378, zmap/zcrypto#364 and
#334

This allows a number of scans to actually succeed, rather than fail out
when parsing the certificate

Example without permissive parsing:

```
echo FAILING_IP | ./zgrab2 http -p 443 --use-https
INFO[0000] started grab at 2023-09-21T21:25:29-05:00
{"ip":"FAILING_IP","data":{"http":{"status":"unknown-error","protocol":"http","result":{},"timestamp":"2023-09-21T21:25:29-05:00","error":"tls: failed to parse certificate from server: asn1: structure error: explicitly tagged member didn't match"}}}
INFO[0001] finished grab at 2023-09-21T21:25:29-05:00
{"statuses":{"http":{"successes":0,"failures":1}},"start":"2023-09-21T21:25:29-05:00","end":"2023-09-21T21:25:29-05:00","duration":"987.606886ms"}
```

With Permissive parsing:

```
echo FAILING_IP | ./zgrab2 http -p 443 --use-https --permissive-parsing
INFO[0000] started grab at 2023-09-21T21:25:34-05:00
{"ip":"FAILING_UP","data":{"http":{"status":"application-error","protocol":"http","result":{"response":{"status_line":"302 Found","status_code":302,"protocol":{"name":"HTTP/1.1","major":1,"minor":1},"headers":{"content_length":["0"],
... all the HTTP and TLS handshake log data
```

* Make permissive parsing the default/no option
@mzpqnxow mzpqnxow deleted the feature/renegotiate branch October 23, 2024 02:22
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.

5 participants