fix: auto-renew TLS certificates without restart#1337
Conversation
Incremental Review Status: 5ad1e09 → f94dfd9📊 Summary of ChangesThis commit includes refactoring and test improvements:
Files changed: 3 files (+20, -12) 🔍 Security Issues Status❌ Issue #1: TOCTOU vulnerability in cert_reloader.goStatus: STILL PRESENT (unchanged) Location: The refactoring extracted
An attacker with filesystem access can still replace cert files with symlinks between these operations. ❌ Issue #2: Missing TLS MinVersion in custom cert configStatus: STILL PRESENT (unchanged) Location: The ❌ Issue #3: Missing TLS MinVersion in ACME configStatus: STILL PRESENT (unchanged) Location: The ✅ New Vulnerabilities IntroducedNone detected. The changes are limited to code refactoring and test coverage improvements. 📝 RecommendationThe 3 previously reported security findings remain unaddressed. Consider implementing the suggested fixes:
The added test coverage in this commit is valuable for preventing regressions once these issues are fixed. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Split HandleWildcardCertificates into NewCertmagicConfig + per-domain cert obtaining so a single certmagic.Config is shared across all domains. The ACME loop now appends to domainCerts/certFiles instead of replacing them on each iteration, fixing multi-domain certificate handling.
![neo-by-projectdiscovery-dev[bot]](https://avatars.githubusercontent.com/in/2878468?s=60&v=4){kind=small}
| r.mu.Lock() | ||
| defer r.mu.Unlock() | ||
|
|
||
| mt, err := latestModTime(r.certPath, r.keyPath) |
There was a problem hiding this comment.
🟠 TOCTOU vulnerability in certificate reload mechanism (CWE-367) — The tryReload() function has a time-of-check-time-of-use (TOCTOU) race condition. It checks file modification time at line 81, then loads the certificate at line 90. An attacker with filesystem access could replace the certificate files with symlinks between these operations, causing the server to load certificates from an attacker-controlled location.
Attack Example
1. Wait for tryReload() to call os.Stat() at line 81
2. Replace cert.pem with symlink to /tmp/attacker-cert.pem between lines 81-90
3. Server loads attacker's certificate at line 90 via tls.LoadX509KeyPair()
Suggested Fix
Use file descriptors instead of paths: open files with O_NOFOLLOW flag, fstat() the fd, then read from the fd. This prevents symlink following and eliminates the TOCTOU window. Example: fd := unix.Open(path, unix.O_RDONLY|unix.O_NOFOLLOW, 0); defer unix.Close(fd); fstat(fd); read certificate from fd
| } else { | ||
| tlsConfig = acmeManagerTLS | ||
| go reloader.Start(context.Background()) | ||
| tlsConfig = &tls.Config{ |
There was a problem hiding this comment.
🟡 Missing TLS MinVersion allows downgrade to TLS 1.2 (CWE-327) — TLS configuration for custom certificates does not specify MinVersion, allowing clients to negotiate TLS 1.2. While Go 1.22+ defaults to TLS 1.2 minimum, explicitly setting TLS 1.3 provides defense-in-depth against future changes and protocol downgrade attacks.
Suggested Fix
Add MinVersion: tls.VersionTLS13 to the tls.Config at line 302:
tlsConfig = &tls.Config{
GetCertificate: reloader.GetCertificate,
NextProtos: []string{"h2", "http/1.1"},
MinVersion: tls.VersionTLS13,
}
| certFiles = append(certFiles, files...) | ||
| } | ||
| } | ||
| tlsConfig = &tls.Config{ |
There was a problem hiding this comment.
🟡 Missing TLS MinVersion in ACME configuration (CWE-327) — TLS configuration for ACME certificates does not specify MinVersion, allowing clients to negotiate TLS 1.2. While Go 1.22+ defaults to TLS 1.2 minimum, explicitly setting TLS 1.3 provides defense-in-depth.
Suggested Fix
Add MinVersion: tls.VersionTLS13 to the tls.Config at line 323:
tlsConfig = &tls.Config{
GetCertificate: cfg.GetCertificate,
NextProtos: []string{"h2", "http/1.1"},
MinVersion: tls.VersionTLS13,
}
Mzack9999
left a comment
Mzack9999
left a comment
There was a problem hiding this comment.
Marking with Request Changes to investigate needs of mutex
| } | ||
|
|
||
| func (r *CertReloader) tryReload() { | ||
| r.mu.Lock() |
There was a problem hiding this comment.
Isn't the tryReload() invoked sequentially without race condition already?
3 participants
Summary
GetCertificatecallback instead of statictls.Config.Certificatesso ACME-renewed certs are served dynamicallyCertReloaderfor custom cert files (--cert/--privkey) that polls for file changes and hot-swaps the certificateCERT_CHECK_INTERVALenv variable (default1h)Fixes #1332