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@gmail.com Reported-by: Marcus Meissner meissner@suse.de Tested-by: Muyu Yu ieatmuttonchuan@gmail.com Signed-off-by: Oliver Hartkopp socketcan@hartkopp.net Cc: linux-stable stable@vger.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);
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@gmail.com Reported-by: Marcus Meissner meissner@suse.de Tested-by: Muyu Yu ieatmuttonchuan@gmail.com Signed-off-by: Oliver Hartkopp socketcan@hartkopp.net Cc: linux-stable stable@vger.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@suse.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@suse.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);
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:
(..)
/* 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:
- 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
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@suse.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);
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:
On Thu, Jan 03, 2019 at 01:26:34PM +0100, Oliver Hartkopp wrote:
(..)
/* 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:
- 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
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:
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.
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.
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:
- 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:
- 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
linux-stable-mirror@lists.linaro.org
-
Michal Kubecek -
Oliver Hartkopp