Re: [PATCH] getsockopt() early argument sanity checking

Solar Designer
2006-08-21T08:28:19Z

(I realize that the patch has been rejected and this message is in no
way meant to affect that decision.)

On Mon, Aug 21, 2006 at 03:00:22AM +0000, David Wagner wrote:
> Solar Designer wrote:
> >The patch makes getsockopt(2) sanity-check the value pointed to by
> >the optlen argument early on. This is a security hardening measure
> >intended to prevent exploitation of certain potential vulnerabilities in
> >socket type specific getsockopt() code on UP systems.
>
> This looks broken to me. It has a TOCTTOU (time-of-check-to-time-of-use)

Yes it does, using Matt Bishop's classification.

> vulnerability (i.e., race condition): you read the length value twice,
> and assume that you will get the same value both times. That assumption
> is not valid.

I don't assume that. I realize that there's the race condition on SMP
and, as pointed out by Andi Kleen, in many cases also on UP systems.
That's why I had the XXX comment in there from the very beginning.

This added check is not supposed to be relied upon; rather, it is a
hardening measure in case we miss a bug in underlying *getsockopt()
functions.

> It looks like it will be easy to bypass this check.

It depends. You might have missed this description of a special case
where it does not appear to be possible to bypass the check:

http://lkml.org/lkml/2006/8/20/148

Yes, the patch is highly controversial and I mostly agree with its
opponents (I had much of the same thoughts myself, except for DaveM's
concern that *optlen might be uninitialized or negative on purpose),
yet I am going to keep it in -ow.

BTW, I had previously submitted a very similar check to do_sysctl(),
also with an XXX comment, which got in a few years back.

I won't be surprised if one of these checks saves a system from a
compromise one day. This world is not perfect - neither the rest of the
Linux kernel code nor vulnerability exploit programs are perfect.

Alexander

P.S. Please CC me on your replies.