lkml.org 
[lkml]   [2006]   [Aug]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] getsockopt() early argument sanity checking
    On Sun, Aug 20, 2006 at 10:34:43AM +0200, Andi Kleen wrote:
    > On Sunday 20 August 2006 01:05, Solar Designer wrote:
    > > I propose the attached patch (extracted from 2.4.33-ow1) for inclusion
    > > into 2.4.34-pre.
    > >
    > > (2.6 kernels could benefit from the same change, too, but at the moment
    > > I am dealing with proper submission of generic changes like this that
    > > are a part of 2.4.33-ow1.)
    >
    > In general I don't think it makes sense to submit stuff for 2.4
    > that isn't in 2.6.

    I generally agree with you on this, but I think that when they are just
    preventive measures, they can be applied in whatever order, provided that
    they don't get lost.

    > > 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.
    >
    > It's not only insufficient on SMP, but even on UP where a thread
    > can sleep in get_user and another one can run in this time.

    Valid point.

    > Doing a check that is inherently racy everywhere doesn't seem like
    > a security improvement to me. If there is really a length checking bug somewhere
    > it needs to be fixed in a race-free way.

    The only race-free solution would be to add an argument to all getsockopt()
    functions and pass them the decoded value. There are other places where
    multiple get_user() are performed on optlen, with some useless tests (eg
    in net/ipv4/tcp.c, len is checked for <0 after having been compared to
    sizeof(int) as unsigned int) and all those places would benefit from this.

    But I don't want to induce such large changes in this kernel. The goal of
    this test is a preventive measure to catch easily exploitable errors that
    might have remained undetected. For instance, a quick glance shows this
    portion of code in net/ipv4/raw.c (both 2.4 and 2.6) :

    static int raw_seticmpfilter(struct sock *sk, char *optval, int optlen)
    {
    if (optlen > sizeof(struct icmp_filter))
    optlen = sizeof(struct icmp_filter);
    if (copy_from_user(&sk->tp_pinfo.tp_raw4.filter, optval, optlen))
    return -EFAULT;
    return 0;
    }

    It only relies on sock_setsockopt() refusing optlen values < sizeof(int),
    and this is not documented. Having part of this code being copied for use
    in another code path would open a breach for optlen < 0.

    > If not then there is no need for a change.

    There are two tests in this patch :

    - one on the validity of the optlen address. This one is race-free and
    should be conserved anyway.

    - one on the optlen range which is valid for most cases but which is
    subject to a race condition and which might be circumvented by
    carefully written code and with some luck as in all race conditions
    issues.

    Some will say that this last one offers a first level of protection by
    transforming determinist attacks into racy attacks which make potential
    vulnerabilities much harder to exploit by code injection. I'm one of them.

    Others will consider it totally useless because it does not cover all cases,
    but I think it is against the general principle of precaution we try to
    apply in security.

    > -Andi

    Regards,
    Willy

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-08-20 12:19    [W:0.026 / U:90.364 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site