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

Merge TLS1.3 Feature Branch into Main #406

Open
wants to merge 47 commits into
base: master
Choose a base branch
from

Conversation

developStorm
Copy link
Member

@developStorm developStorm commented Feb 18, 2025

  • Passes Go unit tests
  • ZGrab works with this branch and negotiates TLS13 correctly
  • Larger scale test with Censys

    Hey, wanted to report back on my testing. Once I updated a few go dependencies, I was able to integrate into our monorepo (I do need to verify the go dependencies with other services, but that is an internal issue). Once that was done, I spot checked several hosts, paying particular attention to ones that support multiple TLS versions (we support TLS 1.3, 1.2, 1.1, 1.0, and SSLv3 when doing versioning checks). I also checked TLS handshake fingerprints that we perform, like JA3S and JA4S.
    All of these tests passed!
    When we do TLS versioning, we use the zcrypto TLS 1.3 branch for these versions: TLS 1.3, 1.2, 1.1, 1.0. That is what I switched over to your refactor merge branch. For SSLv3, we use the zcrypto main, as the TLS 1.3 branch was missing SSLv3 support.
    I presume this is still the case, or does your refactor branch add back in SSLv3? (It is fine if it does not) (this is expected).

I manually inspected every failed Go official unit test after the merge with Wireshark, confirming root cause of the change in recorded traffic flow, and apply fix or adopt the new flow as appropriate.

I also tested this with zgrab. Confirmed we are using TLS 1.3 (and also seems like we supported some new signature algorithms too!)

(Left original, right with new zcrypto)
image
image
image

Features discontinued to support

This list may be incomplete due to the significant differences between branches. I discussed this with @phillip-stephens, and we agreed to identify any missing features on the fly after merging. Since basic TLS and testing with Censys went smoothly, I’m confident that any missing features are unlikely to cause major issues.

  • HeartBeat extension / Heartbleed exploitation
  • Next Protocol Negotiation (deprecated RFC, replaced by ALPN)
  • SSLv3 (can workaround with old branch)
  • ...

dissoupov and others added 30 commits July 15, 2021 10:17
* [DRAFT] Handshake logs

* Removed heartbleed

* Added handshake logs for ZGrab2

* Added handshake logs for ZGrab2

* TLS1.3: added cert logs

* TLS1.3: adding keyAgreement logs

* TLS1.3: log SignatureAndHashes

* TLS1.3: log SupportedCurves

* TLS1.3: log ServerKeyExchange

* TLS 1.3: adding legacy Key Agreement algs
* Extract and output session ticket lifetime hint

This restores the functionality from commit db98bd3, on the TLSv13
branch

* tls: support ForceSessionTicketExt for ticketSupported
For TLS 1.3 connections, SupportedVersions.SelectedVersions will be
present, and be 0x0304.  Add this to the HandshakeLog, if present.
Do not overwrite collected server certs when we are asked for a client cert.
#331)

* Added extension IDs for Server Hello messages to handshake log

* Added marshalling capabilities for unknown extensions with empty data

* Switched to extension extract function on serverHelloMsg

* Re-added whitespace break
…erNotice (#334)

* x509: make jsonifyExtensions() public

* Certificate Policies: add grouped user notices field

The separate fields for NoticeReferencNumbers, NoticeRefOrganization,
and ExplicitTexts introduce ambiguity since these fields are structured
and optional in the source data.

A certificate with a mixture of UserNotices that have only one of
ExplicitText or NoticeReference would previously be impossible to
reconstruct.

Add a new field, UserNotices, which preserved the original grouping of
values, leaving the old format exposed in place, so that this case can
be reconstructed without breaking existing usage.
Add option for CT log client to emit unparseable certs
Prior to the TLS 1.3 backport, there was a type assertion to make sure
that cert.PublicKey.(*rsa.PublicKey) was true.  This was lost in the
backport work, and while very rare we did recently hit a case where
this assertion is not true.  Doing it inline in the call leads to a
panic.

This restores the prior type assertion check, and returns err if it
fails.
The x509 package sets this field true when it finds a valid signature
while validating certificates; copy the behavior here for consistency.
verifier: add AppendFromPEMErr method
verifier: set ValidSignature for certificates in the graph
@developStorm developStorm self-assigned this Feb 18, 2025
@developStorm developStorm marked this pull request as ready for review March 4, 2025 22:11
Copy link
Contributor

@phillip-stephens phillip-stephens left a comment

Choose a reason for hiding this comment

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

This is too large to reasonably review line-by-line. I've done some sanity checks and wanted to report back on why I think this is safe to merge.

After running a 1% of IPv4 space ZMap scan to ports 443 (HTTPS) and 587 (SMTP), I followed up with running a ZGrab scan on the hits with tls.

Of the over 700k targets, there was a slight uplift in success rate with TLS 1.3 included:

  • master -> 67.43%
  • TLS13 -> 68.03%
    With 4,206 new successful hosts able to handshake with TLS13 support.

This tracks with my observed ratio of hosts that only support TLS 1.3, I found that 0.6% of hosts only supported TLS 1.3 on 100k targets, so this explains the difference.

There are some removed fields from the handshake_log, specifically:

data.tls.result.handshake_log.key_material, data.tls.result.handshake_log.client_key_exchange, data.tls.result.handshake_log.server_key_exchange,data.tls.result.handshake_log.client_finished, data.tls.result.handshake_log.server_finished, data.tls.result.handshake_log.key_material.pre_master_secret

@zakird
Copy link
Member

zakird commented Mar 6, 2025 via email

@developStorm
Copy link
Member Author

developStorm commented Mar 6, 2025

@zakird

grep "unsupported protocol version 300" smtp_and_tls_zgrab_13_branch.output | wc -l
802

Out of 705,737 targets

@phillip-stephens phillip-stephens self-requested a review March 11, 2025 17:00
Copy link
Contributor

@phillip-stephens phillip-stephens left a comment

Choose a reason for hiding this comment

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

@developStorm This looks significantly better w.r.t. bringing back TLS 1.2 scan data. There are 2 things I'm noticing in the diff though:
CLI invocation - echo "100.0.59.51, , , 443" | ./zgrab2 tls | jq

 diff /tmp/master-100.0.59.51.json /tmp/branch-100.0.59.51.json
...
133c133
<               "browser_error": "x509: failed to load system roots and no roots provided"
---
>               "browser_error": "x509: unknown error"
...
159,160c159,160
<                 "signature_algorithm": "rsa",
<                 "hash_algorithm": "sha256"
---
>                 "signature_algorithm": "unknown.227",
>                 "hash_algorithm": "sha384"

Can we:

  1. Bring that browser_error back if it indeed was caused by not loading the certs?
  2. Figure out why that signature algorithm says unknown.227

I'm running a bulk scan to be sure there's no regressions there now.

@phillip-stephens
Copy link
Contributor

A 1% scan of TLS and SMTP services yields a 1.24% increase in successful responses between master and a ZGrab branch using this version of ZCrypto.

So large-scale scans look good. I think if we can get those two fields fixed, this is good to go.

@zakird
Copy link
Member

zakird commented Mar 12, 2025

@developStorm once you've gotten those fields added back in, can you paste before and after scans of a few hosts to spot check?

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.

8 participants