xorl %eax, %eax

CVE-2010-3310: Linux kernel ROSE socket Signedness Issues

with 4 comments

This vulnerability was reported by Dan Rosenberg and it affects Linux kernel prior to 2.6.36-rc5-next-20100923 release. The vulnerable code can be found at net/rose/af_rose.c which is where the ROSE socket’s implementation is. The first buggy function is the bind(2) handling routine which is shown below.

static int rose_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
{
        struct sock *sk = sock->sk;
        struct rose_sock *rose = rose_sk(sk);
        struct sockaddr_rose *addr = (struct sockaddr_rose *)uaddr;
        struct net_device *dev;
        ax25_address *source;
        ax25_uid_assoc *user;
        int n;
     ...
        if (addr->srose_ndigis > ROSE_MAX_DIGIS)
                return -EINVAL;

ROSE socket’s bind(2) implementation will check among other things that that ‘srose_ndigis’ isn’t greater than the constant ‘ROSE_MAX_DIGIS’. For better understanding of this have a look at include/linux/rose.h where ‘sockaddr_rose’ structure is defined…

#define ROSE_MAX_DIGIS 6
     ...
struct sockaddr_rose {
        sa_family_t     srose_family;
        rose_address    srose_addr;
        ax25_address    srose_call;
        int             srose_ndigis;
        ax25_address    srose_digi;
};

This means that ‘srose_ndigis’ signed integer will be checked against the constant value ‘ROSE_MAX_DIGIS’ in rose_bind() but since this value is user controlled it could contain a negative integer that will bypass this check. If we move back to rose_back() from where we stopped we can find the following code…

     ...
        rose->source_ndigis = addr->srose_ndigis;

        if (addr_len == sizeof(struct full_sockaddr_rose)) {
                struct full_sockaddr_rose *full_addr = (struct full_sockaddr_rose *)uaddr;
                for (n = 0 ; n < addr->srose_ndigis ; n++)
                        rose->source_digis[n] = full_addr->srose_digis[n];
        } else {
                if (rose->source_ndigis == 1) {
                        rose->source_digis[0] = addr->srose_digi;
                }
        }
     ...
	return 0;
}

If this is a ‘full_sockaddr_rose’ structure the first part of the ‘if’ clause will be executed. This will copy each ‘full_addr->srose_digis[]’ member to ‘rose->source_digis’ using the invalid ‘addr->srose_ndigis’ integer as a counter. But this is probably not our case since this code is for ‘full_sockaddr_rose’ structures that contain an array of AX.25 addresses…

struct full_sockaddr_rose {
        sa_family_t     srose_family;
        rose_address    srose_addr;
        ax25_address    srose_call;
        unsigned int    srose_ndigis;
        ax25_address    srose_digis[ROSE_MAX_DIGIS];
};

which means that it would have different size (it also defines ‘srose_ndigis’ as unsigned integer but that doesn’t change the structure’s size). The code will not fall into the ‘else’ part either since this only affects ‘srose_ndigis’ equal to 1. So, as Dan Rosenberg pointed out, by calling a function similar to rose_getname() you’ll execute the following…

static int rose_getname(struct socket *sock, struct sockaddr *uaddr,
        int *uaddr_len, int peer)
{
        struct full_sockaddr_rose *srose = (struct full_sockaddr_rose *)uaddr;
        struct sock *sk = sock->sk;
        struct rose_sock *rose = rose_sk(sk);
        int n;
     ...
        if (peer != 0) {
                if (sk->sk_state != TCP_ESTABLISHED)
                        return -ENOTCONN;
     ...
                srose->srose_ndigis = rose->dest_ndigis;
                for (n = 0; n < rose->dest_ndigis; n++)
                        srose->srose_digis[n] = rose->dest_digis[n];
        } else {
     ...
                srose->srose_ndigis = rose->source_ndigis;
                for (n = 0; n < rose->source_ndigis; n++)
                        srose->srose_digis[n] = rose->source_digis[n];
        }
     ...
}

This means that in case of an established TCP state the user controlled ‘rose->source_ndigis’ signed integer will be used as a counter to the ‘for’ loop that attempts to copy each entry of one array to the other. As you saw earlier, ‘full_sockaddr_rose’ structure has an array of ‘ROSE_MAX_DIGIS’ (which means 6) AX.25 address structures. However, in this ‘for’ loop a negative ‘rose->source_ndigis’ will result in copying much more than that leading to memory corruption.
To fix this, the patch was to change the size check by casting the signed integer to an unsigned one.

-       if (addr->srose_ndigis > ROSE_MAX_DIGIS)
+       if ((unsigned int) addr->srose_ndigis > ROSE_MAX_DIGIS)
                return -EINVAL;

A similar vulnerability was also present in the connect(2) implementation of that socket as you can see here:

static int rose_connect(struct socket *sock, struct sockaddr *uaddr, int addr_len, int flags)
{
        struct sock *sk = sock->sk;
        struct rose_sock *rose = rose_sk(sk);
        struct sockaddr_rose *addr = (struct sockaddr_rose *)uaddr;
        unsigned char cause, diagnostic;
        struct net_device *dev;
        ax25_uid_assoc *user;
        int n, err = 0;
     ...
        if (addr->srose_ndigis > ROSE_MAX_DIGIS)
                return -EINVAL;
     ...
        return err;
}

Which was patched using the same casting to unsigned integer approach.
Regarding its exploitation, D. Rosenberg says that this can be triggered by unprivileged users only when a ROSE device is available in the system.

Written by xorl

October 16, 2010 at 11:30

Posted in bugs, linux

4 Responses

Subscribe to comments with RSS.

  1. Let us see if i got it right. In the check:

    if (addr->srose_ndigis > ROSE_MAX_DIGIS) return -EINVAL;

    The user should have “send” a value of srose_ndigits that is negative, for the check is to be done as if both values where signed integers, right? Then it comes to this snip:

    srose->srose_ndigis = rose->dest_ndigis;
    for (n = 0; n dest_ndigis; n++)
    srose->srose_digis[n] = rose->dest_digis[n];

    And being sroes_ndigis unsigned, the implicit casts changes that negative number introduced by the user to a big positive one, and the memory corruption occurs. Am I wrong?

    Thanks!!

    pit

    October 18, 2010 at 07:46

  2. The check will be bypassed if ‘addr->srose_ndigis’ is negative exactly as you’ve just said.
    The ‘for’ loop you’re talking about will not be reached since this affects structures with size of: sizeof(struct full_sockaddr_rose) where our structure is sizeof(struct struct sockaddr_rose).
    So, to trigger it you can use another function (rose_getname()) that will execute:

    srose->srose_ndigis = rose->source_ndigis;
    for (n = 0; n source_ndigis; n++)
    srose->srose_digis[n] = rose->source_digis[n];

    That uses the negative ‘srose_ndigis’ value as an index.

    xorl

    October 18, 2010 at 08:19

  3. Ah, ok. Anyway, the loop is mainly the same and the boundary checks ae the same. Now I see that the problem is when the structures are “casted”, because the ndigis is defined as integer in one of them and unsigned on the other, thus converting our initially negative into a big positive, right?

    Thank you very much.

    pit

    October 18, 2010 at 11:19

  4. Another nice write-up. I have a minor correction though: in rose_bind(), a negative value for the srose_ndigis field will bypass the check, and then it will be assigned to the source_ndigis field of the rose_sock struct, which is an unsigned char field. As a result, it is truncated to a single byte, giving us a maximum value of 0xff (255), which allows an overflow because the destination array contains only 6 elements. This is actually much better for exploitation, since a negative value (which would be cast to a very large positive value) would overwrite too much of the kernel heap to keep the system from crashing.

    Dan Rosenberg

    October 18, 2010 at 22:43


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s