Coverity: Untrusted divisor#870
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a Coverity static analysis report (CID 572837) regarding an untrusted divisor by modifying how the ato32() function converts bytes to 32-bit integers. The function now uses explicit masking and incremental shifting to prevent taint propagation in static analysis tools.
Changes:
- Rewrote
ato32()to use explicit byte masking and sequential left-shifts instead of direct bit-shifting - Simplified
DoKexDhInit()by replacing manualGetUint32+ validation withGetStringRefcall - Changed variable
etoconst byte*to matchGetStringRefsignature
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/misc.c | Refactored ato32() to use explicit masking and incremental shifts to satisfy static analysis |
| src/internal.c | Replaced manual string extraction and validation with GetStringRef helper function; added const qualifier |
Comments suppressed due to low confidence (1)
src/internal.c:4787
- When eSz exceeds the buffer size, the code should return an error instead of continuing execution. Currently, if the condition on line 4779 is false, the data is not copied to ssh->handshake->e, but the code continues to set CLIENT_KEXDH_INIT_DONE and calls SendKexDhReply with potentially uninitialized or stale data. This could lead to security vulnerabilities or undefined behavior. Add an else branch that returns an appropriate error code such as WS_BUFFER_E or WS_RECV_OVERFLOW_E.
if (ret == WS_SUCCESS) {
if (eSz <= (word32)sizeof(ssh->handshake->e)) {
WMEMCPY(ssh->handshake->e, e, eSz);
ssh->handshake->eSz = eSz;
}
ssh->clientState = CLIENT_KEXDH_INIT_DONE;
*idx = begin;
ret = SendKexDhReply(ssh);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8461e9d to
3001678
Compare
1. The individual bytes of the value read by ato32() are promoted to int values. Added typecasts to word32 for each of the bytes of the 32-bit value so they are treated as unsigned values like the target type. Also shifted each byte separately after masking them and then oring them into a temp. 2. To get the e value from the KexDhInit message, use the GetStringRef() function. 3. Add bounds checking of eSz. Fixes CID: 572837
3001678 to
3f8a1b8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Coverity report of an untrusted divisor. It goes back to the function ato32(), assuming that the value read is a size (which it is) and that it isn't appropriately checked. It is checked when used, but it is already tainted at that point, and the taint spread to other things. Changed ato32() to mask more and shift in a different manner.
Fixes CID: 572837