Assorted minor security concerns - #2289
Conversation
This is a correctness issue that could allow certain kinds of data smuggling, and result in higher than expected data allocations, but did not necessarily result in protocol errors with well behaved clients. Unlikely to be anything that could be exploited remotely.
📝 WalkthroughWalkthroughThe changes add message-header validation, terminate abstract socket names, correct DTLS refresh encoding and decoding, preserve validated UDP lengths, prevent WebSocket control-flow fallthrough, guard closed listeners, and add regression coverage. ChangesProtocol and transport safety fixes
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/platform/posix/posix_ipclisten.c (1)
427-429: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNull termination is correct; comment claim about kernel name format warrants verification.
The explicit null termination at line 443 is safe: the bounds check at line 436 (
len <= sizeof(l->sa.s_abstract.sa_name)) combined with the two subtractions (lines 438–439) guaranteessa_name[len]is well within the buffer. The terminator is placed at indexsa_len, so it doesn't overwrite any name bytes or altersa_len—preserving length-based semantics for abstract names that may contain embedded NULs (as exercised bytest_abstract_nullinsrc/sp/transport/ipc/ipc_test.c).The expanded comment at lines 427–429 states the kernel-assigned name is "the leading NULL byte, followed by 5 ascii characters." The exact length and format of kernel-auto-assigned abstract socket names varies across Linux kernel versions (older kernels use PID/counter-based schemes, newer ones use random values). The code correctly handles this dynamically via
getsockname, but the "5 ascii characters" claim is overly specific and may be inaccurate on some kernels. Consider softening to "followed by a short sequence of ascii characters" or similar.Also applies to: 443-443
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/posix/posix_ipclisten.c` around lines 427 - 429, The comment describing kernel-assigned abstract socket names is overly specific and may be inaccurate across Linux versions. Update the comment near the abstract-name handling and explicit terminator assignment to describe a leading NUL followed by a short, kernel-defined sequence of characters, without asserting an exact five-character length; leave the bounds checks and null-termination logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/platform/posix/posix_ipclisten.c`:
- Around line 427-429: The comment describing kernel-assigned abstract socket
names is overly specific and may be inaccurate across Linux versions. Update the
comment near the abstract-name handling and explicit terminator assignment to
describe a leading NUL followed by a short, kernel-defined sequence of
characters, without asserting an exact five-character length; leave the bounds
checks and null-termination logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c65ccec-451b-4877-80d1-a38126535383
📒 Files selected for processing (5)
src/core/message.csrc/platform/posix/posix_ipclisten.csrc/sp/transport/dtls/dtls.csrc/sp/transport/udp/udp.csrc/supplemental/websocket/websocket.c
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2289 +/- ##
==========================================
+ Coverage 78.11% 83.47% +5.35%
==========================================
Files 93 99 +6
Lines 20782 22701 +1919
Branches 0 3280 +3280
==========================================
+ Hits 16234 18949 +2715
+ Misses 4548 3669 -879
- Partials 0 83 +83 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sp/transport/ws/websocket.c`:
- Around line 513-517: Update the l->closed branch in the accept completion flow
to route through the existing error path instead of freeing ws and returning
directly, ensuring uaio is removed and completed with NNG_ECLOSED. Preserve the
listener mutex handling and use the existing error cleanup logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3b9a9f5-b2e8-4dc8-993c-fa17284abe7b
📒 Files selected for processing (1)
src/sp/transport/ws/websocket.c
| if (l->closed) { | ||
| nng_stream_free(ws); | ||
| nni_mtx_unlock(&l->mtx); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Complete the pending accept AIO before returning.
When the underlying accept succeeds but listener shutdown wins the race, this branch frees ws and returns without removing or completing uaio. Since wstran_listener_close does not drain l->aios, callers can remain blocked indefinitely instead of receiving NNG_ECLOSED. Route this case through the existing error path.
Proposed fix
if (l->closed) {
nng_stream_free(ws);
- nni_mtx_unlock(&l->mtx);
- return;
+ rv = NNG_ECLOSED;
+ goto error;
}📝 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.
| if (l->closed) { | |
| nng_stream_free(ws); | |
| nni_mtx_unlock(&l->mtx); | |
| return; | |
| } | |
| if (l->closed) { | |
| nng_stream_free(ws); | |
| rv = NNG_ECLOSED; | |
| goto error; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sp/transport/ws/websocket.c` around lines 513 - 517, Update the l->closed
branch in the accept completion flow to route through the existing error path
instead of freeing ws and returning directly, ensuring uaio is removed and
completed with NNG_ECLOSED. Preserve the listener mutex handling and use the
existing error cleanup logic.
This fixes a few different minor (unlikely to be remotely exploitable) security concerns that were found during a comprehensive security audit of our code.
Summary by CodeRabbit