Project

General

Profile

Actions

Bug #6296

closed

assertion on input received from RTP packet in libosmo-netif

Added by neels 5 months ago. Updated 5 months ago.

Status:
Rejected
Priority:
Normal
Assignee:
Target version:
-
Start date:
12/08/2023
Due date:
% Done:

0%

Spec Reference:

Description

osmo-mgw calls

osmo_amr_is_oa([AMR data from network])
osmo_amr_bytes(oa_hdr->ft)
OSMO_ASSERT(amr_ft < AMR_FT_MAX);

osmo-mgw rightfully passes network received data to that function, so osmo_amr_bytes() should not assert.

Actions #1

Updated by neels 5 months ago

  • Assignee set to pespin

git blames pespin =)

Actions #2

Updated by pespin 5 months ago

  • Status changed from New to Feedback
  • Assignee changed from pespin to neels

neels please provide a bit more feedback there. which value is being passed? in which scenario? can you provide a pcap?
Otherwise I'm totally out of scope here.

Actions #3

Updated by neels 5 months ago

everything is clear from the description.
do not OSMO_ASSERT(), but return an error.

IOW, if you send a specific erratic RTP packet to osmo-mgw, you can make it crash by assertion.
This is apparent from seeing that an OSMO_ASSERT() acts on external input data.
Prevent that in all cases.

Actions #4

Updated by pespin 5 months ago

#define AMR_FT_PDC_EFR_SID    11    /* PDC-EFR SID */
#define AMR_FT_NO_DATA            15    /* NO_DATA */
#define AMR_FT_MAX            16    /* INTERNAL, NO NOT USE OUTSIDE libosmo-netif */

// FT is 4 bits in struct amr_hdr *oa_hdr: "ft:4", hence range is 0..15

int osmo_amr_ft_valid(uint8_t amr_ft)
{
    return amr_ft <= AMR_FT_PDC_EFR_SID || amr_ft == AMR_FT_NO_DATA;
}

size_t osmo_amr_bytes(uint8_t amr_ft)
{
    OSMO_ASSERT(amr_ft < AMR_FT_MAX);
    return amr_ft_to_bytes[amr_ft];
}

bool osmo_amr_is_oa(const uint8_t *payload, unsigned int payload_len)
{
...
    /* Match the length of the received payload against the expected frame
     * length that is defined by the frame type. */
    if (!osmo_amr_ft_valid(oa_hdr->ft))
        return false;
----------------------------------- HERE oa_hdr->ft is "< 11 || == 15", so 0..11 || 15
    frame_len = osmo_amr_bytes(oa_hdr->ft);
    if (frame_len != payload_len - sizeof(struct amr_hdr))
        return false;

    return true;
}

osmo_amr_bytes() only asserts if amr_tr >= 16, which never happens in that code path...

Actions #5

Updated by neels 5 months ago

  • Status changed from Feedback to Rejected

ah i missed that! thanks!

Actions

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)