Fix #819: sanitize CN to prevent hang#951
Fix #819: sanitize CN to prevent hang#951thanik123789-coder wants to merge 5 commits intoprojectdiscovery:mainfrom
Conversation
This PR fixes the indefinite hang issue (projectdiscovery#819) by sanitizing SubjectCN and IssuerCN length and encoding. /claim projectdiscovery#819
Analysis ReportFound: 2 critical | 1 low/info CriticalDoS (Unchanged Code) -
DoS (Unchanged Code) -
Low / InfoFixed Code Path - Prompt for AI AgentsFix dos (unchanged code):
Fix dos (unchanged code):
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a private helper Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/tlsx/clients/utils.go (1)
156-161: Consider rune-safe truncation to avoid splitting multi-byte characters.Using
len(s)ands[:256]operates on bytes, not characters. If a multi-byte UTF-8 character spans the 256-byte boundary, slicing will create an invalid sequence thatToValidUTF8then removes. This could unintentionally drop a character that started within the limit.♻️ Proposed rune-aware truncation
func sanitizeCN(s string) string { + const maxLen = 256 - if len(s) > 256 { - s = s[:256] + s = strings.ToValidUTF8(s, "") + if len(s) > maxLen { + // Truncate at rune boundary + truncated := []rune(s) + if len(truncated) > maxLen { + s = string(truncated[:maxLen]) + } } - return strings.ToValidUTF8(s, "") + return s }Alternatively, if byte-length is the hard constraint (e.g., for downstream buffer limits), the current approach is acceptable—just be aware of the edge case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tlsx/clients/utils.go` around lines 156 - 161, sanitizeCN uses byte-based len and slicing which can split multi-byte UTF-8 runes; change sanitizeCN to perform rune-aware truncation: convert s to []rune, if len(runes) > 256 set runes = runes[:256], then return string(runes) (and you can still pass through strings.ToValidUTF8 if desired). Update the function sanitizeCN to use the rune-slicing approach instead of s[:256] to avoid dropping or corrupting multi-byte characters while preserving the existing trimming behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/tlsx/clients/utils.go`:
- Around line 156-161: sanitizeCN uses byte-based len and slicing which can
split multi-byte UTF-8 runes; change sanitizeCN to perform rune-aware
truncation: convert s to []rune, if len(runes) > 256 set runes = runes[:256],
then return string(runes) (and you can still pass through strings.ToValidUTF8 if
desired). Update the function sanitizeCN to use the rune-slicing approach
instead of s[:256] to avoid dropping or corrupting multi-byte characters while
preserving the existing trimming behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d94e61d9-2576-4d7b-9a04-d02e2528bde5
📒 Files selected for processing (1)
pkg/tlsx/clients/utils.go
Updated sanitizeCN to use rune-aware truncation to prevent multi-byte character corruption as suggested by CodeRabbit.
![neo-by-projectdiscovery-dev[bot]](https://avatars.githubusercontent.com/in/2878468?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/tlsx/clients/utils.go`:
- Around line 31-33: The code seeds domainNames with the raw
cert.Subject.CommonName so downstream checks process attacker-controlled input;
fix by applying sanitizeCN to cert.Subject.CommonName before any use (e.g., when
populating domainNames) and propagate that sanitized value to all callers
instead of the raw field, ensuring IsMisMatchedCert and IsWildCardCert receive
the sanitized name; repeat the same change in the ZTLS builder and CT log
builder code paths so every place that currently reads cert.Subject.CommonName
uses sanitizeCN(cert.Subject.CommonName) (or a local sanitizedSubjectCN
variable) before further processing.
- Around line 156-162: The sanitizeCN function currently does []rune(s) which
decodes the entire input at once and can be exploited with large
attacker-controlled CNs; replace that eager decode with a bounded decode loop
using utf8.DecodeRuneInString to read at most 256 runes, appending each decoded
rune to a buffer/string builder and breaking once 256 runes are collected (thus
avoiding scanning the rest), then pass the resulting truncated string to
strings.ToValidUTF8 before returning; reference sanitizeCN and use
utf8.DecodeRuneInString and strings.ToValidUTF8 in the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c06ac77-b623-4d2a-bcb6-22d40a8a9ec1
📒 Files selected for processing (1)
pkg/tlsx/clients/utils.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/tlsx/clients/utils.go (1)
19-22:⚠️ Potential issue | 🔴 CriticalUse the sanitized CN variables everywhere in this builder.
domainNamesis still seeded fromcert.Subject.CommonName, soIsMisMatchedCertandIsWildCardCertcontinue to process attacker-controlled input. Also,subjectCNandissuerCNare currently unused, which is a compile-time error in Go.Proposed fix
- subjectCN := sanitizeCN(cert.Subject.CommonName) + subjectCN := sanitizeCN(cert.Subject.CommonName) issuerCN := sanitizeCN(cert.Issuer.CommonName) - - domainNames := []string{cert.Subject.CommonName} + + domainNames := []string{subjectCN} domainNames = append(domainNames, cert.DNSNames...) @@ - IssuerCN: sanitizeCN(cert.Issuer.CommonName), + IssuerCN: issuerCN, @@ - SubjectCN: sanitizeCN(cert.Subject.CommonName), + SubjectCN: subjectCN,#!/bin/bash # Verify that the sanitized locals are only declared once and that domainNames # still starts from the raw CommonName in the current patch. sed -n '18,40p' pkg/tlsx/clients/utils.go rg -nP '\b(subjectCN|issuerCN)\b' pkg/tlsx/clients/utils.go rg -n 'domainNames := \[]string\{cert\.Subject\.CommonName\}' pkg/tlsx/clients/utils.goAlso applies to: 34-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tlsx/clients/utils.go` around lines 19 - 22, The cert CommonName is sanitized into subjectCN and issuerCN but domainNames is still seeded from the raw cert.Subject.CommonName; update the builder so domainNames is initialized from the sanitized subjectCN (i.e. domainNames := []string{subjectCN}) so downstream functions like IsMisMatchedCert and IsWildCardCert only see sanitized input, and either use issuerCN where intended or remove the unused issuerCN variable to fix the compile error; ensure all references that should use the sanitized value (sanitizeCN, subjectCN, issuerCN, domainNames, IsMisMatchedCert, IsWildCardCert) are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/tlsx/clients/utils.go`:
- Around line 159-160: The function sanitizeCN uses a non-existent identifier
"strings.builder"; update it to use the correct exported type "strings.Builder"
in the sanitizeCN function (and any related uses) so the code compiles; locate
the sanitizeCN function and replace strings.builder with strings.Builder and
ensure any methods called on the builder match the exported API.
---
Duplicate comments:
In `@pkg/tlsx/clients/utils.go`:
- Around line 19-22: The cert CommonName is sanitized into subjectCN and
issuerCN but domainNames is still seeded from the raw cert.Subject.CommonName;
update the builder so domainNames is initialized from the sanitized subjectCN
(i.e. domainNames := []string{subjectCN}) so downstream functions like
IsMisMatchedCert and IsWildCardCert only see sanitized input, and either use
issuerCN where intended or remove the unused issuerCN variable to fix the
compile error; ensure all references that should use the sanitized value
(sanitizeCN, subjectCN, issuerCN, domainNames, IsMisMatchedCert, IsWildCardCert)
are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be0975c7-9bb2-4b43-9f6c-8efda5f0b292
📒 Files selected for processing (1)
pkg/tlsx/clients/utils.go
1 participant
This PR fixes the indefinite hang issue (#819) by sanitizing SubjectCN and IssuerCN length and encoding. /claim #819
Summary by CodeRabbit