netdev - Re: [PATCH net] ieee802154: lowpan_header

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF=yD-JXOGLBdmTT=KD6Ydr0cafgbkvS+58Gd_YRJBhn6754fg@mail.gmail.com>
Date:   Sun, 23 Dec 2018 19:45:08 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Alexander Aring <aring@...atatu.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Alexander Aring <alex.aring@...il.com>,
        jukka.rissanen@...ux.intel.com,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net] ieee802154: lowpan_header_create check must check daddr

On Sun, Dec 23, 2018 at 3:45 PM Alexander Aring <aring@...atatu.com> wrote:
>
> Hi,
>
> thanks Willem to take a look into these callbacks.
>
> On Sun, Dec 23, 2018 at 12:52:18PM -0500, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@...gle.com>
> >
> > Packet sockets may call dev_header_parse with NULL daddr. Make
> > lowpan_header_ops.create fail.
> >
>
> Ok.
>
> > Fixes: 87a93e4eceb4 ("ieee802154: change needed headroom/tailroom")
> > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> >
>
> Acked-by: Alexander Aring <aring@...atatu.com>

Thanks Alexander.

>
> > ---
> >
> > Re: function comment on packet socket address length: that is (now)
> > verified to be at least dev->addr_len.
> >
>
> I had some questions when I was digging AF_PACKET code. So the UAPI
> limitation of AF_PACKET has a sockaddr_t of 8 bytes.

Do you mean the size of sll_addr in struct sockaddr_ll?

It is possible to pass larger addresses, such as INFINIBAND_ALEN. See
also the checks in packet_snd:

                if (msg->msg_namelen < sizeof(struct sockaddr_ll))
                        goto out;
                if (msg->msg_namelen < (saddr->sll_halen +
offsetof(struct sockaddr_ll, sll_addr)))
                        goto out;

The address passed may exceed sizeof(struct sockaddr_ll), in which
case the true size of sll_addr is defined by sll_halen. There are
various hardware lengths well above 8B, such as INFINIBAND_ALEN.

> What is when I assign e.g. more than 8 bytes to dev->addr_len and
> copying dev->addr_len to it. Does we care about that? At least some
> assert warning if somebody try to use larger than 8 bytes dev->addr_len
> for AF_PACKET dgram sockets which might using these pointers and copy
> dev->addr_len size? As I already saw it before, but don't know what the
> best place it is to check on that.

With the recent addition to verify that sll_halen is greater than or
equal to dev->addr_len, this should now be handled correctly.

On a related note, the commit mentions dev->hard_header_len in
the context of variable hardware header lengths. This has since been
clarified. dev->hard_header_len is the maximum header length, a new
dev->min_header_len is used to validate the minimum size.

> > It is customary to return -header_len on failure in .create(), but
> > not sure what that would be here, and any negative value is treated
> > the same by callers, so returning -EINVAL.
> >
> > Is the return 0 on !ETH_P_IPV6 intentional, or should that also be
> > negative?
>
> Should be, maybe not supported. The function of a lowpan device here is
> just header "transforming". I used "transforming" here because it's still
> an IPv6 header afterwards (or more) current case is more compression
> only.
>
> I need to admit, I never tried AF_PACKET on a lowpan interface but I
> thought about it that it ends in bad things... I would like to forbid
> it, because they should use RAW IPv6 sockets where at least we already
> have code to check that we have at least a IPv6 header at
> skb_packer_header() (I hope this is how it works).
>
> Is there any way to do that?

Practically speaking, skb->sk is set by the time dev_hard_header
is called, so the packet type could be inferred.

This is a bigger problem with AF_PACKET (in particular root in userns)
and more in general validation of packets coming from userspace.
It is certainly not limited to lowpan.

User packets need not come only from PF_PACKET. They can also
come from guest kernels through a tap interface. That is potentially
more problematic, as from unprivileged sources. For that reason
blocking AF_PACKET is not a robust solution to these issues.
Though likely less relevant to lowpan, to be fair.