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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Automotive Discussions]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [CAN Bus]

  Powered by Linux