[PATCH BlueZ 0/3] SDP library invalid memory access fixes

[PATCH BlueZ 0/3] SDP library invalid memory access fixes

[Anderson Lizardo]

Hi,

This small set of patches fixes a couple of invalid memory reads/writes
detected by code inspection and confirmed by emulating invalid PDUs.

BTW, I have been silently working for some time on a tool now called "Blueish"
(variant of "bluish", meaning "somewhat blue"). It is fully written in Python
and allows to "easily" generate automated standalone test scripts (that also
only require Python + D-Bus/GLib bindings) for testing scenarios difficult on
real hardware. It uses VHCI for emulation.

For documentation and code, see: https://github.com/lizardo/blueish

The repository contains example data files for the latest patches I sent a
while ago (and these ones).

I tried to make it easy to use by adopting YAML for HCI packet construction.
Still, I'm aware that constructing HCI packets by hand is error prone, so I
plan (someday) to have a nice GUI and even some sort of visualization for the
packets (message sequence charts, maybe?).

That said, I'm still interested on helping with improving unit tests for BlueZ
(specially code not touched for a while). I just could not come up with a nice
way to integrate SDP client unit tests with the current server ones without too
much code duplication.

Best Regards,

Anderson Lizardo (3):
  lib: Fix buffer overflow when processing SDP response
  lib: Add range check for SDP_SVC_ATTR_RSP/SDP_SVC_SEARCH_ATTR_RSP
  lib: Check if SDP buffer has enough data on partial responses

 lib/sdp.c |   26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

-- 
1.7.9.5

[PATCH BlueZ 1/3] lib: Fix buffer overflow when processing SDP response

[Anderson Lizardo]

rsp_count is either read or calculated from untrusted input, and
therefore needs to be checked before being used as offset. The "plen"
variable is appropriate because it is calculated as the sum of fixed and
variable length fields, excluding the continuation state field, which
has at least 1 byte for its own length field.
---
 lib/sdp.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lib/sdp.c b/lib/sdp.c
index b87f392..f3a0c17 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -4189,6 +4189,17 @@ int sdp_process(sdp_session_t *session)
 		goto end;
 	}
 
+	/*
+	 * Out of bound check before using rsp_count as offset for continuation
+	 * state, which has at least a one byte size field.
+	 */
+	if ((n - (int) sizeof(sdp_pdu_hdr_t)) < plen + 1) {
+		t->err = EPROTO;
+		SDPERR("Protocol error: invalid PDU size");
+		status = SDP_INVALID_PDU_SIZE;
+		goto end;
+	}
+
 	pcstate = (sdp_cstate_t *) (pdata + rsp_count);
 
 	SDPDBG("Cstate length : %d\n", pcstate->length);
-- 
1.7.9.5

[PATCH BlueZ 2/3] lib: Add range check for SDP_SVC_ATTR_RSP/SDP_SVC_SEARCH_ATTR_RSP

[Anderson Lizardo]

According to SDP spec, the byte count fields for these PDUs have a valid
range of 0x0002-0xFFFF.
---
 lib/sdp.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/sdp.c b/lib/sdp.c
index f3a0c17..e212c29 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -4169,6 +4169,14 @@ int sdp_process(sdp_session_t *session)
 		rsp_count = bt_get_be16(pdata);
 		SDPDBG("Attrlist byte count : %d\n", rsp_count);
 
+		/* Valid range for rsp_count is 0x0002-0xFFFF */
+		if (rsp_count < 0x0002) {
+			t->err = EPROTO;
+			SDPERR("Protocol error: invalid AttrList size");
+			status = SDP_INVALID_PDU_SIZE;
+			goto end;
+		}
+
 		/*
 		 * Number of bytes in the AttributeLists parameter(without
 		 * continuation state) + AttributeListsByteCount field size.
-- 
1.7.9.5

[PATCH BlueZ 3/3] lib: Check if SDP buffer has enough data on partial responses

[Anderson Lizardo]

Before manipulating data from previous partial responses, make sure the
buffer has enough data.
---
 lib/sdp.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/sdp.c b/lib/sdp.c
index e212c29..3855381 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -4144,7 +4144,7 @@ int sdp_process(sdp_session_t *session)
 		if (t->rsp_concat_buf.data_size == 0) {
 			/* first fragment */
 			rsp_count = sizeof(tsrc) + sizeof(csrc) + csrc * 4;
-		} else {
+		} else if (t->rsp_concat_buf.data_size >= sizeof(uint16_t) * 2) {
 			/* point to the first csrc */
 			uint8_t *pcsrc = t->rsp_concat_buf.data + 2;
 			uint16_t tcsrc, tcsrc2;
@@ -4161,6 +4161,11 @@ int sdp_process(sdp_session_t *session)
 
 			pdata += sizeof(uint16_t); /* point to the first handle */
 			rsp_count = csrc * 4;
+		} else {
+			t->err = EPROTO;
+			SDPERR("Protocol error: invalid PDU size");
+			status = SDP_INVALID_PDU_SIZE;
+			goto end;
 		}
 		status = 0x0000;
 		break;
-- 
1.7.9.5

Re: [PATCH BlueZ 0/3] SDP library invalid memory access fixes

[Johan Hedberg]

Hi Lizardo,

On Sun, Feb 03, 2013, Anderson Lizardo wrote:
> This small set of patches fixes a couple of invalid memory reads/writes
> detected by code inspection and confirmed by emulating invalid PDUs.
> 
> BTW, I have been silently working for some time on a tool now called "Blueish"
> (variant of "bluish", meaning "somewhat blue"). It is fully written in Python
> and allows to "easily" generate automated standalone test scripts (that also
> only require Python + D-Bus/GLib bindings) for testing scenarios difficult on
> real hardware. It uses VHCI for emulation.
> 
> For documentation and code, see: https://github.com/lizardo/blueish
> 
> The repository contains example data files for the latest patches I sent a
> while ago (and these ones).
> 
> I tried to make it easy to use by adopting YAML for HCI packet construction.
> Still, I'm aware that constructing HCI packets by hand is error prone, so I
> plan (someday) to have a nice GUI and even some sort of visualization for the
> packets (message sequence charts, maybe?).
> 
> That said, I'm still interested on helping with improving unit tests for BlueZ
> (specially code not touched for a while). I just could not come up with a nice
> way to integrate SDP client unit tests with the current server ones without too
> much code duplication.
> 
> Best Regards,
> 
> Anderson Lizardo (3):
>   lib: Fix buffer overflow when processing SDP response
>   lib: Add range check for SDP_SVC_ATTR_RSP/SDP_SVC_SEARCH_ATTR_RSP
>   lib: Check if SDP buffer has enough data on partial responses
> 
>  lib/sdp.c |   26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)

All three patches have been applied. Thanks.

Johan