Message ID | 20190219122746.4197-3-kai.heng.feng@canonical.com |
---|---|
State | New |
Headers | show |
Series | CVE-2019-3460 - Heap data infoleak in multiple locations including functionl2cap_parse_conf_rsp | expand |
On 2019-02-19 20:27:45, Kai-Heng Feng wrote: > From: Marcel Holtmann <marcel@holtmann.org> > > When doing option parsing for standard type values of 1, 2 or 4 octets, > the value is converted directly into a variable instead of a pointer. To > avoid being tricked into being a pointer, check that for these option > types that sizes actually match. In L2CAP every option is fixed size and > thus it is prudent anyway to ensure that the remote side sends us the > right option size along with option paramters. > > If the option size is not matching the option type, then that option is > silently ignored. It is a protocol violation and instead of trying to > give the remote attacker any further hints just pretend that option is > not present and proceed with the default values. Implementation > following the specification and its qualification procedures will always > use the correct size and thus not being impacted here. > > To keep the code readable and consistent accross all options, a few > cosmetic changes were also required. > > CVE-2019-3460 > > Signed-off-by: Marcel Holtmann <marcel@holtmann.org> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com> > (cherry picked from commit af3d5d1c87664a4f150fcf3534c6567cb19909b0 linux-next) > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> This looks good to me. As I mentioned on another bluetooth fix yesterday, I don't love that we're having to pick this out of linux-next but these patches have been slow to land in Linus' tree so I think it is appropriate to go ahead and pick them up now. Acked-by: Tyler Hicks <tyhicks@canonical.com> Thanks! Tyler > --- > net/bluetooth/l2cap_core.c | 77 +++++++++++++++++++++++--------------- > 1 file changed, 46 insertions(+), 31 deletions(-) > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > index d17a4736e47c..c192ec471c6d 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -3342,10 +3342,14 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data > > switch (type) { > case L2CAP_CONF_MTU: > + if (olen != 2) > + break; > mtu = val; > break; > > case L2CAP_CONF_FLUSH_TO: > + if (olen != 2) > + break; > chan->flush_to = val; > break; > > @@ -3353,26 +3357,30 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data > break; > > case L2CAP_CONF_RFC: > - if (olen == sizeof(rfc)) > - memcpy(&rfc, (void *) val, olen); > + if (olen != sizeof(rfc)) > + break; > + memcpy(&rfc, (void *) val, olen); > break; > > case L2CAP_CONF_FCS: > + if (olen != 1) > + break; > if (val == L2CAP_FCS_NONE) > set_bit(CONF_RECV_NO_FCS, &chan->conf_state); > break; > > case L2CAP_CONF_EFS: > - if (olen == sizeof(efs)) { > - remote_efs = 1; > - memcpy(&efs, (void *) val, olen); > - } > + if (olen != sizeof(efs)) > + break; > + remote_efs = 1; > + memcpy(&efs, (void *) val, olen); > break; > > case L2CAP_CONF_EWS: > + if (olen != 2) > + break; > if (!(chan->conn->local_fixed_chan & L2CAP_FC_A2MP)) > return -ECONNREFUSED; > - > set_bit(FLAG_EXT_CTRL, &chan->flags); > set_bit(CONF_EWS_RECV, &chan->conf_state); > chan->tx_win_max = L2CAP_DEFAULT_EXT_WINDOW; > @@ -3382,7 +3390,6 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data > default: > if (hint) > break; > - > result = L2CAP_CONF_UNKNOWN; > *((u8 *) ptr++) = type; > break; > @@ -3550,55 +3557,60 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, > > switch (type) { > case L2CAP_CONF_MTU: > + if (olen != 2) > + break; > if (val < L2CAP_DEFAULT_MIN_MTU) { > *result = L2CAP_CONF_UNACCEPT; > chan->imtu = L2CAP_DEFAULT_MIN_MTU; > } else > chan->imtu = val; > - l2cap_add_conf_opt(&ptr, L2CAP_CONF_MTU, 2, chan->imtu, endptr - ptr); > + l2cap_add_conf_opt(&ptr, L2CAP_CONF_MTU, 2, chan->imtu, > + endptr - ptr); > break; > > case L2CAP_CONF_FLUSH_TO: > + if (olen != 2) > + break; > chan->flush_to = val; > - l2cap_add_conf_opt(&ptr, L2CAP_CONF_FLUSH_TO, > - 2, chan->flush_to, endptr - ptr); > + l2cap_add_conf_opt(&ptr, L2CAP_CONF_FLUSH_TO, 2, > + chan->flush_to, endptr - ptr); > break; > > case L2CAP_CONF_RFC: > - if (olen == sizeof(rfc)) > - memcpy(&rfc, (void *)val, olen); > - > + if (olen != sizeof(rfc)) > + break; > + memcpy(&rfc, (void *)val, olen); > if (test_bit(CONF_STATE2_DEVICE, &chan->conf_state) && > rfc.mode != chan->mode) > return -ECONNREFUSED; > - > chan->fcs = 0; > - > - l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, > - sizeof(rfc), (unsigned long) &rfc, endptr - ptr); > + l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc), > + (unsigned long) &rfc, endptr - ptr); > break; > > case L2CAP_CONF_EWS: > + if (olen != 2) > + break; > chan->ack_win = min_t(u16, val, chan->ack_win); > l2cap_add_conf_opt(&ptr, L2CAP_CONF_EWS, 2, > chan->tx_win, endptr - ptr); > break; > > case L2CAP_CONF_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; > - > - l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs), > - (unsigned long) &efs, endptr - ptr); > - } > + if (olen != sizeof(efs)) > + break; > + memcpy(&efs, (void *)val, olen); > + 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); > break; > > case L2CAP_CONF_FCS: > + if (olen != 1) > + break; > if (*result == L2CAP_CONF_PENDING) > if (val == L2CAP_FCS_NONE) > set_bit(CONF_RECV_NO_FCS, > @@ -3730,10 +3742,13 @@ static void l2cap_conf_rfc_get(struct l2cap_chan *chan, void *rsp, int len) > > switch (type) { > case L2CAP_CONF_RFC: > - if (olen == sizeof(rfc)) > - memcpy(&rfc, (void *)val, olen); > + if (olen != sizeof(rfc)) > + break; > + memcpy(&rfc, (void *)val, olen); > break; > case L2CAP_CONF_EWS: > + if (olen != 2) > + break; > txwin_ext = val; > break; > } > -- > 2.17.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On Tue, Feb 19, 2019 at 08:27:45PM +0800, Kai-Heng Feng wrote: > From: Marcel Holtmann <marcel@holtmann.org> > > When doing option parsing for standard type values of 1, 2 or 4 octets, > the value is converted directly into a variable instead of a pointer. To > avoid being tricked into being a pointer, check that for these option > types that sizes actually match. In L2CAP every option is fixed size and > thus it is prudent anyway to ensure that the remote side sends us the > right option size along with option paramters. > > If the option size is not matching the option type, then that option is > silently ignored. It is a protocol violation and instead of trying to > give the remote attacker any further hints just pretend that option is > not present and proceed with the default values. Implementation > following the specification and its qualification procedures will always > use the correct size and thus not being impacted here. > > To keep the code readable and consistent accross all options, a few > cosmetic changes were also required. > > CVE-2019-3460 > > Signed-off-by: Marcel Holtmann <marcel@holtmann.org> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com> > (cherry picked from commit af3d5d1c87664a4f150fcf3534c6567cb19909b0 linux-next) > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> I wish the cosmetic changes had been made in a later patch, but it all looks safe to me. For this patch (I haven't reviewed the trusty patches): Acked-by: Seth Forshee <seth.forshee@canonical.com> Applied to disco/master-next and unstable/master, thanks!
On 2/19/19 1:27 PM, Kai-Heng Feng wrote: > From: Marcel Holtmann <marcel@holtmann.org> > > When doing option parsing for standard type values of 1, 2 or 4 octets, > the value is converted directly into a variable instead of a pointer. To > avoid being tricked into being a pointer, check that for these option > types that sizes actually match. In L2CAP every option is fixed size and > thus it is prudent anyway to ensure that the remote side sends us the > right option size along with option paramters. > > If the option size is not matching the option type, then that option is > silently ignored. It is a protocol violation and instead of trying to > give the remote attacker any further hints just pretend that option is > not present and proceed with the default values. Implementation > following the specification and its qualification procedures will always > use the correct size and thus not being impacted here. > > To keep the code readable and consistent accross all options, a few > cosmetic changes were also required. > > CVE-2019-3460 > > Signed-off-by: Marcel Holtmann <marcel@holtmann.org> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com> > (cherry picked from commit af3d5d1c87664a4f150fcf3534c6567cb19909b0 linux-next) Applied to {xenial,bionic,cosmic}/master-next branches, removing "linux-next" from the provenance line above since the patch has already been merged to Linus' tree. Thanks, Kleber > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > net/bluetooth/l2cap_core.c | 77 +++++++++++++++++++++++--------------- > 1 file changed, 46 insertions(+), 31 deletions(-) > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > index d17a4736e47c..c192ec471c6d 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -3342,10 +3342,14 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data > > switch (type) { > case L2CAP_CONF_MTU: > + if (olen != 2) > + break; > mtu = val; > break; > > case L2CAP_CONF_FLUSH_TO: > + if (olen != 2) > + break; > chan->flush_to = val; > break; > > @@ -3353,26 +3357,30 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data > break; > > case L2CAP_CONF_RFC: > - if (olen == sizeof(rfc)) > - memcpy(&rfc, (void *) val, olen); > + if (olen != sizeof(rfc)) > + break; > + memcpy(&rfc, (void *) val, olen); > break; > > case L2CAP_CONF_FCS: > + if (olen != 1) > + break; > if (val == L2CAP_FCS_NONE) > set_bit(CONF_RECV_NO_FCS, &chan->conf_state); > break; > > case L2CAP_CONF_EFS: > - if (olen == sizeof(efs)) { > - remote_efs = 1; > - memcpy(&efs, (void *) val, olen); > - } > + if (olen != sizeof(efs)) > + break; > + remote_efs = 1; > + memcpy(&efs, (void *) val, olen); > break; > > case L2CAP_CONF_EWS: > + if (olen != 2) > + break; > if (!(chan->conn->local_fixed_chan & L2CAP_FC_A2MP)) > return -ECONNREFUSED; > - > set_bit(FLAG_EXT_CTRL, &chan->flags); > set_bit(CONF_EWS_RECV, &chan->conf_state); > chan->tx_win_max = L2CAP_DEFAULT_EXT_WINDOW; > @@ -3382,7 +3390,6 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data > default: > if (hint) > break; > - > result = L2CAP_CONF_UNKNOWN; > *((u8 *) ptr++) = type; > break; > @@ -3550,55 +3557,60 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, > > switch (type) { > case L2CAP_CONF_MTU: > + if (olen != 2) > + break; > if (val < L2CAP_DEFAULT_MIN_MTU) { > *result = L2CAP_CONF_UNACCEPT; > chan->imtu = L2CAP_DEFAULT_MIN_MTU; > } else > chan->imtu = val; > - l2cap_add_conf_opt(&ptr, L2CAP_CONF_MTU, 2, chan->imtu, endptr - ptr); > + l2cap_add_conf_opt(&ptr, L2CAP_CONF_MTU, 2, chan->imtu, > + endptr - ptr); > break; > > case L2CAP_CONF_FLUSH_TO: > + if (olen != 2) > + break; > chan->flush_to = val; > - l2cap_add_conf_opt(&ptr, L2CAP_CONF_FLUSH_TO, > - 2, chan->flush_to, endptr - ptr); > + l2cap_add_conf_opt(&ptr, L2CAP_CONF_FLUSH_TO, 2, > + chan->flush_to, endptr - ptr); > break; > > case L2CAP_CONF_RFC: > - if (olen == sizeof(rfc)) > - memcpy(&rfc, (void *)val, olen); > - > + if (olen != sizeof(rfc)) > + break; > + memcpy(&rfc, (void *)val, olen); > if (test_bit(CONF_STATE2_DEVICE, &chan->conf_state) && > rfc.mode != chan->mode) > return -ECONNREFUSED; > - > chan->fcs = 0; > - > - l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, > - sizeof(rfc), (unsigned long) &rfc, endptr - ptr); > + l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc), > + (unsigned long) &rfc, endptr - ptr); > break; > > case L2CAP_CONF_EWS: > + if (olen != 2) > + break; > chan->ack_win = min_t(u16, val, chan->ack_win); > l2cap_add_conf_opt(&ptr, L2CAP_CONF_EWS, 2, > chan->tx_win, endptr - ptr); > break; > > case L2CAP_CONF_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; > - > - l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs), > - (unsigned long) &efs, endptr - ptr); > - } > + if (olen != sizeof(efs)) > + break; > + memcpy(&efs, (void *)val, olen); > + 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); > break; > > case L2CAP_CONF_FCS: > + if (olen != 1) > + break; > if (*result == L2CAP_CONF_PENDING) > if (val == L2CAP_FCS_NONE) > set_bit(CONF_RECV_NO_FCS, > @@ -3730,10 +3742,13 @@ static void l2cap_conf_rfc_get(struct l2cap_chan *chan, void *rsp, int len) > > switch (type) { > case L2CAP_CONF_RFC: > - if (olen == sizeof(rfc)) > - memcpy(&rfc, (void *)val, olen); > + if (olen != sizeof(rfc)) > + break; > + memcpy(&rfc, (void *)val, olen); > break; > case L2CAP_CONF_EWS: > + if (olen != 2) > + break; > txwin_ext = val; > break; > }
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c index d17a4736e47c..c192ec471c6d 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -3342,10 +3342,14 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data switch (type) { case L2CAP_CONF_MTU: + if (olen != 2) + break; mtu = val; break; case L2CAP_CONF_FLUSH_TO: + if (olen != 2) + break; chan->flush_to = val; break; @@ -3353,26 +3357,30 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data break; case L2CAP_CONF_RFC: - if (olen == sizeof(rfc)) - memcpy(&rfc, (void *) val, olen); + if (olen != sizeof(rfc)) + break; + memcpy(&rfc, (void *) val, olen); break; case L2CAP_CONF_FCS: + if (olen != 1) + break; if (val == L2CAP_FCS_NONE) set_bit(CONF_RECV_NO_FCS, &chan->conf_state); break; case L2CAP_CONF_EFS: - if (olen == sizeof(efs)) { - remote_efs = 1; - memcpy(&efs, (void *) val, olen); - } + if (olen != sizeof(efs)) + break; + remote_efs = 1; + memcpy(&efs, (void *) val, olen); break; case L2CAP_CONF_EWS: + if (olen != 2) + break; if (!(chan->conn->local_fixed_chan & L2CAP_FC_A2MP)) return -ECONNREFUSED; - set_bit(FLAG_EXT_CTRL, &chan->flags); set_bit(CONF_EWS_RECV, &chan->conf_state); chan->tx_win_max = L2CAP_DEFAULT_EXT_WINDOW; @@ -3382,7 +3390,6 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data default: if (hint) break; - result = L2CAP_CONF_UNKNOWN; *((u8 *) ptr++) = type; break; @@ -3550,55 +3557,60 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, switch (type) { case L2CAP_CONF_MTU: + if (olen != 2) + break; if (val < L2CAP_DEFAULT_MIN_MTU) { *result = L2CAP_CONF_UNACCEPT; chan->imtu = L2CAP_DEFAULT_MIN_MTU; } else chan->imtu = val; - l2cap_add_conf_opt(&ptr, L2CAP_CONF_MTU, 2, chan->imtu, endptr - ptr); + l2cap_add_conf_opt(&ptr, L2CAP_CONF_MTU, 2, chan->imtu, + endptr - ptr); break; case L2CAP_CONF_FLUSH_TO: + if (olen != 2) + break; chan->flush_to = val; - l2cap_add_conf_opt(&ptr, L2CAP_CONF_FLUSH_TO, - 2, chan->flush_to, endptr - ptr); + l2cap_add_conf_opt(&ptr, L2CAP_CONF_FLUSH_TO, 2, + chan->flush_to, endptr - ptr); break; case L2CAP_CONF_RFC: - if (olen == sizeof(rfc)) - memcpy(&rfc, (void *)val, olen); - + if (olen != sizeof(rfc)) + break; + memcpy(&rfc, (void *)val, olen); if (test_bit(CONF_STATE2_DEVICE, &chan->conf_state) && rfc.mode != chan->mode) return -ECONNREFUSED; - chan->fcs = 0; - - l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, - sizeof(rfc), (unsigned long) &rfc, endptr - ptr); + l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc), + (unsigned long) &rfc, endptr - ptr); break; case L2CAP_CONF_EWS: + if (olen != 2) + break; chan->ack_win = min_t(u16, val, chan->ack_win); l2cap_add_conf_opt(&ptr, L2CAP_CONF_EWS, 2, chan->tx_win, endptr - ptr); break; case L2CAP_CONF_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; - - l2cap_add_conf_opt(&ptr, L2CAP_CONF_EFS, sizeof(efs), - (unsigned long) &efs, endptr - ptr); - } + if (olen != sizeof(efs)) + break; + memcpy(&efs, (void *)val, olen); + 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); break; case L2CAP_CONF_FCS: + if (olen != 1) + break; if (*result == L2CAP_CONF_PENDING) if (val == L2CAP_FCS_NONE) set_bit(CONF_RECV_NO_FCS, @@ -3730,10 +3742,13 @@ static void l2cap_conf_rfc_get(struct l2cap_chan *chan, void *rsp, int len) switch (type) { case L2CAP_CONF_RFC: - if (olen == sizeof(rfc)) - memcpy(&rfc, (void *)val, olen); + if (olen != sizeof(rfc)) + break; + memcpy(&rfc, (void *)val, olen); break; case L2CAP_CONF_EWS: + if (olen != 2) + break; txwin_ext = val; break; }