Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Changes Requested - Found 3 critical security issues that must be addressed
1. CRITICAL: Cryptographically Insecure Random String Generation (webrtcserver.go:719-727)
The randomString() function uses time.Now().UnixNano() for generating supposedly unique client IDs. This is not cryptographically secure and predictable.
Location: endpoints/server/webrtcserver.go:719-727
Issue:
func randomString(n int) string {
const letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
b := make([]byte, n)
for i := range b {
b[i] = letters[time.Now().UnixNano()%int64(len(letters))]
time.Sleep(time.Nanosecond) // ensure different values
}
return string(b)
}Security Impact:
- Client IDs are predictable and can be guessed by attackers
- An attacker could hijack WebRTC sessions by predicting valid client IDs
- Session fixation attacks become possible
Fix Required:
Use crypto/rand for cryptographically secure randomness:
import "crypto/rand"
func randomString(n int) string {
const letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
b := make([]byte, n)
if _, err := rand.Read(b); err != nil {
panic(err) // or handle appropriately
}
for i := range b {
b[i] = letters[b[i]%byte(len(letters))]
}
return string(b)
}2. SECURITY: Unrestricted CORS on WebSocket (webrtcserver.go:100-102)
The WebSocket upgrader allows connections from any origin with a comment suggesting this is temporary.
Location: endpoints/server/webrtcserver.go:100-102
Issue:
CheckOrigin: func(r *http.Request) bool {
return true // Allow all origins for now; can be restricted in production
},Security Impact:
- Cross-Site WebSocket Hijacking (CSWH) attacks
- Malicious websites can establish WebSocket connections to your server on behalf of authenticated users
- Data exfiltration and unauthorized access
Fix Required:
Implement proper origin validation:
CheckOrigin: func(r *http.Request) bool {
origin := r.Header.Get("Origin")
// Add config for allowed origins
allowedOrigins := conf.AllowedOrigins // Add this to config
if len(allowedOrigins) == 0 {
return false // Deny all if not configured
}
for _, allowed := range allowedOrigins {
if origin == allowed {
return true
}
}
return false
},Add AllowedOrigins []string to WebRTCConfig struct and configuration file.
3. BUG: Missing sendCh Close in removeClient (webrtcserver.go:671-686)
The removeClient function closes closeCh but never closes the sendCh channel, causing a goroutine leak in clientWritePump.
Location: endpoints/server/webrtcserver.go:671-686
Issue:
func (w *WebRTCServer) removeClient(client *WebSocketClient) {
client.closeOnce.Do(func() {
close(client.closeCh) // closes closeCh
// sendCh is never closed!
w.clientsMu.Lock()
delete(w.clients, client.id)
w.clientsMu.Unlock()
w.clientCount.Add(-1)
if client.pc != nil {
_ = client.pc.Close()
}
log.Info("WebRTC WebSocket client disconnected: %s", client.id)
})
}Impact:
clientWritePumpmay hang waiting onsendCh- Goroutine leak on every client disconnection
- Memory leak over time
Fix Required:
Close the send channel before closing closeCh:
func (w *WebRTCServer) removeClient(client *WebSocketClient) {
client.closeOnce.Do(func() {
close(client.sendCh) // Close sendCh first
close(client.closeCh)
w.clientsMu.Lock()
delete(w.clients, client.id)
w.clientsMu.Unlock()
w.clientCount.Add(-1)
if client.pc != nil {
_ = client.pc.Close()
}
log.Info("WebRTC WebSocket client disconnected: %s", client.id)
})
}Additional Notes
Positive Aspects:
✅ The cryptographic bug fix in initiator.go and responder.go (chain key initialization) looks correct and necessary
✅ Good use of sync.Once for cleanup
✅ Proper mutex protection for shared state
✅ Connection limit enforcement
Minor Suggestions (Not Blocking):
- Consider adding rate limiting per client IP for WebSocket connections
- The
setupDataChannelandsetupClientDataChannelhave significant code duplication - consider refactoring - Missing error handling when calling
RecvPrecheckin webrtcserver.go:474 (error is silently discarded)
Please fix the 3 critical issues above before merging.
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1407 +/- ##
========================================
- Coverage 0.57% 0.55% -0.02%
========================================
Files 87 87
Lines 12523 12877 +354
========================================
Hits 72 72
- Misses 12445 12799 +354
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🚨 Changes Requested - Found 7 issue(s) that must be addressed
Critical Issues (Security & Bugs)
1. Race Condition on client.dc and client.pc ⚠️ HIGH PRIORITY
Files: endpoints/server/webrtcserver.go:362, 344, 533-537, 546-547
The client.dc and client.pc fields are accessed without synchronization from multiple goroutines:
handleOffergoroutine setsclient.pc = pc(line 344)OnDataChannelcallback setsclient.dc = dc(line 362)SendToClientDataChannelreadsclient.dcwithout lock (line 533-537)BroadcastToAllDataChannelsreadsclient.dcwithout lock (line 546-547)
While clientsMu protects the map access, it does NOT protect concurrent access to the client.dc and client.pc fields themselves. This creates a data race.
Fix: Add a mutex to WebSocketClient struct to protect dc and pc fields, or use atomic operations.
2. CORS Bypass Vulnerability 🔴 SECURITY
File: endpoints/server/webrtcserver.go:101-103
CheckOrigin: func(r *http.Request) bool {
return true // Allow all origins for now; can be restricted in production
},This accepts WebSocket connections from ANY origin, enabling Cross-Site WebSocket Hijacking (CSWSH) attacks. An attacker's website can establish WebSocket connections to the server and potentially:
- Perform unauthorized WebRTC signaling
- Create peer connections
- Send malicious data through data channels
Fix: Implement proper origin validation or remove this override to use the default same-origin policy.
3. Weak Random Number Generation 🟡 SECURITY
File: endpoints/server/webrtcserver.go:721-732
func randomString(n int) string {
b := make([]byte, n)
if _, err := rand.Read(b); err != nil {
return "0" // Returns constant on error
}
// ...
}When rand.Read() fails, the function returns "0", causing colliding client IDs. Multiple clients could get the same ID (e.g., 20260119120000-0), leading to:
- Map key collisions in
w.clients - Overwritten connections
- Data sent to wrong clients
Fix: Return an error or use a fallback like UUID instead of returning a constant.
4. Missing Packet Release on Error 🟡 MEMORY LEAK
File: endpoints/server/webrtcserver.go:475-479
_, _, err := w.us.device.RecvPrecheck(pkt)
if err != nil {
// discard if precheck failed
return
}When RecvPrecheck fails, the packet is not released back to the pool, causing memory leaks over time.
Fix: Add w.us.device.ReleasePoolPacket(pkt) before returning on error.
5. Double Close Race Condition 🟡 BUG
File: endpoints/server/webrtcserver.go:673-675, 695-701
In Stop(), you iterate over clients and call client.closeOnce.Do(). However, removeClient() also calls client.closeOnce.Do() and closes client.closeCh. If removeClient() is called concurrently (e.g., from connection state change), you have:
- Thread 1:
Stop()→ closescloseCh - Thread 2:
removeClient()→ closes already-closedcloseCh(panic)
The closeOnce prevents closing twice, but there's still a race where one goroutine checks the channel while another closes it.
Fix: Don't close closeCh in the loop; let removeClient() handle cleanup, or ensure proper synchronization.
6. Cryptographic Chain Key Reset Bug 🔴 CRITICAL
Files: nhp/core/initiator.go:128-142, nhp/core/responder.go:109-123
The PR moves chain hash and chain key initialization OUTSIDE the conditional block that checks if it's the first time. Before:
if mad.ciphers == nil {
// ... initialize ciphers ...
// init chain hash -> ChainHash0
mad.chainHash, err = NewHash(...)
// init chain key -> ChainKey0
mad.noise.MixKey(&mad.chainKey, ...)
}After:
if mad.ciphers == nil {
// ... initialize ciphers ...
}
// init chain hash -> ChainHash0 <- NOW RUNS EVERY TIME
mad.chainHash, err = NewHash(...)
// init chain key -> ChainKey0 <- NOW RUNS EVERY TIME
mad.noise.MixKey(&mad.chainKey, ...)This means the chain key is reset to the initial state on every packet, breaking the Noise Protocol's forward secrecy. Each packet should use the evolved chain key, not reset to ChainKey0.
Impact: This completely breaks the cryptographic protocol. All packets will use the same initial chain key instead of evolving it, eliminating forward secrecy and potentially allowing replay attacks.
Git History: Looking at commit 0361901, the message says "fix a bug in reusing empty intermediate chain key. All packages starts with initial key now." This suggests the author intentionally made this change, but it appears to be incorrect based on Noise Protocol requirements.
Fix: Revert this change. Chain keys should only be initialized once per connection, not per packet.
7. Missing nil Check in derivePacketParserData 🟡 BUG
File: nhp/core/initiator.go:179
After removing the chain initialization from derivePacketParserData, the function no longer initializes ppd.chainHash or ppd.chainKey, but the caller expects these to be set.
Fix: Either restore the initialization or ensure the caller handles this properly.
Minor Issues
8. Missing Error Handling
endpoints/server/webrtcserver.go:734:updatePeerRemoteAddr()returns*net.UDPAddrbut is never used (line 127)- Silent error ignoring with
_assignments throughout
9. Configuration Validation
The config changes don't validate that EnableWebSocket requires the HTTP server to be running. If HTTP is disabled but WebSocket is enabled, it will fail silently.
Recommendations
- Add unit tests for the WebSocket signaling flow
- Add integration tests for concurrent client connections
- Document the security implications of WebRTC in production deployments
- Consider rate limiting for WebSocket connections per IP
- Add authentication/authorization for WebSocket signaling endpoint
Summary
This PR requires significant changes before merging. The cryptographic chain key reset bug (issue #6) is particularly critical and will break the security of the entire protocol. The race conditions and CORS vulnerability also need immediate attention.
There was a problem hiding this comment.
🚨 Changes Requested - Found 3 critical issues that must be addressed
Critical Issues
1. SECURITY: Cryptographic Key State Corruption (nhp/core/initiator.go:128-142, nhp/core/responder.go:114-123)
The PR removes critical code that properly initializes chain hash and chain key states in the Noise Protocol implementation. This creates a severe security vulnerability:
In initiator.go (derivePacketParserData):
- Lines 179-191 (old code) initialized chain hash and chain key from scratch
- New code (line 179) only sets
noise.HashTypebut does not initialize chainHash or chainKey - This means the derived PacketParserData has uninitialized cryptographic state
In responder.go (deriveMsgAssemblerData):
- Lines 203-215 (old code) initialized chain hash and chain key from scratch
- New code (line 198) only sets
noise.HashTypebut does not initialize chainHash or chainKey - This means the derived MsgAssemblerData has uninitialized cryptographic state
Impact:
- Uninitialized cryptographic state can lead to nil pointer dereferences when these objects are used
- Even worse, if Go's zero values are used, the crypto will be completely broken - using zero keys/hashes instead of properly derived Noise Protocol state
- This violates the Noise Protocol Framework specification which requires proper state initialization
- This breaks the security guarantees of the Zero Trust architecture
The removed code was NOT redundant - the new code in createMsgAssemblerData (lines 133-142) and createPacketParserData (lines 114-123) only initializes these states for NEW transactions, but derivePacketParserData/deriveMsgAssemblerData are used to continue EXISTING transactions where the crypto state must be re-initialized from the beginning of the handshake.
Required Fix:
Restore the chain hash and chain key initialization in both derivePacketParserData and deriveMsgAssemblerData functions. The initialization must happen in ALL code paths, not just the initial creation path.
2. BUG: Missing Packet Validation in WebRTC Data Channel (endpoints/server/webrtcserver.go:627)
In the setupDataChannel function's OnMessage handler, the old code (line 628 in the diff context) called:
_, _, err := w.us.device.RecvPrecheck(pkt)
if err != nil {
return // discard if precheck failed
}The new code removes this critical validation check entirely (only keeping the call at line 475 in setupClientDataChannel). This means:
- Packets from the legacy file-based signaling path are processed WITHOUT validation
- Malformed packets, replay attacks, and protocol violations will not be caught
- This bypasses the security checks that exist for UDP-based connections
Impact:
- Opens attack vectors for malformed packet injection
- Bypasses security validations that are present in the UDP code path
- Creates inconsistent security posture between WebSocket and file-based WebRTC signaling
Required Fix:
Restore the RecvPrecheck call and error handling at line 627 in the setupDataChannel function to match the setupClientDataChannel implementation.
3. SECURITY: WebSocket Origin Validation Disabled (endpoints/server/webrtcserver.go:101-103)
The WebSocket upgrader is configured with:
CheckOrigin: func(r *http.Request) bool {
return true // Allow all origins for now; can be restricted in production
},Impact:
- This allows Cross-Site WebSocket Hijacking (CSWSH) attacks
- Any malicious website can establish WebRTC connections to the NHP server from a victim's browser
- Combined with the Zero Trust architecture, this is especially dangerous as it bypasses expected security boundaries
- The comment "can be restricted in production" suggests this is a placeholder, but shipping it like this is dangerous
Required Fix:
Implement proper origin validation. At minimum:
- Add a configuration option for allowed origins (whitelist)
- Default to rejecting all cross-origin requests
- Document the security implications clearly
Minor Issues (Suggestions)
4. Stop() Race Condition (endpoints/server/webrtcserver.go:690-705)
The Stop() function closes stopCh, then iterates clients while holding the lock. However, client goroutines (clientReadPump/clientWritePump) may still be accessing client.closeCh. Consider closing client connections before closing stopCh to ensure clean shutdown.
5. Error Handling in randomString (endpoints/server/webrtcserver.go:724-726)
When rand.Read fails, the function returns "0" which could cause collision issues. Consider using a panic or better fallback (e.g., timestamp-based ID).
6. Dead Code Comment (endpoints/server/webrtcserver.go:725)
The commented //panic(err) should either be removed or properly handled.
Testing Recommendations
- Add unit tests for the cryptographic state initialization paths
- Add integration tests for WebRTC WebSocket signaling
- Test the security validations (origin checks, packet validation)
- Verify the Noise Protocol state machine still works correctly after the crypto changes
Summary: The cryptographic changes (issues #1 and #2) represent critical security bugs that will break the protocol's security guarantees. These must be fixed before merging.
There was a problem hiding this comment.
🚨 Changes Requested - Found 6 issue(s) that must be addressed
Thank you for implementing WebSocket signaling for WebRTC! The cryptographic chain key initialization fixes look correct. However, there are several critical issues that must be addressed before merging.
Critical Issues
1. Race Condition in Connection Limit (Security/Bug)
Location: endpoints/server/webrtcserver.go:186-211
Problem: TOCTOU race condition between checking clientCount (line 186) and incrementing it (line 211). Multiple concurrent connections can bypass the MaxConnections limit, leading to potential resource exhaustion DoS.
// VULNERABLE: Check and increment are not atomic
if int(w.clientCount.Load()) >= w.conf.MaxConnections { // Check
return
}
// Gap where multiple goroutines can pass through
wsConn, err := w.upgrader.Upgrade(rw, r, nil)
...
w.clientCount.Add(1) // Increment (too late)Fix: Use atomic compare-and-swap or move the increment before the upgrade:
// Increment first, decrement if upgrade fails
if w.clientCount.Add(1) > int32(w.conf.MaxConnections) {
w.clientCount.Add(-1)
http.Error(rw, "Maximum connections reached", http.StatusServiceUnavailable)
return
}2. Resource Leak in Stop() Method (Bug)
Location: endpoints/server/webrtcserver.go:690-713
Problem: The Stop() method doesn't close client.sendCh, only client.closeCh. This can cause clientWritePump goroutines to hang or panic.
// In Stop() - sendCh is NOT closed here
client.closeOnce.Do(func() {
close(client.closeCh) // Only this is closed
// missing: close(client.sendCh)
})Fix: Close sendCh in the Stop method:
client.closeOnce.Do(func() {
close(client.sendCh) // Add this
close(client.closeCh)
if client.pc != nil {
_ = client.pc.Close()
}
})3. Memory Leak on Packet Error Path (Bug)
Location: endpoints/server/webrtcserver.go:475-481
Problem: When RecvPrecheck fails, the allocated packet is not released back to the pool.
pkt := w.us.device.AllocatePoolPacket()
// ... packet setup ...
_, _, err := w.us.device.RecvPrecheck(pkt)
if err != nil {
return // LEAK: packet not released
}Fix: Release packet on error:
if err != nil {
w.us.device.ReleasePoolPacket(pkt)
return
}4. Weak Error Handling in Session ID Generation (Security)
Location: endpoints/server/webrtcserver.go:720-730
Problem: randomString returns "0" when crypto/rand fails, creating predictable/colliding session IDs.
if _, err := rand.Read(b); err != nil {
return "0" // Predictable fallback!
}Fix: Never silently fail crypto operations:
if _, err := rand.Read(b); err != nil {
panic(fmt.Sprintf("crypto/rand failed: %v", err))
}5. Unrestricted CORS Policy (Security)
Location: endpoints/server/webrtcserver.go:101-103
Problem: CheckOrigin: func(r *http.Request) bool { return true } allows Cross-Site WebSocket Hijacking attacks.
Fix: Implement origin validation or make it configurable:
CheckOrigin: func(r *http.Request) bool {
origin := r.Header.Get("Origin")
// Check against allowlist from config
return isOriginAllowed(origin)
},6. Potential Nil Pointer Dereference (Bug)
Location: endpoints/server/webrtcserver.go:429, 460, 486
Problem: *dc.ID() assumes DataChannel ID is never nil. While unlikely, defensive programming suggests checking.
Fix: Add nil check or document assumption:
dcID := dc.ID()
if dcID == nil {
log.Error("DataChannel ID is nil")
return
}
addr := &net.UDPAddr{IP: net.IPv4zero, Port: int(*dcID)}Positive Notes
✅ The cryptographic chain key initialization fixes in nhp/core/initiator.go and nhp/core/responder.go correctly ensure chain keys are always initialized
✅ Good use of sync.Once for cleanup
✅ Proper WaitGroup management for goroutine lifecycle
Please address the critical issues (1-5) before this can be merged. Let me know if you have questions!
There was a problem hiding this comment.
🚨 Changes Requested - Found 6 issue(s) that must be addressed
Thank you for implementing WebSocket signaling for WebRTC! The cryptographic chain key initialization fixes look correct. However, there are several critical issues that must be addressed before merging.
Critical Issues
1. Race Condition in Connection Limit (Security/Bug)
Location: endpoints/server/webrtcserver.go:186-211
Problem: TOCTOU race condition between checking clientCount (line 186) and incrementing it (line 211). Multiple concurrent connections can bypass the MaxConnections limit, leading to potential resource exhaustion DoS.
// VULNERABLE: Check and increment are not atomic
if int(w.clientCount.Load()) >= w.conf.MaxConnections { // Check
return
}
// Gap where multiple goroutines can pass through
wsConn, err := w.upgrader.Upgrade(rw, r, nil)
...
w.clientCount.Add(1) // Increment (too late)Fix: Use atomic compare-and-swap or move the increment before the upgrade:
// Increment first, decrement if upgrade fails
if w.clientCount.Add(1) > int32(w.conf.MaxConnections) {
w.clientCount.Add(-1)
http.Error(rw, "Maximum connections reached", http.StatusServiceUnavailable)
return
}2. Resource Leak in Stop() Method (Bug)
Location: endpoints/server/webrtcserver.go:690-713
Problem: The Stop() method doesn't close client.sendCh, only client.closeCh. This can cause clientWritePump goroutines to hang or panic.
// In Stop() - sendCh is NOT closed here
client.closeOnce.Do(func() {
close(client.closeCh) // Only this is closed
// missing: close(client.sendCh)
})Fix: Close sendCh in the Stop method:
client.closeOnce.Do(func() {
close(client.sendCh) // Add this
close(client.closeCh)
if client.pc != nil {
_ = client.pc.Close()
}
})3. Memory Leak on Packet Error Path (Bug)
Location: endpoints/server/webrtcserver.go:475-481
Problem: When RecvPrecheck fails, the allocated packet is not released back to the pool.
pkt := w.us.device.AllocatePoolPacket()
// ... packet setup ...
_, _, err := w.us.device.RecvPrecheck(pkt)
if err != nil {
return // LEAK: packet not released
}Fix: Release packet on error:
if err != nil {
w.us.device.ReleasePoolPacket(pkt)
return
}4. Weak Error Handling in Session ID Generation (Security)
Location: endpoints/server/webrtcserver.go:720-730
Problem: randomString returns "0" when crypto/rand fails, creating predictable/colliding session IDs.
if _, err := rand.Read(b); err != nil {
return "0" // Predictable fallback!
}Fix: Never silently fail crypto operations:
if _, err := rand.Read(b); err != nil {
panic(fmt.Sprintf("crypto/rand failed: %v", err))
}5. Unrestricted CORS Policy (Security)
Location: endpoints/server/webrtcserver.go:101-103
Problem: CheckOrigin: func(r *http.Request) bool { return true } allows Cross-Site WebSocket Hijacking attacks.
Fix: Implement origin validation or make it configurable:
CheckOrigin: func(r *http.Request) bool {
origin := r.Header.Get("Origin")
// Check against allowlist from config
return isOriginAllowed(origin)
},6. Potential Nil Pointer Dereference (Bug)
Location: endpoints/server/webrtcserver.go:429, 460, 486
Problem: *dc.ID() assumes DataChannel ID is never nil. While unlikely, defensive programming suggests checking.
Fix: Add nil check or document assumption:
dcID := dc.ID()
if dcID == nil {
log.Error("DataChannel ID is nil")
return
}
addr := &net.UDPAddr{IP: net.IPv4zero, Port: int(*dcID)}Positive Notes
✅ The cryptographic chain key initialization fixes in nhp/core/initiator.go and nhp/core/responder.go correctly ensure chain keys are always initialized
✅ Good use of sync.Once for cleanup
✅ Proper WaitGroup management for goroutine lifecycle
Please address the critical issues (1-5) before this can be merged. Let me know if you have questions!
2 participants
Summary
Brief description of changes.
Type of Change
Related Issues
Fixes #
Testing
Checklist