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

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 <redacted>
Reported-by: Marcus Meissner <redacted>
Tested-by: Muyu Yu <redacted>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: linux-stable <redacted> # >= 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);
 

-- 
2.19.2

On Thu, Jan 03, 2019 at 01:26:34PM +0100, Oliver Hartkopp wrote:

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 <redacted>
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 <redacted>
---
 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

Hi Michal,

On 1/3/19 3:01 PM, Michal Kubecek wrote:

On Thu, Jan 03, 2019 at 01:26:34PM +0100, Oliver Hartkopp wrote:

(..)

quoted

  	/* 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.

In fact the can-gw frame modification and checksum functionalities lack CAN FD support today.

If you take a look into the netlink API only struct can_frame's can be supplied for frame modifications - and so are the checks e.g. in cgw_chk_csum_parms().

The given patch fixes the problem as described in the commit message in all stable Linux versions since can-gw appeared in Linux 3.2.

Anyway your modification makes definitely sense, as it allows to process CAN FD frames in struct canfd_frame as long as only data is modified that is also available in a struct can_frame. AND - as a bonus - it should work for stable 3.2 too, when CAN FD was not even introduced. Good idea!

If it's ok for you I would like to re-send the patch together with the CVE number and would like to credit your suggestion in the text and with "Suggested-by:".

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.

This is wrong! First there is no negative offset, as can_dlc is a u8 value. Therefore you can try to hit content in the tail of the skb only.
Second can-gw rules can only be configured by *root* and not by any regular user - and finally it is definitely not namespace related.

So the user root can configure a can-gw rule to shoot into the skb tail to kill the machine. I can imagine better things to do when I'm already root ;-)

Thanks & best regards,
Oliver

On Thu, Jan 03, 2019 at 08:31:51PM +0100, Oliver Hartkopp wrote:

Hi Michal,

On 1/3/19 3:01 PM, Michal Kubecek wrote:

quoted

On Thu, Jan 03, 2019 at 01:26:34PM +0100, Oliver Hartkopp wrote:

(..)

quoted

quoted

  	/* 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.

In fact the can-gw frame modification and checksum functionalities lack CAN
FD support today.

If you take a look into the netlink API only struct can_frame's can be
supplied for frame modifications - and so are the checks e.g. in
cgw_chk_csum_parms().

The given patch fixes the problem as described in the commit message in all
stable Linux versions since can-gw appeared in Linux 3.2.

Anyway your modification makes definitely sense, as it allows to process CAN
FD frames in struct canfd_frame as long as only data is modified that is
also available in a struct can_frame. AND - as a bonus - it should work for
stable 3.2 too, when CAN FD was not even introduced. Good idea!

If it's ok for you I would like to re-send the patch together with the CVE
number and would like to credit your suggestion in the text and with
"Suggested-by:".

OK

quoted

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.

This is wrong! First there is no negative offset, as can_dlc is a u8 value.
Therefore you can try to hit content in the tail of the skb only.

I probably didn't use the right term. By "negative offset" I meant the
value of cgw_csum_xor::result_idx which, if negative, is interpreted as
an offset relative to can_dlc. If the (invalid) value of modified
can_dlc is sufficiently large (larger then actual nskb->len), userspace
can enforce writing past packet data.

See http://bugzilla.suse.com/show_bug.cgi?id=1120386 (comment 1) for an
example which can crash unfixed kernel by rewriting a pointer in skb
shared data which is later dereferenced when the skb is freed.

Second can-gw rules can only be configured by *root* and not by any regular
user - and finally it is definitely not namespace related.

So the user root can configure a can-gw rule to shoot into the skb tail to
kill the machine. I can imagine better things to do when I'm already root

Sorry for the noise, I misread the code (and commit 90f62cf30a78) so
that I thought netlink_ns_capable() is used in net/can/gw.c; now I see
that it's netlink_capable() so that global CAP_NET_ADMIN is required
rather than namespace one and the bug cannot be exploited by a regular
user.

Michal Kubecek

Hi Michal,

On 1/3/19 9:33 PM, Michal Kubecek wrote:

On Thu, Jan 03, 2019 at 08:31:51PM +0100, Oliver Hartkopp wrote:

quoted

Anyway your modification makes definitely sense, as it allows to process CAN
FD frames in struct canfd_frame as long as only data is modified that is
also available in a struct can_frame. AND - as a bonus - it should work for
stable 3.2 too, when CAN FD was not even introduced. Good idea!

If it's ok for you I would like to re-send the patch together with the CVE
number and would like to credit your suggestion in the text and with
"Suggested-by:".

OK

Thanks!

I have created a new patch that allows CAN FD frames to be handled within the possibilities a struct can_frame offers (so especially with can_id and can_dlc). As checksum calculation currently only supports 'Classic CAN' CAN FD frames are dropped when the can_dlc becomes > 8.

Two other hints about the upcoming patch:

1. I'm still using the test "if (cf->can_dlc > 8)" with a proper comment as CAN_MAX_DLC and CAN_MAX_DLEN are not defined in Linux stable 3.2 - so that we can apply the patch to all versions of gw.c

2. The counter gwj->deleted_frames is increased when dropping *invalid* frames as this indicates a potential misconfiguration. gwj->dropped_frames is only used for allocation problems like OOM.

quoted

quoted

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.

This is wrong! First there is no negative offset, as can_dlc is a u8 value.
Therefore you can try to hit content in the tail of the skb only.

I probably didn't use the right term. By "negative offset" I meant the
value of cgw_csum_xor::result_idx which, if negative, is interpreted as
an offset relative to can_dlc.

If result_idx is negative the position is calculated back from the receive dlc - so to some index between 0 and dlc.

If the (invalid) value of modified
can_dlc is sufficiently large (larger then actual nskb->len), userspace
can enforce writing past packet data.

Yes. This hits the point.

See http://bugzilla.suse.com/show_bug.cgi?id=1120386 (comment 1) for an
example which can crash unfixed kernel by rewriting a pointer in skb
shared data which is later dereferenced when the skb is freed.

quoted

Second can-gw rules can only be configured by *root* and not by any regular
user - and finally it is definitely not namespace related.

So the user root can configure a can-gw rule to shoot into the skb tail to
kill the machine. I can imagine better things to do when I'm already root

Sorry for the noise, I misread the code (and commit 90f62cf30a78) so
that I thought netlink_ns_capable() is used in net/can/gw.c; now I see
that it's netlink_capable() so that global CAP_NET_ADMIN is required
rather than namespace one and the bug cannot be exploited by a regular
user.

No problem :-)

Thanks for going through the code that deep!

Best regards,
Oliver

On Thu, Jan 03, 2019 at 10:03:47PM +0100, Oliver Hartkopp wrote:

1. I'm still using the test "if (cf->can_dlc > 8)" with a proper comment as
CAN_MAX_DLC and CAN_MAX_DLEN are not defined in Linux stable 3.2 - so that
we can apply the patch to all versions of gw.c

I may be biased as substantial part of my work consists of trying to
understand unfamiliar code written by other people but I believe
readability of the code is more important than avoiding a bit of work
with backport to older stable branches.

Michal Kubecek

Hi Michal,

On 1/4/19 10:01 AM, Michal Kubecek wrote:

On Thu, Jan 03, 2019 at 10:03:47PM +0100, Oliver Hartkopp wrote:

quoted

1. I'm still using the test "if (cf->can_dlc > 8)" with a proper comment as
CAN_MAX_DLC and CAN_MAX_DLEN are not defined in Linux stable 3.2 - so that
we can apply the patch to all versions of gw.c

I may be biased as substantial part of my work consists of trying to
understand unfamiliar code written by other people but I believe
readability of the code is more important than avoiding a bit of work
with backport to older stable branches.

I am with you.
Please take a look into the new patch. I think either the comment and the commit message make it very clear where the value comes from.

E.g. in cgw_chk_csum_parms() we have tons of fixed constants that have been introduced when CAN FD was far away from thinking. I introduced CAN_MAX_DLC and CAN_MAX_DLEN myself when I started the CAN FD support in Linux. gw.c would need a rework for full CAN FD support - where these defines then definitely would be introduced.

In this single case the simple applying to stable kernels weighted more to me.

Regards,
Oliver