Message-ID: <715d1d82-af65-9344-dcce-0ddcace50274@hartkopp.net>
Date:   Thu, 3 Jan 2019 22:03:47 +0100
From:   Oliver Hartkopp <socketcan@...tkopp.net>
To:     Michal Kubecek <mkubecek@...e.cz>
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

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

Powered by blists - more mailing lists