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

Open
allornothingai wants to merge 1 commit into
base: main
Choose a base branch
from
+21 −12
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 6 additions & 2 deletions internal/pdcp/writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,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 +91,11 @@ 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:
gologger.Warning().Msgf("PDCP upload buffer full, skipping result")
}
}
}

Expand Down
4 changes: 3 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,12 @@ func (c *Client) EnumerateCiphers(hostname, ip, port string, options clients.Con

conn := tls.Client(baseConn, baseCfg)

if err := conn.Handshake(); err == nil {
ctx, cancel := context.WithTimeout(context.Background(), time.Duration(c.options.Timeout)*time.Second)
if err := conn.HandshakeContext(ctx); err == nil {
ciphersuite := conn.ConnectionState().CipherSuite
enumeratedCiphers = append(enumeratedCiphers, tls.CipherSuiteName(ciphersuite))
}
cancel()
Comment on lines +239 to +244
Copy link

@coderabbitai coderabbitai bot Mar 15, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Missing timeout validation: Timeout == 0 causes immediate context expiration.

Unlike ConnectWithOptions (lines 110-113), this code does not check c.options.Timeout != 0 before creating the context. If Timeout is 0, context.WithTimeout(..., 0) creates an already-expired context, causing every handshake to fail immediately.

🐛 Proposed fix to guard against zero timeout 
+		timeout := time.Duration(c.options.Timeout) * time.Second
+		if timeout == 0 {
+			timeout = 5 * time.Second // default timeout
+		}
+		ctx, cancel := context.WithTimeout(context.Background(), timeout)
-		ctx, cancel := context.WithTimeout(context.Background(), time.Duration(c.options.Timeout)*time.Second)
 		if err := conn.HandshakeContext(ctx); err == nil {

Alternatively, skip the timeout wrapper when Timeout == 0 to match ConnectWithOptions behavior.

🤖 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 - 244, The handshake loop uses
context.WithTimeout with c.options.Timeout which if zero yields an
already-expired context and makes conn.HandshakeContext fail; modify the code
around context.WithTimeout/conn.HandshakeContext so it first checks
c.options.Timeout != 0 and only creates a timeout context in that case,
otherwise use context.Background() (or context.WithCancel) for the handshake;
update the cancel() usage accordingly to only call cancel when a cancellable
timeout context was created (refer to c.options.Timeout, context.WithTimeout,
conn.HandshakeContext, and how ConnectWithOptions guards Timeout).

_ = conn.Close() // close baseConn internally
}
return enumeratedCiphers, nil
Expand Down
21 changes: 12 additions & 9 deletions pkg/tlsx/ztls/ztls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,10 +257,12 @@ 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, cancel := context.WithTimeout(context.Background(), time.Duration(c.options.Timeout)*time.Second)
if err := c.tlsHandshakeWithTimeout(conn, ctx); err == nil {
h1 := conn.GetHandshakeLog()
enumeratedCiphers = append(enumeratedCiphers, h1.ServerHello.CipherSuite.String())
}
cancel()
Comment on lines +260 to +265
Copy link

@coderabbitai coderabbitai bot Mar 15, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Same timeout validation issue: Timeout == 0 causes immediate context expiration.

This matches the issue in pkg/tlsx/tls/tls.go. Unlike ConnectWithOptions (lines 118-122 in this file), there's no guard against Timeout == 0, which would cause all cipher enumeration handshakes to fail immediately.

🐛 Proposed fix to guard against zero timeout 
+		timeout := time.Duration(c.options.Timeout) * time.Second
+		if timeout == 0 {
+			timeout = 5 * time.Second // default timeout
+		}
+		ctx, cancel := context.WithTimeout(context.Background(), timeout)
-		ctx, cancel := context.WithTimeout(context.Background(), time.Duration(c.options.Timeout)*time.Second)
 		if err := c.tlsHandshakeWithTimeout(conn, ctx); err == nil {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ctx, cancel := context.WithTimeout(context.Background(), time.Duration(c.options.Timeout)*time.Second)
if err := c.tlsHandshakeWithTimeout(conn, ctx); err == nil {
h1 := conn.GetHandshakeLog()
enumeratedCiphers = append(enumeratedCiphers, h1.ServerHello.CipherSuite.String())
}
cancel()
timeout := time.Duration(c.options.Timeout) * time.Second
if timeout == 0 {
timeout = 5 * time.Second // default timeout
}
ctx, cancel := context.WithTimeout(context.Background(), timeout)
if err := c.tlsHandshakeWithTimeout(conn, ctx); err == nil {
h1 := conn.GetHandshakeLog()
enumeratedCiphers = append(enumeratedCiphers, h1.ServerHello.CipherSuite.String())
}
cancel()
🤖 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 260 - 265, The cipher-enumeration loop
uses context.WithTimeout with c.options.Timeout which when zero makes the
context expire immediately; update the code around tlsHandshakeWithTimeout and
the block that builds ctx, cancel to guard for c.options.Timeout == 0 (same
pattern as ConnectWithOptions) by choosing a sensible default or skipping the
timeout logic so handshakes can run, then call tlsHandshakeWithTimeout(conn,
ctx) and cancel() as before; reference c.options.Timeout,
tlsHandshakeWithTimeout, conn.GetHandshakeLog and enumeratedCiphers to locate
and apply the guard.

_ = conn.Close() // also closes baseConn internally
}
return enumeratedCiphers, nil
Expand Down Expand Up @@ -323,17 +325,18 @@ 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 {
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
}
return err
}