Skip to content

fix: indefinite hangs during TLS handshake and cipher enumeration #960

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.

Sign up for GitHub

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

Closed
allornothingai wants to merge 3 commits into from
+40 −14
Closed

fix: indefinite hangs during TLS handshake and cipher enumeration #960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
37436e1
fix: indefinite hangs during TLS handshake and cipher enumeration
Mar 15, 2026
27c1b5d
fix: Add timeout validation to EnumerateCiphers
allornothingai Mar 17, 2026
679cf13
chore: Address CodeRabbit nitpicks
allornothingai Mar 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions internal/pdcp/writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ type UploadWriter struct {
assetGroupID string
assetGroupName string
counter atomic.Int32
droppedCounter atomic.Int32
closed atomic.Bool
TeamID string
}
Expand All @@ -65,7 +66,7 @@ func NewUploadWriterCallback(ctx context.Context, creds *pdcpauth.PDCPCredential
u := &UploadWriter{
creds: creds,
done: make(chan struct{}, 1),
data: make(chan *clients.Response, 8), // default buffer size
data: make(chan *clients.Response, 1000), // increased buffer size
TeamID: "",
}
var err error
Expand All @@ -91,7 +92,12 @@ func NewUploadWriterCallback(ctx context.Context, creds *pdcpauth.PDCPCredential
// GetWriterCallback returns the writer callback
func (u *UploadWriter) GetWriterCallback() func(*clients.Response) {
return func(resp *clients.Response) {
u.data <- resp
select {
case u.data <- resp:
default:
u.droppedCounter.Add(1)
gologger.Warning().Msgf("PDCP upload buffer full, skipping result")
}
}
}

Expand Down Expand Up @@ -125,6 +131,9 @@ func (u *UploadWriter) autoCommit(ctx context.Context) {
} else {
gologger.Info().Msgf("Found %v results, View found results in dashboard : %v", u.counter.Load(), getAssetsDashBoardURL(u.assetGroupID, u.TeamID))
}
if dropped := u.droppedCounter.Load(); dropped > 0 {
gologger.Warning().Msgf("Dropped %v results due to upload buffer overflow", dropped)
}
}()
// temporary buffer to store the results
buff := &bytes.Buffer{}
Expand Down
10 changes: 9 additions & 1 deletion pkg/tlsx/tls/tls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,10 +236,18 @@ func (c *Client) EnumerateCiphers(hostname, ip, port string, options clients.Con

conn := tls.Client(baseConn, baseCfg)

if err := conn.Handshake(); err == nil {
ctx := context.Background()
var cancel context.CancelFunc
if c.options.Timeout != 0 {
ctx, cancel = context.WithTimeout(ctx, time.Duration(c.options.Timeout)*time.Second)
}
Comment on lines +239 to +243
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 17, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the file structure and locate the relevant files
fd -t f "tls\.go|utils\.go" | grep -E "(pkg/tlsx|tlsx)" | head -20

Repository: projectdiscovery/tlsx

Length of output: 203

🏁 Script executed:

# Read the actual code at lines 239-243 and surrounding context
cat -n pkg/tlsx/tls/tls.go | sed -n '230,260p'

Repository: projectdiscovery/tlsx

Length of output: 1252

🏁 Script executed:

# Find and examine the clients/utils.go file to understand the timeout default
fd -t f "utils\.go" | xargs grep -l "Timeout" | grep clients

Repository: projectdiscovery/tlsx

Length of output: 90

🏁 Script executed:

# Read the clients/utils.go file
cat -n pkg/tlsx/clients/utils.go

Repository: projectdiscovery/tlsx

Length of output: 5897

🏁 Script executed:

# Search for Timeout handling and GetConn function in clients
rg -A 10 -B 5 "Timeout.*==.*0|GetConn|5.*time\.Second" pkg/tlsx/clients/

Repository: projectdiscovery/tlsx

Length of output: 2422

🏁 Script executed:

# Check imports in tls.go to verify ProjectDiscovery library usage
head -50 pkg/tlsx/tls/tls.go | grep -E "^import|^\"" | head -30

Repository: projectdiscovery/tlsx

Length of output: 73

🏁 Script executed:

# Get full imports section
cat -n pkg/tlsx/tls/tls.go | sed -n '1,30p'

Repository: projectdiscovery/tlsx

Length of output: 1067

🏁 Script executed:

# Final verification: Check if there are any other places in EnumerateCiphers that handle timeout
cat -n pkg/tlsx/tls/tls.go | sed -n '200,255p'

Repository: projectdiscovery/tlsx

Length of output: 2120

Add default handshake timeout to cipher enumeration to prevent indefinite blocking.

pkg/tlsx/clients/utils.go treats Timeout == 0 as a 5-second default via GetConn(). However, EnumerateCiphers uses pool.Acquire() directly, bypassing this default. Without a context deadline, conn.HandshakeContext() can hang indefinitely if a peer accepts TCP but never completes the TLS handshake.

Proposed fix 
-		ctx := context.Background()
-		var cancel context.CancelFunc
-		if c.options.Timeout != 0 {
-			ctx, cancel = context.WithTimeout(ctx, time.Duration(c.options.Timeout)*time.Second)
-		}
+		timeout := time.Duration(c.options.Timeout) * time.Second
+		if timeout == 0 {
+			timeout = 5 * time.Second
+		}
+		ctx, cancel := context.WithTimeout(context.Background(), timeout)
 		if err := conn.HandshakeContext(ctx); err == nil {
 			ciphersuite := conn.ConnectionState().CipherSuite
 			enumeratedCiphers = append(enumeratedCiphers, tls.CipherSuiteName(ciphersuite))
 		}
-		if cancel != nil {
-			cancel()
-		}
+		cancel()
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/tlsx/tls/tls.go` around lines 239 - 243, EnumerateCiphers currently
creates a background context and only sets a deadline if c.options.Timeout != 0,
causing conn.HandshakeContext to potentially block indefinitely; change the
logic in EnumerateCiphers (around ctx := context.Background(), pool.Acquire and
conn.HandshakeContext) to treat c.options.Timeout == 0 as a 5-second default
(the same default used by GetConn()), i.e. create a context.WithTimeout(ctx,
time.Duration(timeout)*time.Second) where timeout := c.options.Timeout; if
timeout == 0 set timeout = 5, and ensure you call the cancel function defer
cancel() after creating the context so the deadline is always enforced and
resources are released.

if err := conn.HandshakeContext(ctx); err == nil {
ciphersuite := conn.ConnectionState().CipherSuite
enumeratedCiphers = append(enumeratedCiphers, tls.CipherSuiteName(ciphersuite))
}
if cancel != nil {
cancel()
}
_ = conn.Close() // close baseConn internally
}
return enumeratedCiphers, nil
Expand Down
31 changes: 20 additions & 11 deletions pkg/tlsx/ztls/ztls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func (c *Client) ConnectWithOptions(hostname, ip, port string, options clients.C

// new tls connection
tlsConn := tls.Client(conn, config)
err = c.tlsHandshakeWithTimeout(tlsConn, ctx)
err = c.tlsHandshakeWithTimeout(ctx, tlsConn)
if err != nil {
if clients.IsClientCertRequiredError(err) {
clientCertRequired = true
Expand Down Expand Up @@ -257,10 +257,18 @@ func (c *Client) EnumerateCiphers(hostname, ip, port string, options clients.Con
conn := tls.Client(baseConn, baseCfg)
baseCfg.CipherSuites = []uint16{ztlsCiphers[v]}

if err := c.tlsHandshakeWithTimeout(conn, context.TODO()); err == nil {
ctx := context.Background()
var cancel context.CancelFunc
if c.options.Timeout != 0 {
ctx, cancel = context.WithTimeout(ctx, time.Duration(c.options.Timeout)*time.Second)
}
if err := c.tlsHandshakeWithTimeout(ctx, conn); err == nil {
h1 := conn.GetHandshakeLog()
enumeratedCiphers = append(enumeratedCiphers, h1.ServerHello.CipherSuite.String())
}
if cancel != nil {
cancel()
}
_ = conn.Close() // also closes baseConn internally
}
return enumeratedCiphers, nil
Expand Down Expand Up @@ -321,19 +329,20 @@ func (c *Client) getConfig(hostname, ip, port string, options clients.ConnectOpt
}

// tlsHandshakeWithCtx attempts tls handshake with given timeout
func (c *Client) tlsHandshakeWithTimeout(tlsConn *tls.Conn, ctx context.Context) error {
func (c *Client) tlsHandshakeWithTimeout(ctx context.Context, tlsConn *tls.Conn) error {
errChan := make(chan error, 1)
defer close(errChan)

go func() {
errChan <- tlsConn.Handshake()
}()

select {
case <-ctx.Done():
return errorutil.NewWithTag("ztls", "timeout while attempting handshake") //nolint
case errChan <- tlsConn.Handshake():
}

err := <-errChan
if err == tls.ErrCertsOnly {
err = nil
case err := <-errChan:
if err == tls.ErrCertsOnly {
err = nil
}
return err
}
Comment on lines +332 to 347
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 17, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's read the file to see the code at lines 332-347 and surrounding context
wc -l pkg/tlsx/ztls/ztls.go

Repository: projectdiscovery/tlsx

Length of output: 90

🏁 Script executed:

# Read the specific section mentioned in the review
sed -n '320,360p' pkg/tlsx/ztls/ztls.go

Repository: projectdiscovery/tlsx

Length of output: 816

🏁 Script executed:

# Search for EnumerateCiphers function and how baseCfg.CipherSuites is used
rg -A 30 "func.*EnumerateCiphers" pkg/tlsx/ztls/

Repository: projectdiscovery/tlsx

Length of output: 1792

🏁 Script executed:

# Search for how tlsHandshakeWithTimeout is called and how baseCfg is passed
rg -B 10 "tlsHandshakeWithTimeout" pkg/tlsx/ztls/

Repository: projectdiscovery/tlsx

Length of output: 1835

🏁 Script executed:

# Get more context around the tlsHandshakeWithTimeout call in EnumerateCiphers
rg -B 5 -A 15 "conn := tls.Client.*baseConn.*baseCfg" pkg/tlsx/ztls/

Repository: projectdiscovery/tlsx

Length of output: 1274

🏁 Script executed:

# Search for the loop that contains this code
rg -B 30 "baseCfg.CipherSuites = \[\]uint16" pkg/tlsx/ztls/

Repository: projectdiscovery/tlsx

Length of output: 1860

🏁 Script executed:

# Check if there's any defer cancel() or cleanup that ensures goroutines are waited on
sed -n '140,280p' pkg/tlsx/ztls/ztls.go

Repository: projectdiscovery/tlsx

Length of output: 4787

🏁 Script executed:

# Check for go.mod or go version requirements
cat go.mod | head -5

Repository: projectdiscovery/tlsx

Length of output: 126

🏁 Script executed:

# Check tls.Config.Clone() documentation from Go standard library
web_search
Go tls.Config Clone() method standard library

Repository: projectdiscovery/tlsx

Length of output: 246

🌐 Web query:

Go crypto/tls Config Clone() method availability version

💡 Result:

(*crypto/tls.Config).Clone() is part of the Go standard library starting in Go 1.8 (“added in go1.8” in the official crypto/tls docs), and is therefore available in Go 1.8+. [1]

Clone the config to prevent race conditions in cipher enumeration.

On timeout, tlsHandshakeWithTimeout returns immediately while the handshake goroutine continues running. Since tls.Client() holds a reference to the config (not a copy), the next iteration's mutation of baseCfg.CipherSuites can affect the in-flight handshake, causing enumeration flakiness under timeout pressure.

Fix: Clone config for each attempt 
-		conn := tls.Client(baseConn, baseCfg)
-		baseCfg.CipherSuites = []uint16{ztlsCiphers[v]}
+		cfg := baseCfg.Clone()
+		cfg.CipherSuites = []uint16{ztlsCiphers[v]}
+		conn := tls.Client(baseConn, cfg)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/tlsx/ztls/ztls.go` around lines 332 - 347, tlsHandshakeWithTimeout
returns on context timeout while the goroutine running tlsConn.Handshake may
still reference a mutable tls.Config (created via tls.Client using baseCfg),
which allows concurrent mutation of baseCfg.CipherSuites to race with the
in-flight handshake; fix by cloning the tls.Config for each handshake attempt so
the goroutine uses an immutable copy (i.e., create a shallow copy of baseCfg or
use baseCfg.Clone() before calling tls.Client/handshake) and pass that cloned
config to tls.Client/tlsConn so mutations to baseCfg.CipherSuites in later
iterations cannot affect the running handshake.

return err
}