| Message ID | 20190110062917.GB15047@kroah.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/2] Bluetooth: check message types in l2cap_get_conf_opt | expand |
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
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
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
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
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
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 &&
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(-)