| Message ID | 20190219122746.4197-2-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Trusty,1/2] Bluetooth: Prevent stack info leak from the EFS element. | expand |
On 2019-02-19 20:27:44, Kai-Heng Feng wrote: > This issue has been assigned CVE-2017-1000410 > > CVE-2019-3460 Thanks for working on this issue! Unfortunately, something isn't right here as the patch is for CVE-2017-1000410 but the Ubuntu CVE Tracker contains incorrect data about the fix for CVE-2017-1000410. Give me a little bit to straighten things out and then I'll have a recommendation about the correct metadata to include in this commit message. Tyler
On 2019-02-19 20:27:44, Kai-Heng Feng wrote: > From: Ben Seri <ben@armis.com> > > In the function l2cap_parse_conf_rsp and in the function > l2cap_parse_conf_req the following variable is declared without > initialization: > > struct l2cap_conf_efs efs; > > In addition, when parsing input configuration parameters in both of > these functions, the switch case for handling EFS elements may skip the > memcpy call that will write to the efs variable: > > ... > case L2CAP_CONF_EFS: > if (olen == sizeof(efs)) > memcpy(&efs, (void *)val, olen); > ... > > The olen in the above if is attacker controlled, and regardless of that > if, in both of these functions the efs variable would eventually be > added to the outgoing configuration request that is being built: > > l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs), (unsigned long) &efs); > > So by sending a configuration request, or response, that contains an > L2CAP_CONF_EFS element, but with an element length that is not > sizeof(efs) - the memcpy to the uninitialized efs variable can be > avoided, and the uninitialized variable would be returned to the > attacker (16 bytes). > > This issue has been assigned CVE-2017-1000410 > > CVE-2019-3460 CVE-2019-3460 should not be added to this commit message. The line above should be changed to 'CVE-2017-1000410' when committing this patch. With that modification, Acked-by: Tyler Hicks <tyhicks@canonical.com> Tyler > > Cc: Marcel Holtmann <marcel@holtmann.org> > Cc: Gustavo Padovan <gustavo@padovan.org> > Cc: Johan Hedberg <johan.hedberg@gmail.com> > Cc: stable <stable@vger.kernel.org> > Signed-off-by: Ben Seri <ben@armis.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > (cherry picked from commit 06e7e776ca4d36547e503279aeff996cbb292c16) > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > net/bluetooth/l2cap_core.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > index 5eb3b2b55f2e..61d0f290c0c6 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -3305,9 +3305,10 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data > break; > > case L2CAP_CONF_EFS: > - remote_efs = 1; > - if (olen == sizeof(efs)) > + if (olen == sizeof(efs)) { > + remote_efs = 1; > memcpy(&efs, (void *) val, olen); > + } > break; > > case L2CAP_CONF_EWS: > @@ -3526,16 +3527,17 @@ 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)) { > memcpy(&efs, (void *)val, olen); > > - if (chan->local_stype != L2CAP_SERV_NOTRAFIC && > - efs.stype != L2CAP_SERV_NOTRAFIC && > - efs.stype != chan->local_stype) > - return -ECONNREFUSED; > + if (chan->local_stype != L2CAP_SERV_NOTRAFIC && > + efs.stype != L2CAP_SERV_NOTRAFIC && > + efs.stype != chan->local_stype) > + return -ECONNREFUSED; > > - l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs), > - (unsigned long) &efs, endptr - ptr); > + l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs), > + (unsigned long) &efs, endptr - ptr); > + } > break; > > case L2CAP_CONF_FCS: > -- > 2.17.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 19.02.19 13:27, Kai-Heng Feng wrote: > From: Ben Seri <ben@armis.com> > > In the function l2cap_parse_conf_rsp and in the function > l2cap_parse_conf_req the following variable is declared without > initialization: > > struct l2cap_conf_efs efs; > > In addition, when parsing input configuration parameters in both of > these functions, the switch case for handling EFS elements may skip the > memcpy call that will write to the efs variable: > > ... > case L2CAP_CONF_EFS: > if (olen == sizeof(efs)) > memcpy(&efs, (void *)val, olen); > ... > > The olen in the above if is attacker controlled, and regardless of that > if, in both of these functions the efs variable would eventually be > added to the outgoing configuration request that is being built: > > l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs), (unsigned long) &efs); > > So by sending a configuration request, or response, that contains an > L2CAP_CONF_EFS element, but with an element length that is not > sizeof(efs) - the memcpy to the uninitialized efs variable can be > avoided, and the uninitialized variable would be returned to the > attacker (16 bytes). > > This issue has been assigned CVE-2017-1000410 > > CVE-2019-3460 > > Cc: Marcel Holtmann <marcel@holtmann.org> > Cc: Gustavo Padovan <gustavo@padovan.org> > Cc: Johan Hedberg <johan.hedberg@gmail.com> > Cc: stable <stable@vger.kernel.org> > Signed-off-by: Ben Seri <ben@armis.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > (cherry picked from commit 06e7e776ca4d36547e503279aeff996cbb292c16) > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- With CVE change mentioned by Tyler. > net/bluetooth/l2cap_core.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > index 5eb3b2b55f2e..61d0f290c0c6 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -3305,9 +3305,10 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data > break; > > case L2CAP_CONF_EFS: > - remote_efs = 1; > - if (olen == sizeof(efs)) > + if (olen == sizeof(efs)) { > + remote_efs = 1; > memcpy(&efs, (void *) val, olen); > + } > break; > > case L2CAP_CONF_EWS: > @@ -3526,16 +3527,17 @@ 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)) { > memcpy(&efs, (void *)val, olen); > > - if (chan->local_stype != L2CAP_SERV_NOTRAFIC && > - efs.stype != L2CAP_SERV_NOTRAFIC && > - efs.stype != chan->local_stype) > - return -ECONNREFUSED; > + if (chan->local_stype != L2CAP_SERV_NOTRAFIC && > + efs.stype != L2CAP_SERV_NOTRAFIC && > + efs.stype != chan->local_stype) > + return -ECONNREFUSED; > > - l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs), > - (unsigned long) &efs, endptr - ptr); > + l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs), > + (unsigned long) &efs, endptr - ptr); > + } > break; > > case L2CAP_CONF_FCS: >
On 2/19/19 1:27 PM, Kai-Heng Feng wrote: > From: Ben Seri <ben@armis.com> > > In the function l2cap_parse_conf_rsp and in the function > l2cap_parse_conf_req the following variable is declared without > initialization: > > struct l2cap_conf_efs efs; > > In addition, when parsing input configuration parameters in both of > these functions, the switch case for handling EFS elements may skip the > memcpy call that will write to the efs variable: > > ... > case L2CAP_CONF_EFS: > if (olen == sizeof(efs)) > memcpy(&efs, (void *)val, olen); > ... > > The olen in the above if is attacker controlled, and regardless of that > if, in both of these functions the efs variable would eventually be > added to the outgoing configuration request that is being built: > > l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs), (unsigned long) &efs); > > So by sending a configuration request, or response, that contains an > L2CAP_CONF_EFS element, but with an element length that is not > sizeof(efs) - the memcpy to the uninitialized efs variable can be > avoided, and the uninitialized variable would be returned to the > attacker (16 bytes). > > This issue has been assigned CVE-2017-1000410 > > CVE-2019-3460 Applied to trusty/master-next branch, changing the above line to: CVE-2017-1000410 Thanks, Kleber > > Cc: Marcel Holtmann <marcel@holtmann.org> > Cc: Gustavo Padovan <gustavo@padovan.org> > Cc: Johan Hedberg <johan.hedberg@gmail.com> > Cc: stable <stable@vger.kernel.org> > Signed-off-by: Ben Seri <ben@armis.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > (cherry picked from commit 06e7e776ca4d36547e503279aeff996cbb292c16) > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > net/bluetooth/l2cap_core.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > index 5eb3b2b55f2e..61d0f290c0c6 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -3305,9 +3305,10 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data > break; > > case L2CAP_CONF_EFS: > - remote_efs = 1; > - if (olen == sizeof(efs)) > + if (olen == sizeof(efs)) { > + remote_efs = 1; > memcpy(&efs, (void *) val, olen); > + } > break; > > case L2CAP_CONF_EWS: > @@ -3526,16 +3527,17 @@ 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)) { > memcpy(&efs, (void *)val, olen); > > - if (chan->local_stype != L2CAP_SERV_NOTRAFIC && > - efs.stype != L2CAP_SERV_NOTRAFIC && > - efs.stype != chan->local_stype) > - return -ECONNREFUSED; > + if (chan->local_stype != L2CAP_SERV_NOTRAFIC && > + efs.stype != L2CAP_SERV_NOTRAFIC && > + efs.stype != chan->local_stype) > + return -ECONNREFUSED; > > - l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs), > - (unsigned long) &efs, endptr - ptr); > + l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs), > + (unsigned long) &efs, endptr - ptr); > + } > break; > > case L2CAP_CONF_FCS:
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c index 5eb3b2b55f2e..61d0f290c0c6 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -3305,9 +3305,10 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data break; case L2CAP_CONF_EFS: - remote_efs = 1; - if (olen == sizeof(efs)) + if (olen == sizeof(efs)) { + remote_efs = 1; memcpy(&efs, (void *) val, olen); + } break; case L2CAP_CONF_EWS: @@ -3526,16 +3527,17 @@ 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)) { memcpy(&efs, (void *)val, olen); - if (chan->local_stype != L2CAP_SERV_NOTRAFIC && - efs.stype != L2CAP_SERV_NOTRAFIC && - efs.stype != chan->local_stype) - return -ECONNREFUSED; + if (chan->local_stype != L2CAP_SERV_NOTRAFIC && + efs.stype != L2CAP_SERV_NOTRAFIC && + efs.stype != chan->local_stype) + return -ECONNREFUSED; - l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs), - (unsigned long) &efs, endptr - ptr); + l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs), + (unsigned long) &efs, endptr - ptr); + } break; case L2CAP_CONF_FCS: