netdev - Re: [patch v2] net: heap overflow in __audit

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]

Date:	Wed, 27 Nov 2013 22:18:23 +0100
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Eric Wong <normalperson@...t.net>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	"David S. Miller" <davem@...emloft.net>,
	Network Development <netdev@...r.kernel.org>,
	"security@...nel.org" <security@...nel.org>,
	Jüri Aedla <juri.aedla@...il.com>,
	"# .39.x" <stable@...nel.org>
Subject: Re: [patch v2] net: heap overflow in __audit_sockaddr()

On Wed, Nov 27, 2013 at 12:24:41PM -0800, Linus Torvalds wrote:
> On Wed, Nov 27, 2013 at 3:51 AM, Hannes Frederic Sowa
> <hannes@...essinduktion.org> wrote:
> >
> > We have to clamp msg_namelen to max sizeof(struct sockaddr_storage).
> > The sendmsg handler will check msg_namelen again and error out correctly if
> > the size of msg_name is too short.
> 
> Yeah, clamping sounds like the right thing to do at least for
> receiving. For sending, you should say "we can't send packets that big
> due to memory constraints" (of, for the case of a sockaddr, "to an
> address this big"), but for receiving the size of the user space
> buffer is kind of irrelevant - if the user gives a bigger buffer than
> necessary, who cares? We just need to make sure that the kernel
> doesn't then allocate silly-big temporary buffers internally.
>
> There seems to be a patch floating around to clamp things already.

Data buffers are clamed fine already.

The sockaddr buffers are currently always 128 bytes (== sizeof(struct
sockaddr_storage)) in size and are allocated on the stack of the
recvmsg/sendmsg syscall handlers. We normally don't have high stack usage
on recvmsg calls but it could be worth trying to optimize that on sendmsg,
I agree. I have not seen a patch which tries to do so but maybe I haven't
looked far enough back in the mailing list archives.

Clamping on sending seems necessary to not break exisiting applications. I
guess those programs expect the kernel to know which namelen the
protocol expects and only use that part of the provided sockaddr
buffer (an example is the mentioned ruby bug in this thread which
seems to pass down the size of a union for all possile sockaddrs:
<https://bugs.ruby-lang.org/issues/9124#note-2>).

I recently noticed a some more subtile annoyance in that code:

We don't know the anticipated sockaddr size before calling the recvmsg
handler. Hence it is currently possible that we dequeue a packet from
the socket receiving queue and later error out and drop the packet because
the user provided a socket address buffer which is too small. IMHO we
should catch that before dequeueing the packet. Either we can export the
address size via the per-protocol-structures or we have to start passing the
user provided buffer sizes down the stack (currently all recvmsg handlers
expect to always have 128 bytes for storing addresses).

I'll look into this.

Greetings,

  Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html