Project

General

Profile

Actions

Bug #5119

closed

mgcp_client.c should not assert on unexpected codec name in the input data

Added by neels almost 3 years ago. Updated almost 3 years ago.

Status:
Resolved
Priority:
High
Assignee:
Category:
-
Target version:
-
Start date:
04/18/2021
Due date:
% Done:

90%

Spec Reference:

Description

see https://git.osmocom.org/osmo-mgw/tree/src/libosmo-mgcp-client/mgcp_client.c?id=9ffaba7c1b0e3e44469e8d4c9e30dfe0e31f07f2#n1121

static int add_lco(struct msgb *msg, struct mgcp_msg *mgcp_msg)
{
    unsigned int i;
    int rc = 0;
    const char *codec;
    unsigned int pt;

    rc += msgb_printf(msg, "L:");

    if (mgcp_msg->ptime)
        rc += msgb_printf(msg, " p:%u,", mgcp_msg->ptime);

    if (mgcp_msg->codecs_len) {
        rc += msgb_printf(msg, " a:");
        for (i = 0; i < mgcp_msg->codecs_len; i++) {
            pt = mgcp_msg->codecs[i];
            codec = get_value_string_or_null(osmo_mgcpc_codec_names, pt);

            /* Note: Use codec descriptors from enum mgcp_codecs
             * in mgcp_client only! */
            OSMO_ASSERT(codec);                                        <------ HERE
            rc += msgb_printf(msg, "%s", extract_codec_name(codec));
            if (i < mgcp_msg->codecs_len - 1)
                rc += msgb_printf(msg, ";");
        }
        rc += msgb_printf(msg, ",");
    }

    rc += msgb_printf(msg, " nt:IN\r\n");

    return rc;
}

Results in

20210418153207176 DCHAN DEBUG lchan_rtp(0-0-1-TCH_F-0)[0x6120000105a0]{WAIT_MGW_ENDPOINT_AVAILABLE}: state_chg to WAIT_MGW_ENDPOINT_AVAILABLE (lchan_rtp_fsm.c:119)
20210418153207176 DRLL DEBUG MGCP_CONN(msc0-conn1_subscr-IMSI-001019876543210)[0x6120000102a0]{ST_CRCX}: Allocated (fsm.c:461)
20210418153207176 DRLL DEBUG MGCP_CONN(msc0-conn1_subscr-IMSI-001019876543210)[0x6120000102a0]{ST_CRCX}: is child of mgw-endp(msc0-conn1_subscr-IMSI-001019876543210)[0x612000010420] (fsm.c:491)
20210418153207176 DRLL DEBUG MGCP_CONN(msc0-conn1_subscr-IMSI-001019876543210)[0x6120000102a0]{ST_CRCX}: Received Event EV_CRCX (mgcp_client_fsm.c:636)
20210418153207176 DRLL DEBUG MGCP_CONN(msc0-conn1_subscr-IMSI-001019876543210)[0x6120000102a0]{ST_CRCX}: MGW/CRCX: creating connection on MGW endpoint:rtpbridge/*@mgw... (mgcp_client_fsm.c:221)
Assert failed codec ../../../../src/osmo-mgw/src/libosmo-mgcp-client/mgcp_client.c:1121
backtrace() returned 38 addresses

Apparently an incorrect codec string in the CRCX input data leads to an OsmoBSC crash.
OSMO_ASSERT() is the wrong error handling for input data.

The context of this is that I am working on the Channel Mode Modify code path in OsmoBSC.
It is probably a bug in that code path that passes a wrong codec name,
but still this should not crash the entire BSC.


Related issues

Related to OsmoMGW - Bug #5123: coredump nightly mgw on 3g voicecall startupResolvedroh04/20/2021

Actions
Actions #1

Updated by laforge almost 3 years ago

  • Related to Bug #5123: coredump nightly mgw on 3g voicecall startup added
Actions #2

Updated by laforge almost 3 years ago

  • Priority changed from Normal to High

In general, no matter what happens at a remote implementation that sends packets to us, we must never OSMO_ASSERT(). This is a serious problem. OSMO_ASSERT() is to guard against conditions entirely under control of our implementation (mgw in this case).

Any remote user, even a malicious one, must always be ble to send us anything without us running into OSMO_ASSERT(). If a remote user can trigger this, it's a denial of service vulnerability.

Actions #3

Updated by dexter almost 3 years ago

  • Status changed from New to In Progress
  • % Done changed from 0 to 90

I have replaced the ASSERT with a normal check that just returns an error code. The reason for the ASSERT might have been that the API user is expected to use only the constants from mgcp_client.h. However, it may still be that the input data is derived from some other source and not filled in by the API user manually. So errors in the input data are possible.

https://gerrit.osmocom.org/c/osmo-mgw/+/24182 mgcp_client: do not crash when lco/sdp can not be generated

Actions #4

Updated by dexter almost 3 years ago

  • Status changed from In Progress to Resolved

I see that the patch got merged so I think this ticket can be closed.

Actions

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)