netdev - Re: [PATCH] can: gw: ensure DLC boundaries after CAN frame modification

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 3 Jan 2019 15:01:21 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Oliver Hartkopp <socketcan@...tkopp.net>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        ieatmuttonchuan@...il.com, meissner@...e.de,
        linux-can@...r.kernel.org, linux-stable <stable@...r.kernel.org>
Subject: Re: [PATCH] can: gw: ensure DLC boundaries after CAN frame
 modification

On Thu, Jan 03, 2019 at 01:26:34PM +0100, Oliver Hartkopp wrote:
> The CAN frame modification rules allow bitwise logical operations which can
> be also applied to the can_dlc field. Ensure the manipulation result to
> maintain the can_dlc boundaries so that the CAN drivers do not accidently
> write arbitrary content beyond the data registers in the CAN controllers
> I/O mem when processing can-gw manipulated outgoing frames. When passing these
> frames to user space this issue did not have any effect to the kernel or any
> leaked data as we always strictly copy sizeof(struct can_frame) bytes.
> 
> Reported-by: Muyu Yu <ieatmuttonchuan@...il.com>
> Reported-by: Marcus Meissner <meissner@...e.de>
> Tested-by: Muyu Yu <ieatmuttonchuan@...il.com>
> Signed-off-by: Oliver Hartkopp <socketcan@...tkopp.net>
> Cc: linux-stable <stable@...r.kernel.org> # >= v3.2
> ---
>  net/can/gw.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/can/gw.c b/net/can/gw.c
> index faa3da88a127..9000d9b8a133 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -418,6 +418,10 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
>  
>  	/* check for checksum updates when the CAN frame has been modified */
>  	if (modidx) {
> +		/* ensure DLC boundaries after the different mods */
> +		if (cf->can_dlc > 8)
> +			cf->can_dlc = 8;
> +
>  		if (gwj->mod.csumfunc.crc8)
>  			(*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);
>  

IMHO "8" should rather be "CAN_MAX_DLEN". I can see two problems with
your patch:

1. If I understand the code correctly, canfd_frame packets (which allow
larger lenth) are also processed by this code path.

2. Silently capping the DLC feels wrong, IMHO it would be cleaner to
drop the resulting packet if it's invalid.

This is the patch I came with:

>From 8cc56bc825fa88803c08a8f85dc315bc112a8b05 Mon Sep 17 00:00:00 2001
From: Michal Kubecek <mkubecek@...e.cz>
Date: Thu, 3 Jan 2019 11:00:26 +0100
Subject: [PATCH] can: recheck dlc value of modified packet

CAN frame modification rules allow modification (set, and, or, xor) of DLC
(or payload length value). The new value is not checked against
CAN(FD)_MAX_DLEN so that indicated payload length may reach beyond actual
packet length.

As reported to security list, cgw_csum_xor_rel() with negative offset can
then rewrite e.g. frag_list pointer in skb_shared_info, crashing the
system. With unprivileged user namespaces, this can be exploited by any
regular user.

Rather than distinguishing between can_frame and canfd_frame, check if
resulting payload length does not reach beyond nskb->len which is what we
are actually interested in.

Signed-off-by: Michal Kubecek <mkubecek@...e.cz>
---
 net/can/gw.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/can/gw.c b/net/can/gw.c
index faa3da88a127..87b7043e3250 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -418,6 +418,15 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 
 	/* check for checksum updates when the CAN frame has been modified */
 	if (modidx) {
+		int max_dlc = nskb->len - offsetof(struct can_frame, data);
+
+		/* dlc may have changed, check the new value */
+		if (cf->can_dlc > max_dlc) {
+			gwj->dropped_frames++;
+			kfree_skb(nskb);
+			return;
+		}
+
 		if (gwj->mod.csumfunc.crc8)
 			(*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);
 
-- 
2.20.1