[X/B/C/D,1/1] Bluetooth: Check L2CAP option sizes returned from l2cap_get_conf_opt

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>
---
 net/bluetooth/l2cap_core.c | 77 +++++++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 31 deletions(-)

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;
>  		}