lkml.org 
[lkml]   [2010]   [Sep]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH] sctp: prevent reading out-of-bounds memory
    From
    Ugh, just remembered the port number is also dereferenced, so the
    second of these two checks needs to be expanded to the size of a
    sockaddr_in. Note to self: don't write patches on too little sleep.
    Apologies for the unnecessary traffic.

    Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com>

    --- linux-2.6.35.4.orig/net/sctp/socket.c 2010-09-03 08:58:48.127080114 -0400
    +++ linux-2.6.35.4/net/sctp/socket.c 2010-09-03 10:45:08.467098052 -0400
    @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st
    /* Walk through the addrs buffer and count the number of addresses. */
    addr_buf = kaddrs;
    while (walk_size < addrs_size) {
    +
    + if (walk_size + sizeof(sa_family_t) > addrs_size) {
    + kfree(kaddrs);
    + return -EINVAL;
    + }
    +
    sa_addr = (struct sockaddr *)addr_buf;
    af = sctp_get_af_specific(sa_addr->sa_family);

    @@ -1002,6 +1008,12 @@ static int __sctp_connect(struct sock* s
    /* Walk through the addrs buffer and count the number of addresses. */
    addr_buf = kaddrs;
    while (walk_size < addrs_size) {
    +
    + if (walk_size + sizeof(struct sockaddr_in) > addrs_size) {
    + err = -EINVAL;
    + goto out_free;
    + }
    +
    sa_addr = (union sctp_addr *)addr_buf;
    af = sctp_get_af_specific(sa_addr->sa.sa_family);
    port = ntohs(sa_addr->v4.sin_port);


    On Fri, Sep 3, 2010 at 10:35 AM, Dan Rosenberg
    <dan.j.rosenberg@gmail.com> wrote:
    > Ha, I knew there was an easier way.  Take two:
    >
    > Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com>
    >
    > --- linux-2.6.35.4.orig/net/sctp/socket.c       2010-09-03 08:58:48.127080114 -0400
    > +++ linux-2.6.35.4/net/sctp/socket.c    2010-09-03 10:28:14.929595312 -0400
    > @@ -916,6 +916,12 @@ SCTP_STATIC int sctp_setsockopt_bindx(st
    >        /* Walk through the addrs buffer and count the number of addresses. */
    >        addr_buf = kaddrs;
    >        while (walk_size < addrs_size) {
    > +
    > +               if (walk_size + sizeof(sa_family_t) > addrs_size) {
    > +                       kfree(kaddrs);
    > +                       return -EINVAL;
    > +               }
    > +
    >                sa_addr = (struct sockaddr *)addr_buf;
    >                af = sctp_get_af_specific(sa_addr->sa_family);
    >
    > @@ -1002,6 +1008,12 @@ static int __sctp_connect(struct sock* s
    >        /* Walk through the addrs buffer and count the number of addresses. */
    >        addr_buf = kaddrs;
    >        while (walk_size < addrs_size) {
    > +
    > +               if (walk_size + sizeof(sa_family_t) > addrs_size) {
    > +                       err = -EINVAL;
    > +                       goto out_free;
    > +               }
    > +
    >                sa_addr = (union sctp_addr *)addr_buf;
    >                af = sctp_get_af_specific(sa_addr->sa.sa_family);
    >                port = ntohs(sa_addr->v4.sin_port);
    >
    >
    >>
    >> Hm.. we already validate that we have the proper amount of space for a given sockaddr.
    >> The only thing we are missing is making sure that there is room to get the proper address
    >> family and I think you can do that without adding any extra variables:
    >>
    >>        if (walk_size + sizeof(sa_family_t) > addr_size) {
    >>                /* Not enough room for address family */
    >>                kfree(kaddrs);
    >>                return -EINVAL;
    >>        }
    >>
    >> -vlad
    >>
    >
    > On Fri, Sep 3, 2010 at 10:15 AM, Vlad Yasevich
    > <vladislav.yasevich@hp.com> wrote:
    >> On 09/03/2010 09:48 AM, Dan Rosenberg wrote:
    >>> Two user-controlled allocations in SCTP are subsequently dereferenced
    >>> as sockaddr structs, without checking if the dereferenced struct
    >>> members fall beyond the end of the allocated chunk.  There doesn't
    >>> appear to be any information leakage here based on how these members
    >>> are used and additional checking, but it's still worth fixing.
    >>>
    >>> Signed-off-by: Dan Rosenberg <dan.j.rosenberg@gmail.com>
    >>>
    >>> --- linux-2.6.35.4.orig/net/sctp/socket.c     2010-09-03 08:58:48.127080114 -0400
    >>> +++ linux-2.6.35.4/net/sctp/socket.c  2010-09-03 09:22:06.337096825 -0400
    >>> @@ -889,6 +889,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st
    >>>       int err;
    >>>       int addrcnt = 0;
    >>>       int walk_size = 0;
    >>> +     unsigned int remaining = addrs_size;
    >>>       struct sockaddr *sa_addr;
    >>>       void *addr_buf;
    >>>       struct sctp_af *af;
    >>> @@ -916,6 +917,13 @@ SCTP_STATIC int sctp_setsockopt_bindx(st
    >>>       /* Walk through the addrs buffer and count the number of addresses. */
    >>>       addr_buf = kaddrs;
    >>>       while (walk_size < addrs_size) {
    >>> +
    >>> +             /* Don't read out-of-bounds memory */
    >>> +             if (remaining < sizeof(struct sockaddr)) {
    >>> +                     kfree(kaddrs);
    >>> +                     return -EINVAL;
    >>> +             }
    >>> +
    >>>               sa_addr = (struct sockaddr *)addr_buf;
    >>>               af = sctp_get_af_specific(sa_addr->sa_family);
    >>>
    >>> @@ -929,6 +937,7 @@ SCTP_STATIC int sctp_setsockopt_bindx(st
    >>>               addrcnt++;
    >>>               addr_buf += af->sockaddr_len;
    >>>               walk_size += af->sockaddr_len;
    >>> +             remaining -= af->sockaddr_len;
    >>>       }
    >>>
    >>>       /* Do the work. */
    >>> @@ -984,6 +993,7 @@ static int __sctp_connect(struct sock* s
    >>>       void *addr_buf;
    >>>       unsigned short port;
    >>>       unsigned int f_flags = 0;
    >>> +     unsigned int remaining = addrs_size;
    >>>
    >>>       sp = sctp_sk(sk);
    >>>       ep = sp->ep;
    >>> @@ -1002,6 +1012,13 @@ static int __sctp_connect(struct sock* s
    >>>       /* Walk through the addrs buffer and count the number of addresses. */
    >>>       addr_buf = kaddrs;
    >>>       while (walk_size < addrs_size) {
    >>> +
    >>> +             /* Don't read out-of-bounds memory */
    >>> +             if (remaining < sizeof(union sctp_addr)) {
    >>> +                     err = -EINVAL;
    >>> +                     goto out_free;
    >>> +             }
    >>> +
    >>>               sa_addr = (union sctp_addr *)addr_buf;
    >>>               af = sctp_get_af_specific(sa_addr->sa.sa_family);
    >>>               port = ntohs(sa_addr->v4.sin_port);
    >>> @@ -1101,6 +1118,7 @@ static int __sctp_connect(struct sock* s
    >>>               addrcnt++;
    >>>               addr_buf += af->sockaddr_len;
    >>>               walk_size += af->sockaddr_len;
    >>> +             remaining -= af->sockaddr_len;
    >>>       }
    >>>
    >>>       /* In case the user of sctp_connectx() wants an association
    >>>
    >>
    >>
    >
    --
    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: 2010-09-03 16:51    [W:0.033 / U:178.096 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site