BlueZ

Email Archive: bluez-devel (read-only)

[Bluez-devel] SDP payload processing vulnerability

From: Glenn Durfee <gdurfee@go...> - 2008-06-16 20:27
Attachments: bluetooth.patch

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 }

Thread View

Thread	Author	Date
[Bluez-devel] SDP payload processing vulnerability	Glenn Durfee <gdurfee@go...>

Re: [Bluez-devel] SDP payload processing vulnerability

From: Glenn Durfee <gdurfee@go...> - 2008-06-16 20:49

Attachments: bluetooth.patch.2

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

On Mon, Jun 16, 2008 at 1:27 PM, Glenn Durfee <gdurfee@go...> 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 }
>

Re: [Bluez-devel] SDP payload processing vulnerability

From: Marcel Holtmann <marcel@ho...> - 2008-06-16 20:55

Hi Glenn,

> 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.

the SDP part has always been the weak point in every Bluetooth stack.
Our server side it pretty good, but it seems the client one is really
bad and I must admit that I never looked into in that in details.

> 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.

Changing the API is really a problem here. We can't do that. At least
not that easily. We can extend the API with more safe calls and then
slowly move over the clients.

Regards

Marcel

Re: [Bluez-devel] SDP payload processing vulnerability

From: Glenn Durfee <gdurfee@go...> - 2008-06-16 21:21

Marcel,

Thank you for the quick reply.

> the SDP part has always been the weak point in every Bluetooth stack.
> Our server side it pretty good, but it seems the client one is really
> bad and I must admit that I never looked into in that in details.

I'm concerned that hcid is also vulnerable to this (which would make
it both a server and a client problem?).

> Changing the API is really a problem here. We can't do that. At least
> not that easily. We can extend the API with more safe calls and then
> slowly move over the clients.

That sounds good. I can write safe versions of the parsing routines
and move everything I can find over to the new API; old clients will
still work with the old (unsafe) API. This may present some
maintenance challenges since there will be 2x parsing code, but it is
better than leaving security holes everywhere.

Is bluez.org currently down? I can't seem to get at the latest version...

Glenn

On Mon, Jun 16, 2008 at 1:59 PM, Marcel Holtmann <marcel@ho...> wrote:
> Hi Glenn,
>
>> 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.
>
> the SDP part has always been the weak point in every Bluetooth stack.
> Our server side it pretty good, but it seems the client one is really
> bad and I must admit that I never looked into in that in details.
>
>> 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.
>
> Changing the API is really a problem here. We can't do that. At least
> not that easily. We can extend the API with more safe calls and then
> slowly move over the clients.
>
> Regards
>
> Marcel
>
>
>
> -------------------------------------------------------------------------
> Check out the new SourceForge.net Marketplace.
> It's the best place to buy or sell services for
> just about anything Open Source.
> http://sourceforge.net/services/buy/index.php
> _______________________________________________
> Bluez-devel mailing list
> Bluez-devel@li...
> https://lists.sourceforge.net/lists/listinfo/bluez-devel
>

Re: [Bluez-devel] SDP payload processing vulnerability

From: Marcel Holtmann <marcel@ho...> - 2008-06-16 21:33

Hi Glenn,

> > the SDP part has always been the weak point in every Bluetooth stack.
> > Our server side it pretty good, but it seems the client one is really
> > bad and I must admit that I never looked into in that in details.
>
> I'm concerned that hcid is also vulnerable to this (which would make
> it both a server and a client problem?).

in theory it is, but you have to trigger a SDP client transaction first
and it is almost impossible to do this remotely. Yes, I can think of
tricks on how to do that, but that is besides the point here.

> > Changing the API is really a problem here. We can't do that. At least
> > not that easily. We can extend the API with more safe calls and then
> > slowly move over the clients.
>
> That sounds good. I can write safe versions of the parsing routines
> and move everything I can find over to the new API; old clients will
> still work with the old (unsafe) API. This may present some
> maintenance challenges since there will be 2x parsing code, but it is
> better than leaving security holes everywhere.

You can have the old API call the new one with a NULL parameter and yes,
it might be confusing, but it is better than breaking the API.

Please use the BlueZ coding style when doing the patch and have small
pieces. I am not going to review the whole think as once. Please send
small updates. It is faster this way.

> Is bluez.org currently down? I can't seem to get at the latest version...

Yeah. The server lost its routing information. I am working on it.

Use the CVS at bluez.sf.net or the GIT clone at git.infradead.org since
patches against the last release are outdated.

Regards

Marcel

BlueZ

Email Archive: bluez-devel (read-only)

Thread View

About SourceForge

Find Software

Develop Software

Community

Help