[XS] Change in libosmo-netif[master]: coverity

Attention is currently required from: fixeria, pespin. neels has posted comments on this change by neels. ( https://gerrit.osmocom.org/c/libosmo-netif/+/37992?usp=email ) Change subject: coverity ...................................................................... Patch Set 1: (1 comment) File src/rtp.c: https://gerrit.osmocom.org/c/libosmo-netif/+/37992/comment/1f92160e_4e49a2a… : PS1, Line 120: payload_len = ((int)msg->len) - sizeof(struct rtp_hdr) - csrc_len; coverity says CID#216829 ```` rtpxh = (struct rtp_x_hdr *)payload; 4. tainted_return_value: Function ntohs returns tainted data. 5. lower_bounds: Casting narrower unsigned ntohs(rtpxh->length) to wider signed type int effectively tests its lower bound. 6. var_assign_var: Assigning: x_len = ntohs(rtpxh->length) * 4 + 4UL. Both are now tainted. 133 x_len = ntohs(rtpxh->length) * 4 + sizeof(struct rtp_x_hdr); 7. var_assign_var: Compound assignment involving tainted variable x_len to variable payload taints payload. 134 payload += x_len; 8. var_assign_var: Compound assignment involving tainted variable x_len to variable payload_len taints payload_len. 135 payload_len -= x_len; 9. Condition payload_len < 0, taking false branch. 10. lower_bounds: Checking lower bounds of signed scalar payload_len by taking the false branch of payload_len < 0. 136 if (payload_len < 0) { 137 DEBUGPC(DLMUX, "received RTP frame too short, " 138 "extension header exceeds frame length\n"); 139 return NULL; 140 } 141 } 11. Condition rtph->padding, taking true branch. 142 if (rtph->padding) { 12. Condition payload_len < 0, taking false branch. 13. lower_bounds: Checking lower bounds of signed scalar payload_len by taking the false branch of payload_len < 0. 143 if (payload_len < 0) { 144 DEBUGPC(DLMUX, "received RTP frame too short for " 145 "padding length\n"); 146 return NULL; 147 } CID 216829: (#1 of 1): Untrusted pointer read (TAINTED_SCALAR) 14. tainted_data: Using tainted variable payload_len - 1 as an index to pointer payload. Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range. ```` The code does look sane, but indeed I find it a bit hard to get a grip on whether all necessary range checks on values from the wire are in place. Maybe with less complex range checking, more like 'if (a < b)' instead of a subtraction operation with unsigned types, coverity will also pick up on it? (Or there is an actual bug that coverity sees.) TL;DR let's make it trivial "if (a < b)" checks, do you agree? -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/37992?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I30beeac45ff2d8c08905986af9fabfda071ddc5b Gerrit-Change-Number: 37992 Gerrit-PatchSet: 1 Gerrit-Owner: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-CC: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Comment-Date: Wed, 04 Sep 2024 00:42:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels &lt;nhofmeyr(a)sysmocom.de&gt; Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt;