Prevent OOB read for RTCP XR block

[trengginas]

When receiving RTCP XR block, the packet will be parsed by using cast

switch (rb_hdr->bt) {
case BT_RR_TIME:
    rb_rr_time = (pjmedia_rtcp_xr_rb_rr_time*) rb_hdr;
    break;
case BT_DLRR:
    rb_dlrr = (pjmedia_rtcp_xr_rb_dlrr*) rb_hdr;
    break;
case BT_STATS:
    rb_stats = (pjmedia_rtcp_xr_rb_stats*) rb_hdr;
    break;
case BT_VOIP_METRICS:
    rb_voip_mtc = (pjmedia_rtcp_xr_rb_voip_mtc*) rb_hdr;
    break;
default:
    break;
}

Without checking the length, this can lead to out of bound read.
This ticket will prevent such issue by checking the packet length.

[sauwming]

This looks correct, but not easy to understand (I had to think really hard to follow the algorithm).

Why not compare it against the end packet buffer instead?
i.e.

pj_int32_t *pkt_end = (pj_int32_t*)pkt + pkt_len + 1;

if ((pj_int32_t *)rb_hdr + sizeof(something)/4 > pkt_end) break;

Or

if ((char *)rb_hdr + sizeof(something) > (char *)pkt + size) break;

[the-storm]

I am curious if this should have been a private PR since it is a security issue and wether it should have a GH security advisory attached to it with a CVE.

[the-storm]

I think the fix looks good. My concern here what happens if in the future a new type is added that's not divisible by 4?
Will a similar check be sufficient to prevent an OOB?
imagine a custom type with struct pjmedia_rtcp_xr_rb_some_custom_type that has sizeof() => 3

a check that's copy and paste from this check wont work. This concern also applies if any of these struct changes in size and doesn't become divisible by 4 anymore.

I think getting the end of the buffer, and check that the buffer + sizeof(*rb_rr_time) is within the bounds of the buffer should be more general and work hopefully right regardless of the struct size

Does this sound reasonable?

[the-storm]

nit: maybe you can declare this outside the loop?

[the-storm]

why this is pkt_len -1 and not pkt_len ?

[the-storm]

This looks good to me from security :)
I will defer approval from someone from the team who is more aware of the codebase :)

Thanks for fixing this @trengginas

[the-storm]

I am curious if this should have been a private PR since it is a security issue and wether it should have a GH security advisory attached to it with a CVE.

@trengginas any comments with regard to this?