Attention is currently required from: fixeria, pespin.
1 comment:
File src/rtp.c:
Patch Set #1, Line 120: payload_len = ((int)msg->len) - sizeof(struct rtp_hdr) - csrc_len;
I think I misunderstood what coverity was complaining about. […]
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 change 37992. To unsubscribe, or for help writing mail filters, visit settings.