[PATCH] add some length verification to avoid reading not owned memory

[PATCH] add some length verification to avoid reading not owned memory

[Jens Rehsack]

[-- Attachment #1: Type: text/plain, Size: 186 bytes --]

Hi,

while reading mmsd sources I stumbled over missing length
checks in src/push.c:mms_push_notify(). I didn't re-read
the entire source to prove overall ;)

Best regards,
Jens

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-add-some-length-verification-to-avoid-reading-not-ow.patch --]
[-- Type: text/x-patch, Size: 1350 bytes --]

>From 7fe33082555f43d6eb52d2bebb0a9c36f0bc4adc Mon Sep 17 00:00:00 2001
From: Jens Rehsack <jr_extern@vfnet.de>
Date: Mon, 27 Feb 2012 10:15:43 +0100
Subject: [PATCH] add some length verification to avoid reading not owned
 memory

---
 src/push.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/src/push.c b/src/push.c
index 6a54907..6107352 100644
--- a/src/push.c
+++ b/src/push.c
@@ -351,13 +351,16 @@ gboolean mms_push_notify(unsigned char *pdu, unsigned int len,
 	/* Consume TID and Type */
 	nread = 2;
 
-	if (wsp_decode_uintvar(pdu + nread, len,
+	if (wsp_decode_uintvar(pdu + nread, len - nread,
 					&headerslen, &consumed) == FALSE)
 		return FALSE;
 
 	/* Consume uintvar bytes */
 	nread += consumed;
 
+	/* Check if content type could be read */
+	if (headerslen > (len - nread))
+		return FALSE;
 	/* Try to decode content-type */
 	if (wsp_decode_content_type(pdu + nread, headerslen, &ct,
 			&consumed, &param_len) == FALSE)
@@ -370,6 +373,9 @@ gboolean mms_push_notify(unsigned char *pdu, unsigned int len,
 	consumed += param_len;
 	nread += consumed;
 
+	/* Check if application_id could be read */
+	if ((headerslen - consumed) > (len - nread))
+		return FALSE;
 	/* Parse header to decode application_id */
 	wsp_header_iter_init(&iter, pdu + nread, headerslen - consumed, 0);
 
-- 
1.7.9.1

Re: [PATCH] add some length verification to avoid reading not owned memory

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

Hi Jens,

> while reading mmsd sources I stumbled over missing length
> checks in src/push.c:mms_push_notify(). I didn't re-read
> the entire source to prove overall ;)

please use git format-patch and git send-email to send us inline
patches. In case your email client doesn't support inline patches
nicely. Reviewing patches that are attached is super painful and in most
cases they get ignored then.

Regards

Marcel

Re: [PATCH] add some length verification to avoid reading not owned memory

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 835 bytes --]

Hi Jens,

> Well, my mail client (Thunderbird) and most modern clients I
> know are able to show attached files of text/x-patch (and
> similar) type. OTOH I wonder why you want to review patches
> in your mail client. Wouldn't it much better to use a

It is not that we read patches in the mail client, but that is also
useful.  The main use-case is to be able to easily comment on the patch
if changes are required.  And for that we require patches to be sent inline.

> specialized too which highlights the changes like
> $ gvim "+vert diffpatch <patchfile>" <originalfile>
> $ emacs
> or
> $ xfdiff ?
> Don't you have to save the patch files in such cases anyway?
> 

No, the typical workflow (at least for me) is to

- 'Save Email'
- vim email
- if looks okay, git am and compile test

Regards,
-Denis

Re: [PATCH] add some length verification to avoid reading not owned memory

[Jens Rehsack]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

Am 27.02.2012 18:22, schrieb Marcel Holtmann:
> Hi Jens,

Hi Marcel,

>> while reading mmsd sources I stumbled over missing length
>> checks in src/push.c:mms_push_notify(). I didn't re-read
>> the entire source to prove overall ;)
> 
> please use git format-patch and git send-email to send us inline
> patches. In case your email client doesn't support inline patches
> nicely.

Well, inline patches aren't requested at
http://ofono.org/wiki/ofono-etiquette. You probably should do.

> Reviewing patches that are attached is super painful and in most
> cases they get ignored then.

Well, my mail client (Thunderbird) and most modern clients I
know are able to show attached files of text/x-patch (and
similar) type. OTOH I wonder why you want to review patches
in your mail client. Wouldn't it much better to use a
specialized too which highlights the changes like
$ gvim "+vert diffpatch <patchfile>" <originalfile>
$ emacs
or
$ xfdiff ?
Don't you have to save the patch files in such cases anyway?

Best regards,
Jens