| Message ID | 20190104091353.2438-1-socketcan@hartkopp.net |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | can: gw: ensure DLC boundaries after CAN frame modification | expand |
On Fri, Jan 04, 2019 at 10:13:53AM +0100, Oliver Hartkopp wrote: > Muyu Yu provided a POC where user root with CAP_NET_ADMIN can create a CAN > frame modification rule that makes the data length code a higher value than > the available CAN frame data size. In combination with a configured checksum > calculation where the result is stored relatively to the end of the data > (e.g. cgw_csum_xor_rel) the tail of the skb (e.g. frag_list pointer in > skb_shared_info) can be rewritten which finally can cause a system crash. > > Michael Kubecek suggested to drop frames that have a DLC exceeding the > available space after the modification process and provided a patch that can > handle CAN FD frames too. Within this patch we also limit the length for the > checksum calculations to the maximum of Classic CAN data length (8). > > CAN frames that are dropped by these additional checks are counted with the > CGW_DELETED counter which indicates misconfigurations in can-gw rules. > > This fixes CVE-2019-3701. > > Reported-by: Muyu Yu <ieatmuttonchuan@gmail.com> > Reported-by: Marcus Meissner <meissner@suse.de> > Suggested-by: Michal Kubecek <mkubecek@suse.cz> > Tested-by: Muyu Yu <ieatmuttonchuan@gmail.com> > Tested-by: Oliver Hartkopp <socketcan@hartkopp.net> > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> > Cc: linux-stable <stable@vger.kernel.org> # >= v3.2 > --- > net/can/gw.c | 36 +++++++++++++++++++++++++++++++----- > 1 file changed, 31 insertions(+), 5 deletions(-) > > diff --git a/net/can/gw.c b/net/can/gw.c > index faa3da88a127..180c389af5b1 100644 > --- a/net/can/gw.c > +++ b/net/can/gw.c > @@ -416,13 +416,31 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data) > while (modidx < MAX_MODFUNCTIONS && gwj->mod.modfunc[modidx]) > (*gwj->mod.modfunc[modidx++])(cf, &gwj->mod); > > - /* check for checksum updates when the CAN frame has been modified */ > + /* Has the CAN frame been modified? */ > if (modidx) { > - if (gwj->mod.csumfunc.crc8) > - (*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8); > + /* get available space for the processed CAN frame type */ > + int max_len = nskb->len - offsetof(struct can_frame, data); > > - if (gwj->mod.csumfunc.xor) > - (*gwj->mod.csumfunc.xor)(cf, &gwj->mod.csum.xor); > + /* dlc may have changed, make sure it fits to the CAN frame */ > + if (cf->can_dlc > max_len) > + goto out_delete; > + > + /* check for checksum updates in classic CAN length only */ > + if (gwj->mod.csumfunc.crc8) { > + if (cf->can_dlc > 8) > + goto out_delete; > + else > + (*gwj->mod.csumfunc.crc8) > + (cf, &gwj->mod.csum.crc8); > + } > + > + if (gwj->mod.csumfunc.xor) { > + if (cf->can_dlc > 8) > + goto out_delete; > + else > + (*gwj->mod.csumfunc.xor) > + (cf, &gwj->mod.csum.xor); > + } > } > > /* clear the skb timestamp if not configured the other way */ > @@ -434,6 +452,14 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data) > gwj->dropped_frames++; > else > gwj->handled_frames++; > + > + return; > + > +out_delete: > + /* delete frame due to misconfiguration */ > + gwj->deleted_frames++; > + kfree_skb(nskb); > + return; > } > > static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj) > -- > 2.19.2 > Except for the "8" vs "CAN_MAX_DLEN" issue discussed in v1, looks good to me. Michal Kubecek
On 1/4/19 11:31 AM, Michal Kubecek wrote: > On Fri, Jan 04, 2019 at 10:13:53AM +0100, Oliver Hartkopp wrote: >> Muyu Yu provided a POC where user root with CAP_NET_ADMIN can create a CAN >> frame modification rule that makes the data length code a higher value than >> the available CAN frame data size. In combination with a configured checksum >> calculation where the result is stored relatively to the end of the data >> (e.g. cgw_csum_xor_rel) the tail of the skb (e.g. frag_list pointer in >> skb_shared_info) can be rewritten which finally can cause a system crash. >> >> Michael Kubecek suggested to drop frames that have a DLC exceeding the >> available space after the modification process and provided a patch that can >> handle CAN FD frames too. Within this patch we also limit the length for the >> checksum calculations to the maximum of Classic CAN data length (8). >> >> CAN frames that are dropped by these additional checks are counted with the >> CGW_DELETED counter which indicates misconfigurations in can-gw rules. >> >> This fixes CVE-2019-3701. >> >> Reported-by: Muyu Yu <ieatmuttonchuan@gmail.com> >> Reported-by: Marcus Meissner <meissner@suse.de> >> Suggested-by: Michal Kubecek <mkubecek@suse.cz> >> Tested-by: Muyu Yu <ieatmuttonchuan@gmail.com> >> Tested-by: Oliver Hartkopp <socketcan@hartkopp.net> >> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> >> Cc: linux-stable <stable@vger.kernel.org> # >= v3.2 >> --- >> net/can/gw.c | 36 +++++++++++++++++++++++++++++++----- >> 1 file changed, 31 insertions(+), 5 deletions(-) >> >> diff --git a/net/can/gw.c b/net/can/gw.c >> index faa3da88a127..180c389af5b1 100644 >> --- a/net/can/gw.c >> +++ b/net/can/gw.c >> @@ -416,13 +416,31 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data) >> while (modidx < MAX_MODFUNCTIONS && gwj->mod.modfunc[modidx]) >> (*gwj->mod.modfunc[modidx++])(cf, &gwj->mod); >> >> - /* check for checksum updates when the CAN frame has been modified */ >> + /* Has the CAN frame been modified? */ >> if (modidx) { >> - if (gwj->mod.csumfunc.crc8) >> - (*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8); >> + /* get available space for the processed CAN frame type */ >> + int max_len = nskb->len - offsetof(struct can_frame, data); >> >> - if (gwj->mod.csumfunc.xor) >> - (*gwj->mod.csumfunc.xor)(cf, &gwj->mod.csum.xor); >> + /* dlc may have changed, make sure it fits to the CAN frame */ >> + if (cf->can_dlc > max_len) >> + goto out_delete; >> + >> + /* check for checksum updates in classic CAN length only */ >> + if (gwj->mod.csumfunc.crc8) { >> + if (cf->can_dlc > 8) >> + goto out_delete; >> + else >> + (*gwj->mod.csumfunc.crc8) >> + (cf, &gwj->mod.csum.crc8); >> + } >> + >> + if (gwj->mod.csumfunc.xor) { >> + if (cf->can_dlc > 8) >> + goto out_delete; >> + else >> + (*gwj->mod.csumfunc.xor) >> + (cf, &gwj->mod.csum.xor); >> + } >> } >> >> /* clear the skb timestamp if not configured the other way */ >> @@ -434,6 +452,14 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data) >> gwj->dropped_frames++; >> else >> gwj->handled_frames++; >> + >> + return; >> + >> +out_delete: >> + /* delete frame due to misconfiguration */ >> + gwj->deleted_frames++; >> + kfree_skb(nskb); >> + return; >> } >> >> static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj) >> -- >> 2.19.2 >> > > Except for the "8" vs "CAN_MAX_DLEN" issue discussed in v1, looks good > to me. Thanks for the review! Best regards, Oliver
Hi all, just for the records: I did some tests with the cangw tool from https://github.com/linux-can/can-utils where I removed some sanity checks (6,7) in parse_mod() to be able to reproduce Muyu's setup. The filters are configured as follows: # cangw -L cangw -A -s vcan0 -d vcan1 -e -m SET:IL:400.41.0000000000000000 -f 001:C00007FF # 0 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:401.40.0000000000000000 -f 001:C00007FF # 0 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:400.3F.0000000000000000 -f 001:C00007FF # 0 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:300.9.0000000000000000 -f 001:C00007FF # 0 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:301.8.0000000000000000 -f 001:C00007FF # 0 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:300.7.0000000000000000 -f 001:C00007FF # 0 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:200.41.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 0 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:201.40.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 0 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:200.3F.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 0 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:102.9.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 0 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:101.8.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 0 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:100.7.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 0 handled 0 dropped 0 deleted After sending a Classic CAN frame DLC of 1: $ cansend vcan0 001#22 candump produces this: $ candump -ta any (1546606529.898372) vcan0 001 [1] 22 (1546606529.898830) vcan1 301 [8] 22 00 00 00 00 00 00 00 (1546606529.898921) vcan1 300 [7] 22 00 00 00 00 00 00 (1546606529.899308) vcan1 101 [8] 22 00 00 00 00 00 22 00 (1546606529.899399) vcan1 100 [7] 22 00 00 00 00 22 00 And the handled and deleted frames are counted as follows: # cangw -L cangw -A -s vcan0 -d vcan1 -e -m SET:IL:400.41.0000000000000000 -f 001:C00007FF # 0 handled 0 dropped 1 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:401.40.0000000000000000 -f 001:C00007FF # 0 handled 0 dropped 1 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:400.3F.0000000000000000 -f 001:C00007FF # 0 handled 0 dropped 1 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:300.9.0000000000000000 -f 001:C00007FF # 0 handled 0 dropped 1 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:301.8.0000000000000000 -f 001:C00007FF # 1 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:300.7.0000000000000000 -f 001:C00007FF # 1 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:200.41.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 0 handled 0 dropped 1 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:201.40.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 0 handled 0 dropped 1 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:200.3F.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 0 handled 0 dropped 1 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:102.9.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 0 handled 0 dropped 1 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:101.8.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 1 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:100.7.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 1 handled 0 dropped 0 deleted So all modified DLC values that did not fit into the frame lead to deleted CAN frames. When sending a CAN FD frame with DLC of 1 as an alternative: $ cansend vcan0 001##022 candump produces this: $ candump -ta any (1546607375.109495) vcan0 001 [01] 22 (1546607375.109749) vcan1 401 [64] 22 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 (1546607375.109850) vcan1 400 [63] 22 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 (1546607375.109949) vcan1 300 [09] 22 00 00 00 00 00 00 00 00 (1546607375.110049) vcan1 301 [08] 22 00 00 00 00 00 00 00 (1546607375.110148) vcan1 300 [07] 22 00 00 00 00 00 00 (1546607375.110574) vcan1 101 [08] 22 00 00 00 00 00 22 00 (1546607375.110674) vcan1 100 [07] 22 00 00 00 00 22 00 And the handled and deleted frames are counted as follows: # cangw -L cangw -A -s vcan0 -d vcan1 -e -m SET:IL:400.41.0000000000000000 -f 001:C00007FF # 0 handled 0 dropped 1 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:401.40.0000000000000000 -f 001:C00007FF # 1 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:400.3F.0000000000000000 -f 001:C00007FF # 1 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:300.9.0000000000000000 -f 001:C00007FF # 1 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:301.8.0000000000000000 -f 001:C00007FF # 1 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:300.7.0000000000000000 -f 001:C00007FF # 1 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:200.41.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 0 handled 0 dropped 1 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:201.40.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 0 handled 0 dropped 1 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:200.3F.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 0 handled 0 dropped 1 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:102.9.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 0 handled 0 dropped 1 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:101.8.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 1 handled 0 dropped 0 deleted cangw -A -s vcan0 -d vcan1 -e -m SET:IL:100.7.0000000000000000 -x -6:0:-2:00 -f 001:C00007FF # 1 handled 0 dropped 0 deleted Here you can see the additional effect of deleting frames with a dlc > 8 that are configured to calculate a checksum. So finally all new code paths of the patch have been triggered and do the intended stuff. Best regards, Oliver
On 1/4/19 10:13 AM, Oliver Hartkopp wrote: > Muyu Yu provided a POC where user root with CAP_NET_ADMIN can create a CAN > frame modification rule that makes the data length code a higher value than > the available CAN frame data size. In combination with a configured checksum > calculation where the result is stored relatively to the end of the data > (e.g. cgw_csum_xor_rel) the tail of the skb (e.g. frag_list pointer in > skb_shared_info) can be rewritten which finally can cause a system crash. > > Michael Kubecek suggested to drop frames that have a DLC exceeding the > available space after the modification process and provided a patch that can > handle CAN FD frames too. Within this patch we also limit the length for the > checksum calculations to the maximum of Classic CAN data length (8). > > CAN frames that are dropped by these additional checks are counted with the > CGW_DELETED counter which indicates misconfigurations in can-gw rules. > > This fixes CVE-2019-3701. > > Reported-by: Muyu Yu <ieatmuttonchuan@gmail.com> > Reported-by: Marcus Meissner <meissner@suse.de> > Suggested-by: Michal Kubecek <mkubecek@suse.cz> > Tested-by: Muyu Yu <ieatmuttonchuan@gmail.com> > Tested-by: Oliver Hartkopp <socketcan@hartkopp.net> > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> > Cc: linux-stable <stable@vger.kernel.org> # >= v3.2 > --- > net/can/gw.c | 36 +++++++++++++++++++++++++++++++----- > 1 file changed, 31 insertions(+), 5 deletions(-) > > diff --git a/net/can/gw.c b/net/can/gw.c > index faa3da88a127..180c389af5b1 100644 > --- a/net/can/gw.c > +++ b/net/can/gw.c > @@ -416,13 +416,31 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data) > while (modidx < MAX_MODFUNCTIONS && gwj->mod.modfunc[modidx]) > (*gwj->mod.modfunc[modidx++])(cf, &gwj->mod); > > - /* check for checksum updates when the CAN frame has been modified */ > + /* Has the CAN frame been modified? */ > if (modidx) { > - if (gwj->mod.csumfunc.crc8) > - (*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8); > + /* get available space for the processed CAN frame type */ > + int max_len = nskb->len - offsetof(struct can_frame, data); > > - if (gwj->mod.csumfunc.xor) > - (*gwj->mod.csumfunc.xor)(cf, &gwj->mod.csum.xor); > + /* dlc may have changed, make sure it fits to the CAN frame */ > + if (cf->can_dlc > max_len) > + goto out_delete; > + > + /* check for checksum updates in classic CAN length only */ > + if (gwj->mod.csumfunc.crc8) { > + if (cf->can_dlc > 8) > + goto out_delete; > + else > + (*gwj->mod.csumfunc.crc8) > + (cf, &gwj->mod.csum.crc8); What about removing the else, then we can keep this in one line. > + } > + > + if (gwj->mod.csumfunc.xor) { > + if (cf->can_dlc > 8) > + goto out_delete; > + else > + (*gwj->mod.csumfunc.xor) > + (cf, &gwj->mod.csum.xor); same here > + } > } > > /* clear the skb timestamp if not configured the other way */ > @@ -434,6 +452,14 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data) > gwj->dropped_frames++; > else > gwj->handled_frames++; > + > + return; > + > +out_delete: > + /* delete frame due to misconfiguration */ > + gwj->deleted_frames++; > + kfree_skb(nskb); > + return; > } > > static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj) > Marc
diff --git a/net/can/gw.c b/net/can/gw.c index faa3da88a127..180c389af5b1 100644 --- a/net/can/gw.c +++ b/net/can/gw.c @@ -416,13 +416,31 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data) while (modidx < MAX_MODFUNCTIONS && gwj->mod.modfunc[modidx]) (*gwj->mod.modfunc[modidx++])(cf, &gwj->mod); - /* check for checksum updates when the CAN frame has been modified */ + /* Has the CAN frame been modified? */ if (modidx) { - if (gwj->mod.csumfunc.crc8) - (*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8); + /* get available space for the processed CAN frame type */ + int max_len = nskb->len - offsetof(struct can_frame, data); - if (gwj->mod.csumfunc.xor) - (*gwj->mod.csumfunc.xor)(cf, &gwj->mod.csum.xor); + /* dlc may have changed, make sure it fits to the CAN frame */ + if (cf->can_dlc > max_len) + goto out_delete; + + /* check for checksum updates in classic CAN length only */ + if (gwj->mod.csumfunc.crc8) { + if (cf->can_dlc > 8) + goto out_delete; + else + (*gwj->mod.csumfunc.crc8) + (cf, &gwj->mod.csum.crc8); + } + + if (gwj->mod.csumfunc.xor) { + if (cf->can_dlc > 8) + goto out_delete; + else + (*gwj->mod.csumfunc.xor) + (cf, &gwj->mod.csum.xor); + } } /* clear the skb timestamp if not configured the other way */ @@ -434,6 +452,14 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data) gwj->dropped_frames++; else gwj->handled_frames++; + + return; + +out_delete: + /* delete frame due to misconfiguration */ + gwj->deleted_frames++; + kfree_skb(nskb); + return; } static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj)