Patchwork [2/2] Bluetooth: check the buffer size for some messages before parsing

login
register
mail settings
Submitter Greg Kroah-Hartman
Date Jan. 10, 2019, 6:29 a.m.
Message ID <20190110062917.GB15047@kroah.com>
Download mbox | patch
Permalink /patch/696371/
State New
Headers show

Comments

Greg Kroah-Hartman - Jan. 10, 2019, 6:29 a.m.
The L2CAP_CONF_EFS and L2CAP_CONF_RFC messages can be sent from
userspace so their structure sizes need to be checked before parsing
them.

Based on a patch from Ran Menscher.

Reported-by: Ran Menscher <ran.menscher@karambasecurity.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 net/bluetooth/l2cap_core.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
Greg Kroah-Hartman - Jan. 10, 2019, 6:30 a.m.
On Thu, Jan 10, 2019 at 07:29:17AM +0100, Greg Kroah-Hartman wrote:
> The L2CAP_CONF_EFS and L2CAP_CONF_RFC messages can be sent from
> userspace so their structure sizes need to be checked before parsing
> them.
> 
> Based on a patch from Ran Menscher.

Ran, can you verify if these two patches solve the problems you reported
or not?

thanks,

greg k-h
Marcel Holtmann - Jan. 18, 2019, 9:37 a.m.
Hi Greg,

> The L2CAP_CONF_EFS and L2CAP_CONF_RFC messages can be sent from
> userspace so their structure sizes need to be checked before parsing
> them.

this message is confusing me. How can these be send from userspace?

> 
> Based on a patch from Ran Menscher.
> 
> Reported-by: Ran Menscher <ran.menscher@karambasecurity.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> net/bluetooth/l2cap_core.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 93daf94565cf..55e48e6efc2b 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -3361,7 +3361,8 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data
> 			break;
> 
> 		case L2CAP_CONF_RFC:
> -			if (olen == sizeof(rfc))
> +			if ((olen == sizeof(rfc)) &&
> +			    (endptr - ptr >= L2CAP_CONF_OPT_SIZE + sizeof(rfc)))
> 				memcpy(&rfc, (void *) val, olen);

We don’t do ((x == y) && (..)) actually. Using (x == y && ..) is plenty.

> 			break;
> 
> @@ -3371,7 +3372,8 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data
> 			break;
> 
> 		case L2CAP_CONF_EFS:
> -			if (olen == sizeof(efs)) {
> +			if ((olen == sizeof(efs)) &&
> +			    (endptr - ptr >= L2CAP_CONF_OPT_SIZE + sizeof(efs))) {
> 				remote_efs = 1;
> 				memcpy(&efs, (void *) val, olen);
> 			}
> @@ -3576,7 +3578,8 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len,
> 			break;
> 
> 		case L2CAP_CONF_RFC:
> -			if (olen == sizeof(rfc))
> +			if ((olen == sizeof(rfc)) &&
> +			    (endptr - ptr >= L2CAP_CONF_OPT_SIZE + sizeof(rfc)))
> 				memcpy(&rfc, (void *)val, olen);
> 
> 			if (test_bit(CONF_STATE2_DEVICE, &chan->conf_state) &&
> @@ -3596,7 +3599,8 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len,
> 			break;
> 
> 		case L2CAP_CONF_EFS:
> -			if (olen == sizeof(efs)) {
> +			if ((olen == sizeof(efs)) &&
> +			    (endptr - ptr >= L2CAP_CONF_OPT_SIZE + sizeof(efs))) {
> 				memcpy(&efs, (void *)val, olen);
> 
> 				if (chan->local_stype != L2CAP_SERV_NOTRAFIC &&

Regards

Marcel
Greg Kroah-Hartman - Jan. 18, 2019, 10:21 a.m.
On Fri, Jan 18, 2019 at 10:37:25AM +0100, Marcel Holtmann wrote:
> Hi Greg,
> 
> > The L2CAP_CONF_EFS and L2CAP_CONF_RFC messages can be sent from
> > userspace so their structure sizes need to be checked before parsing
> > them.
> 
> this message is confusing me. How can these be send from userspace?

So claimed the original reporter.  You have the information in your
inbox, is it incorrect?

> > 
> > Based on a patch from Ran Menscher.
> > 
> > Reported-by: Ran Menscher <ran.menscher@karambasecurity.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > net/bluetooth/l2cap_core.c | 12 ++++++++----
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 93daf94565cf..55e48e6efc2b 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -3361,7 +3361,8 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data
> > 			break;
> > 
> > 		case L2CAP_CONF_RFC:
> > -			if (olen == sizeof(rfc))
> > +			if ((olen == sizeof(rfc)) &&
> > +			    (endptr - ptr >= L2CAP_CONF_OPT_SIZE + sizeof(rfc)))
> > 				memcpy(&rfc, (void *) val, olen);
> 
> We don’t do ((x == y) && (..)) actually. Using (x == y && ..) is plenty.

Ick, ok, whatever, you all trust that your brains can remember C
priority levels, me, I trust ()...

I can fix this up to remove the extra (), but I would like _SOMEONE_ to
at least validate that this resolves the reported issues...

thanks,

greg k-h
Marcel Holtmann - Jan. 18, 2019, 11:11 a.m.
Hi Greg,

>>> The L2CAP_CONF_EFS and L2CAP_CONF_RFC messages can be sent from
>>> userspace so their structure sizes need to be checked before parsing
>>> them.
>> 
>> this message is confusing me. How can these be send from userspace?
> 
> So claimed the original reporter.  You have the information in your
> inbox, is it incorrect?

I am pretty sure he meant that the remote attacker can control it from userspace. This is still a wire protocol and not some socket options.

>>> 
>>> Based on a patch from Ran Menscher.
>>> 
>>> Reported-by: Ran Menscher <ran.menscher@karambasecurity.com>
>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> ---
>>> net/bluetooth/l2cap_core.c | 12 ++++++++----
>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>>> index 93daf94565cf..55e48e6efc2b 100644
>>> --- a/net/bluetooth/l2cap_core.c
>>> +++ b/net/bluetooth/l2cap_core.c
>>> @@ -3361,7 +3361,8 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data
>>> 			break;
>>> 
>>> 		case L2CAP_CONF_RFC:
>>> -			if (olen == sizeof(rfc))
>>> +			if ((olen == sizeof(rfc)) &&
>>> +			    (endptr - ptr >= L2CAP_CONF_OPT_SIZE + sizeof(rfc)))
>>> 				memcpy(&rfc, (void *) val, olen);
>> 
>> We don’t do ((x == y) && (..)) actually. Using (x == y && ..) is plenty.
> 
> Ick, ok, whatever, you all trust that your brains can remember C
> priority levels, me, I trust ()...
> 
> I can fix this up to remove the extra (), but I would like _SOMEONE_ to
> at least validate that this resolves the reported issues…

I need to reproduce this and then I can tell you.

Regards

Marcel
Marcel Holtmann - Jan. 18, 2019, 12:44 p.m.
Hi Greg,

>>>> The L2CAP_CONF_EFS and L2CAP_CONF_RFC messages can be sent from
>>>> userspace so their structure sizes need to be checked before parsing
>>>> them.
>>> 
>>> this message is confusing me. How can these be send from userspace?
>> 
>> So claimed the original reporter.  You have the information in your
>> inbox, is it incorrect?
> 
> I am pretty sure he meant that the remote attacker can control it from userspace. This is still a wire protocol and not some socket options.
> 
>>>> 
>>>> Based on a patch from Ran Menscher.
>>>> 
>>>> Reported-by: Ran Menscher <ran.menscher@karambasecurity.com>
>>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>> ---
>>>> net/bluetooth/l2cap_core.c | 12 ++++++++----
>>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>>> 
>>>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>>>> index 93daf94565cf..55e48e6efc2b 100644
>>>> --- a/net/bluetooth/l2cap_core.c
>>>> +++ b/net/bluetooth/l2cap_core.c
>>>> @@ -3361,7 +3361,8 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data
>>>> 			break;
>>>> 
>>>> 		case L2CAP_CONF_RFC:
>>>> -			if (olen == sizeof(rfc))
>>>> +			if ((olen == sizeof(rfc)) &&
>>>> +			    (endptr - ptr >= L2CAP_CONF_OPT_SIZE + sizeof(rfc)))
>>>> 				memcpy(&rfc, (void *) val, olen);
>>> 
>>> We don’t do ((x == y) && (..)) actually. Using (x == y && ..) is plenty.
>> 
>> Ick, ok, whatever, you all trust that your brains can remember C
>> priority levels, me, I trust ()...
>> 
>> I can fix this up to remove the extra (), but I would like _SOMEONE_ to
>> at least validate that this resolves the reported issues…
> 
> I need to reproduce this and then I can tell you.

so I think that just checking

	if (len < 0)
		break;

will just prevent any of these attacks. Since in theory you can also do this via the options, but then you can leak at max 2 octets.

I posted a simple patch for this. It would be however good if this gets verified that I understood the issues correctly.

Regards

Marcel

Patch

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 93daf94565cf..55e48e6efc2b 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3361,7 +3361,8 @@  static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data
 			break;
 
 		case L2CAP_CONF_RFC:
-			if (olen == sizeof(rfc))
+			if ((olen == sizeof(rfc)) &&
+			    (endptr - ptr >= L2CAP_CONF_OPT_SIZE + sizeof(rfc)))
 				memcpy(&rfc, (void *) val, olen);
 			break;
 
@@ -3371,7 +3372,8 @@  static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data
 			break;
 
 		case L2CAP_CONF_EFS:
-			if (olen == sizeof(efs)) {
+			if ((olen == sizeof(efs)) &&
+			    (endptr - ptr >= L2CAP_CONF_OPT_SIZE + sizeof(efs))) {
 				remote_efs = 1;
 				memcpy(&efs, (void *) val, olen);
 			}
@@ -3576,7 +3578,8 @@  static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len,
 			break;
 
 		case L2CAP_CONF_RFC:
-			if (olen == sizeof(rfc))
+			if ((olen == sizeof(rfc)) &&
+			    (endptr - ptr >= L2CAP_CONF_OPT_SIZE + sizeof(rfc)))
 				memcpy(&rfc, (void *)val, olen);
 
 			if (test_bit(CONF_STATE2_DEVICE, &chan->conf_state) &&
@@ -3596,7 +3599,8 @@  static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len,
 			break;
 
 		case L2CAP_CONF_EFS:
-			if (olen == sizeof(efs)) {
+			if ((olen == sizeof(efs)) &&
+			    (endptr - ptr >= L2CAP_CONF_OPT_SIZE + sizeof(efs))) {
 				memcpy(&efs, (void *)val, olen);
 
 				if (chan->local_stype != L2CAP_SERV_NOTRAFIC &&