Re: SDP payload processing vulnerability

"Glenn Durfee" <gdurfee@xxxxxxxxxx> · Mon, 16 Jun 2008 13:49:18 -0700

Oops, sent the wrong version, meant to send this one instead.

On Mon, Jun 16, 2008 at 1:27 PM, Glenn Durfee <gdurfee@xxxxxxxxxx> wrote:
> Hi all,
>
> The SDP parsing code blindly trusts string length fields in incoming
> SDP packets, exposing reliant applications to over-the-wireless memory
> manipulation attacks.   An attacker need only send a malformed
> response to an SDP query to take advantage of this.
>
> This is most apparent in file bluez-libs-3.30/src/sdp.c, lines 988,
> 994, 1002 (see below).  Also elsewhere in the code where input
> pointers are advanced without checking bytes remaining to be parsed.
> The root of the problem is that in bluez-libs-3.30/src/sdp.c:1125, the
> function sdp_extract_pdu() takes a buffer to parse (in) and a pointer
> to a length field (out), but it does not take an incoming length field
> (in).
>
> Attached is a patch to fix this issue.  Basically I added a
> "bytesleft" argument to all of the SDP payload processing routines;
> length fields are checked
> against the number of remaining bytes to ensure the parser doesn't run
> past the end of the packet, or do crazy things like malloc two gigs of
> memory.  This touches a lot of places, and changes the external API
> for SDP payload processing, but I don't see any other way to do this
> -- the parser MUST be aware of the incoming packet size in order for
> this to be secure.
>
> Glenn
>
> bluez-libs-3.30/src/sdp.c:
>
> 972 static sdp_data_t *extract_str(const void *p, int *len)
> 973 {
> 974        char *s;
> 975        int n;
> 976        sdp_data_t *d = malloc(sizeof(sdp_data_t));
> 977
> 978        memset(d, 0, sizeof(sdp_data_t));
> 979        d->dtd = *(uint8_t *) p;
> 980        p += sizeof(uint8_t);
> 981        *len += sizeof(uint8_t);
> 982
> 983        switch (d->dtd) {
> 984        case SDP_TEXT_STR8:
> 985        case SDP_URL_STR8:
> 986                n = *(uint8_t *) p;  // <-- from the incoming packet
> 987                p += sizeof(uint8_t);
> 988                *len += sizeof(uint8_t) + n;  // <-- blindly
> trusted here, may advance parser past end of packet
> 989                break;
> 990        case SDP_TEXT_STR16:
> 991        case SDP_URL_STR16:
> 992                n = ntohs(bt_get_unaligned((uint16_t *) p));  //
> <-- from the incoming packet
> 993                p += sizeof(uint16_t);
> 994                *len += sizeof(uint16_t) + n;  // <-- blindly
> trusted here, may advance parser past end of packet
> 995                break;
> 996        default:
> 997                SDPERR("Sizeof text string > UINT16_MAX\n");
> 998                free(d);
> 999                return 0;
> 1000        }
> 1001
> 1002        s = malloc(n + 1);  // <-- really blindly trusted here,
> also no NULL checking
> 1003        memset(s, 0, n + 1);
> 1004        memcpy(s, p, n);
> 1005
> 1006        SDPDBG("Len : %d\n", n);
> 1007        SDPDBG("Str : %s\n", s);
> 1008
> 1009        d->val.str = s;
> 1010        d->unitSize = n + sizeof(uint8_t);  // <-- more blind trust
> 1011        return d;
> 1012 }
>