Patchwork Bluetooth: Fix incorrect strncpy() in hidp_setup_hid()

login
register
mail settings
Submitter Luis Henriques
Date April 9, 2013, 10:52 a.m.
Message ID <1365504751-10093-1-git-send-email-luis.henriques@canonical.com>
Download mbox | patch
Permalink /patch/235035/
State New
Headers show

Comments

Luis Henriques - April 9, 2013, 10:52 a.m.
From: Anderson Lizardo <anderson.lizardo@openbossa.org>

CVE-2013-0349

BugLink: http://bugs.launchpad.net/bugs/1134503

The length parameter should be sizeof(req->name) - 1 because there is no
guarantee that string provided by userspace will contain the trailing
'\0'.

Can be easily reproduced by manually setting req->name to 128 non-zero
bytes prior to ioctl(HIDPCONNADD) and checking the device name setup on
input subsystem:

$ cat /sys/devices/pnp0/00\:04/tty/ttyS0/hci0/hci0\:1/input8/name
AAAAAA[...]AAAAAAAAf0:af:f0:af:f0:af

("f0:af:f0:af:f0:af" is the device bluetooth address, taken from "phys"
field in struct hid_device due to overflow.)

Cc: stable@vger.kernel.org
Signed-off-by: Anderson Lizardo <anderson.lizardo@openbossa.org>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
(back ported from commit 0a9ab9bdb3e891762553f667066190c1d22ad62b)

Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
---
 net/bluetooth/hidp/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Luis Henriques - April 9, 2013, 10:54 a.m.
Hit 'send' too soon... Forgot to specify this is for the Lucid kernel,
to fix CVE-2013-0349.

On Tue, Apr 09, 2013 at 11:52:31AM +0100, Luis Henriques wrote:
> From: Anderson Lizardo <anderson.lizardo@openbossa.org>
> 
> CVE-2013-0349
> 
> BugLink: http://bugs.launchpad.net/bugs/1134503
> 
> The length parameter should be sizeof(req->name) - 1 because there is no
> guarantee that string provided by userspace will contain the trailing
> '\0'.
> 
> Can be easily reproduced by manually setting req->name to 128 non-zero
> bytes prior to ioctl(HIDPCONNADD) and checking the device name setup on
> input subsystem:
> 
> $ cat /sys/devices/pnp0/00\:04/tty/ttyS0/hci0/hci0\:1/input8/name
> AAAAAA[...]AAAAAAAAf0:af:f0:af:f0:af
> 
> ("f0:af:f0:af:f0:af" is the device bluetooth address, taken from "phys"
> field in struct hid_device due to overflow.)
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Anderson Lizardo <anderson.lizardo@openbossa.org>
> Acked-by: Marcel Holtmann <marcel@holtmann.org>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> (back ported from commit 0a9ab9bdb3e891762553f667066190c1d22ad62b)
> 
> Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
> ---
>  net/bluetooth/hidp/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 49d8495..0c2c59d 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -778,7 +778,7 @@ static int hidp_setup_hid(struct hidp_session *session,
>  	hid->version = req->version;
>  	hid->country = req->country;
>  
> -	strncpy(hid->name, req->name, 128);
> +	strncpy(hid->name, req->name, sizeof(req->name) - 1);
>  	strncpy(hid->phys, batostr(&src), 64);
>  	strncpy(hid->uniq, batostr(&dst), 64);
>  
> -- 
> 1.8.1.2
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

Cheers,
--
Luis
Colin King - April 9, 2013, 11:06 a.m.
On 09/04/13 11:54, Luis Henriques wrote:
> Hit 'send' too soon... Forgot to specify this is for the Lucid kernel,
> to fix CVE-2013-0349.
> 
> On Tue, Apr 09, 2013 at 11:52:31AM +0100, Luis Henriques wrote:
>> From: Anderson Lizardo <anderson.lizardo@openbossa.org>
>>
>> CVE-2013-0349
>>
>> BugLink: http://bugs.launchpad.net/bugs/1134503
>>
>> The length parameter should be sizeof(req->name) - 1 because there is no
>> guarantee that string provided by userspace will contain the trailing
>> '\0'.
>>
>> Can be easily reproduced by manually setting req->name to 128 non-zero
>> bytes prior to ioctl(HIDPCONNADD) and checking the device name setup on
>> input subsystem:
>>
>> $ cat /sys/devices/pnp0/00\:04/tty/ttyS0/hci0/hci0\:1/input8/name
>> AAAAAA[...]AAAAAAAAf0:af:f0:af:f0:af
>>
>> ("f0:af:f0:af:f0:af" is the device bluetooth address, taken from "phys"
>> field in struct hid_device due to overflow.)
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Anderson Lizardo <anderson.lizardo@openbossa.org>
>> Acked-by: Marcel Holtmann <marcel@holtmann.org>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> (back ported from commit 0a9ab9bdb3e891762553f667066190c1d22ad62b)
>>
>> Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
>> ---
>>  net/bluetooth/hidp/core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
>> index 49d8495..0c2c59d 100644
>> --- a/net/bluetooth/hidp/core.c
>> +++ b/net/bluetooth/hidp/core.c
>> @@ -778,7 +778,7 @@ static int hidp_setup_hid(struct hidp_session *session,
>>  	hid->version = req->version;
>>  	hid->country = req->country;
>>  
>> -	strncpy(hid->name, req->name, 128);
>> +	strncpy(hid->name, req->name, sizeof(req->name) - 1);
>>  	strncpy(hid->phys, batostr(&src), 64);
>>  	strncpy(hid->uniq, batostr(&dst), 64);
>>  
>> -- 
>> 1.8.1.2
>>
>>
>> -- 
>> kernel-team mailing list
>> kernel-team@lists.ubuntu.com
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
> 
> Cheers,
> --
> Luis
> 
Simple one liner from upstream. looks fine to me.

Acked-by Colin Ian King <colin.king@canonical.com>
Andy Whitcroft - April 9, 2013, noon
On Tue, Apr 09, 2013 at 11:54:37AM +0100, Luis Henriques wrote:
> Hit 'send' too soon... Forgot to specify this is for the Lucid kernel,
> to fix CVE-2013-0349.
> 
> On Tue, Apr 09, 2013 at 11:52:31AM +0100, Luis Henriques wrote:
> > From: Anderson Lizardo <anderson.lizardo@openbossa.org>
> > 
> > CVE-2013-0349
> > 
> > BugLink: http://bugs.launchpad.net/bugs/1134503
> > 
> > The length parameter should be sizeof(req->name) - 1 because there is no
> > guarantee that string provided by userspace will contain the trailing
> > '\0'.
> > 
> > Can be easily reproduced by manually setting req->name to 128 non-zero
> > bytes prior to ioctl(HIDPCONNADD) and checking the device name setup on
> > input subsystem:
> > 
> > $ cat /sys/devices/pnp0/00\:04/tty/ttyS0/hci0/hci0\:1/input8/name
> > AAAAAA[...]AAAAAAAAf0:af:f0:af:f0:af
> > 
> > ("f0:af:f0:af:f0:af" is the device bluetooth address, taken from "phys"
> > field in struct hid_device due to overflow.)
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Anderson Lizardo <anderson.lizardo@openbossa.org>
> > Acked-by: Marcel Holtmann <marcel@holtmann.org>
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > (back ported from commit 0a9ab9bdb3e891762553f667066190c1d22ad62b)
> > 
> > Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
> > ---
> >  net/bluetooth/hidp/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> > index 49d8495..0c2c59d 100644
> > --- a/net/bluetooth/hidp/core.c
> > +++ b/net/bluetooth/hidp/core.c
> > @@ -778,7 +778,7 @@ static int hidp_setup_hid(struct hidp_session *session,
> >  	hid->version = req->version;
> >  	hid->country = req->country;
> >  
> > -	strncpy(hid->name, req->name, 128);
> > +	strncpy(hid->name, req->name, sizeof(req->name) - 1);
> >  	strncpy(hid->phys, batostr(&src), 64);
> >  	strncpy(hid->uniq, batostr(&dst), 64);
> >  
> > -- 
> > 1.8.1.2
> > 
> > 
> > -- 
> > kernel-team mailing list
> > kernel-team@lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team

Looks simple.  'name' is a char name[128] jobbie so this looks safe to
me.

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw
Andy Whitcroft - April 9, 2013, 12:06 p.m.
Applied to Lucid.

-apw

Patch

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 49d8495..0c2c59d 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -778,7 +778,7 @@  static int hidp_setup_hid(struct hidp_session *session,
 	hid->version = req->version;
 	hid->country = req->country;
 
-	strncpy(hid->name, req->name, 128);
+	strncpy(hid->name, req->name, sizeof(req->name) - 1);
 	strncpy(hid->phys, batostr(&src), 64);
 	strncpy(hid->uniq, batostr(&dst), 64);