wireless: fix 64K kernel heap content leak via ioctl

This problem was originally tracked down by Brad Spengler.

When calling wireless ioctls, if a driver does not correctly validate/shrink iwp->length, the resulting copy_to_user can leak up to 64K of kernel heap contents.

It seems that this is triggerable[1] in 2.6.32 at least on ath5k, but I was not able to track down how. The twisty maze of ioctl handlers stumped me. :)

Other drivers I checked did not appear to have any problems,
but the potential remains. I'm not sure if this patch is the right approach; it was fixed differently[2] in grsecurity.

[1] http://forums.grsecurity.net/viewtopic.php?f=3&t=2290&start=0
[2] http://grsecurity.net/~spender/wireless-infoleak-fix2.patch

Quoted 9 lines · Kees Cook

This problem was originally tracked down by Brad Spengler.

When calling wireless ioctls, if a driver does not correctly validate/shrink iwp->length, the resulting copy_to_user can leak up to 64K of kernel heap contents.

It seems that this is triggerable[1] in 2.6.32 at least on ath5k, but I was not able to track down how. The twisty maze of ioctl handlers stumped me. :)

You can always ask.

Quoted 3 lines

Other drivers I checked did not appear to have any problems,
but the potential remains. I'm not sure if this patch is the right approach; it was fixed differently[2] in grsecurity.

Did you tried your patch for real ? With large scan request ? I ask because at first glance, it looks incorrect, asI believe
it kills large request. But someone would need to test, for sure.

Quoted 2 lines

[1] http://forums.grsecurity.net/viewtopic.php?f=3&t=2290&start=0
[2] http://grsecurity.net/~spender/wireless-infoleak-fix2.patch

I believe this patch would make the situation worse.

Would you mind validating the following patch ? I've just
verified that it compiles and I believe it does what you are asking in a much more predictable way.

Hi Jean,

The comment should probably be clarified -- it's the caller's iwp->length that may be causing problems

Regardless, the above patch would appear to limit the copy_to_user to only the kzalloced region.

Thanks!

-Kees

Quoted 9 lines · Kees Cook

This problem was originally tracked down by Brad Spengler.

When calling wireless ioctls, if a driver does not correctly validate/shrink iwp->length, the resulting copy_to_user can leak up to 64K of kernel heap contents.

It seems that this is triggerable[1] in 2.6.32 at least on ath5k, but I was not able to track down how. The twisty maze of ioctl handlers stumped me. :)

       You can always ask.

Quoted 3 lines

Other drivers I checked did not appear to have any problems,
but the potential remains. I'm not sure if this patch is the right approach; it was fixed differently[2] in grsecurity.

       Did you tried your patch for real ? With large scan request ? I ask because at first glance, it looks incorrect, asI believe it kills large request. But someone would need to test, for sure.

Quoted 2 lines

[1] http://forums.grsecurity.net/viewtopic.php?f=3&t=2290&start=0
[2] http://grsecurity.net/~spender/wireless-infoleak-fix2.patch

       I believe this patch would make the situation worse.

       Would you mind validating the following patch ? I've just verified that it compiles and I believe it does what you are asking in a much more predictable way.
       Regards,

       Jean

diff -u -p wext.j2.c wext.c

wext.c 2010-08-27 14:19:33.000000000 -0700 renamed from wext.j2.c 2010-08-27 14:17:26.000000000 -0700 +0 -2

@@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
                       goto out;
               }
 
-               if (copy_to_user(iwp->pointer, extra,
-                                iwp->length *

Quoted 8 lines

diff -u -p wext.j2.c wext.c

wext.c 2010-08-27 14:19:33.000000000 -0700 renamed from wext.j2.c 2010-08-27 14:17:26.000000000 -0700 +0 -0

@@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
                       goto out;
               }
 

wext.c 2010-08-27 14:19:33.000000000 -0700 static int ioctl_standard_iw_point(struc · line 811

Quoted 12 lines

wext.c 2010-08-27 14:19:33.000000000 -0700 renamed from wext.j2.c 2010-08-27 14:17:26.000000000 -0700 +6 -3

-               if (copy_to_user(iwp->pointer, extra,
-                                iwp->length *
-                                descr->token_size)) {
+               /* Verify how much we should return. Some driver
+                * may abuse iwp->length... */
+               if((iwp->length * descr->token_size) < extra_size)
+                       extra_size = iwp->length * descr->token_size;
+
+               if (copy_to_user(iwp->pointer, extra, extra_size)) {
                       err = -EFAULT;
                       goto out;
               }

Jean, can you submit in a new thread and right before the SOB add in the commit log Cc: [email protected] [2.6.32+]

Luis

Quoted 8 lines · Luis R. Rodriguez

Quoted 8 lines

diff -u -p wext.j2.c wext.c

wext.c      2010-08-27 14:19:33.000000000 -0700 renamed from wext.j2.c   2010-08-27 14:17:26.000000000 -0700 +0 -0

@@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
                        goto out;
                }
 

Quoted 16 lines

wext.c      2010-08-27 14:19:33.000000000 -0700 static int ioctl_standard_iw_point(struc · line 811

Quoted 12 lines

wext.c      2010-08-27 14:19:33.000000000 -0700 renamed from wext.j2.c   2010-08-27 14:17:26.000000000 -0700 +6 -3

-               if (copy_to_user(iwp->pointer, extra,
-                                iwp->length *
-                                descr->token_size)) {
+               /* Verify how much we should return. Some driver
+                * may abuse iwp->length... */
+               if((iwp->length * descr->token_size) < extra_size)
+                       extra_size = iwp->length * descr->token_size;
+
+               if (copy_to_user(iwp->pointer, extra, extra_size)) {
                        err = -EFAULT;
                        goto out;
                }

Jean, can you submit in a new thread and right before the SOB add in the commit log Cc: [email protected] [2.6.32+]

       The current patch was made for 2.6.27 and was only compiled. Someone would need to verify it works for 2.6.32. I could probably find some time next week.

Would you mind validating the following patch ? I've just
verified that it compiles and I believe it does what you are asking in a much more predictable way.
Regards,

(quoted diff) +0 -1

@@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
                        goto out;
                }
 
-               if (copy_to_user(iwp->pointer, extra,

Quoted 11 lines · Jean Tourrilhes

Would you mind validating the following patch ? I've just
verified that it compiles and I believe it does what you are asking in a much more predictable way.
Regards,

(quoted diff) +0 -1

@@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
                        goto out;
                }
 
-               if (copy_to_user(iwp->pointer, extra,

Review static int ioctl_standard_iw_point(struc · line 810

Quoted 10 lines

(quoted diff) +6 -2

-                                iwp->length *
-                                descr->token_size)) {
+               /* Verify how much we should return. Some driver
+                * may abuse iwp->length... */
+               if((iwp->length * descr->token_size) < extra_size)
+                       extra_size = iwp->length * descr->token_size;
+
+               if (copy_to_user(iwp->pointer, extra, extra_size)) {
                        err = -EFAULT;
                        goto out;

Based on the code _before_ this hunk, I believe this patch to be wrong (the goto out matches):

        /* If we have something to return to the user */
        if (!err && IW_IS_GET(cmd)) {
                /* Check if there is enough buffer up there */
                if (user_length < iwp->length) {
                        err = -E2BIG;
                        goto out;
                }

Thus, apparently drivers were intended to be allowed to return more information than userspace had allocated space for (which also matches the initial extra_size calculation in this function), so your comment is wrong, and your check is also wrong because you actually put the burden on the driver, contrary to the apparent intention of this code.

I believe the below patch is a much better fix as it allows the -E2BIG code path to be invoked which is more informative to users than truncated information (which, in your code, may even be truncated in the middle of a token!!)

Quoted 4 lines

Quoted 4 lines

(quoted diff) +0 -0

@@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
                        goto out;
                }
 

Quoted 11 lines

Review static int ioctl_standard_iw_point(struc · line 810

Quoted 11 lines

(quoted diff) +6 -3

-               if (copy_to_user(iwp->pointer, extra,
-                                iwp->length *
-                                descr->token_size)) {
+               /* Verify how much we should return. Some driver
+                * may abuse iwp->length... */
+               if((iwp->length * descr->token_size) < extra_size)
+                       extra_size = iwp->length * descr->token_size;
+
+               if (copy_to_user(iwp->pointer, extra, extra_size)) {
                        err = -EFAULT;
                        goto out;

Quoted 4 lines

I believe the below patch is a much better fix as it allows the -E2BIG code path to be invoked which is more informative to users than truncated information (which, in your code, may even be truncated in the middle of a token!!)

Hi Johannes,

Quoted 6 lines · Johannes Berg

net/wireless/wext-compat.c +0 -0

@@ -1420,6 +1420,9 @@ int cfg80211_wext_giwessid(struct net_de
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 

net/wireless/wext-compat.c int cfg80211_wext_giwessid(struct net_de · line 1428

Quoted 6 lines

net/wireless/wext-compat.c +3 -0

+	data->flags = 0;
+	data->length = 0;
+
 	switch (wdev->iftype) {
 	case NL80211_IFTYPE_ADHOC:
 		return cfg80211_ibss_wext_giwessid(dev, info, data, ssid);

Thanks for all your work on this! Were you able to trigger the leak through cfg80211? If so, then this will need a CVE assigned and sent to stable too, I think.

Ok I finally fully understood the issue.

This will fix the problem, but the comment is completely bogus, which I guess means you didn't actually understand the problem.

The comment should probably be clarified -- it's the caller's iwp->length that may be causing problems

My patch also didn't fix the problem, I didn't understand the problem correctly and was continuously wondering how drivers would ever fill the buffer with more than max_tokens (which would be a more serious bug, since they'd overwrite a slab object after "extra").

What really fixes the problem is the patch below though. Had to realise that the path where the driver didn't do ANYTHING AT ALL was the problem....

johannes

Ha ! I see. It would be for regular iwpoint queries, not for
extended NOMAX queries (scan is a extended NOMAX query).

I actually like your patch better than mine, it's closer to
the original intent of the API. Go for it ;-)

Thanks a lot for the second pair of eyes.

This problem was originally tracked down by Brad Spengler.

When calling wireless ioctls, if a driver does not correctly validate/shrink iwp->length, the resulting copy_to_user can leak up to 64K of kernel heap contents.

It seems that this is triggerable[1] in 2.6.32 at least on ath5k, but I was not able to track down how. The twisty maze of ioctl handlers stumped me. :) Other drivers I checked did not appear to have any problems, but the potential remains. I'm not sure if this patch is the right approach; it was fixed differently[2] in grsecurity.

[1] http://forums.grsecurity.net/viewtopic.php?f=3&t=2290&start=0
[2] http://grsecurity.net/~spender/wireless-infoleak-fix2.patch

Quoted 12 lines · Kees Cook

This problem was originally tracked down by Brad Spengler.

When calling wireless ioctls, if a driver does not correctly validate/shrink iwp->length, the resulting copy_to_user can leak up to 64K of kernel heap contents.

It seems that this is triggerable[1] in 2.6.32 at least on ath5k, but I was not able to track down how. The twisty maze of ioctl handlers stumped me. :) Other drivers I checked did not appear to have any problems, but the potential remains. I'm not sure if this patch is the right approach; it was fixed differently[2] in grsecurity.

Quoted 2 lines

[1] http://forums.grsecurity.net/viewtopic.php?f=3&t=2290&start=0
[2] http://grsecurity.net/~spender/wireless-infoleak-fix2.patch

Is this fixed differently upstream in the kernel with commit id 42da2f948d949efd0111309f5827bf0298bcc9a4?

Quoted 13 lines · Greg KH

Quoted 12 lines · Kees Cook

This problem was originally tracked down by Brad Spengler.

When calling wireless ioctls, if a driver does not correctly validate/shrink iwp->length, the resulting copy_to_user can leak up to 64K of kernel heap contents.

It seems that this is triggerable[1] in 2.6.32 at least on ath5k, but I was not able to track down how. The twisty maze of ioctl handlers stumped me. :) Other drivers I checked did not appear to have any problems, but the potential remains. I'm not sure if this patch is the right approach; it was fixed differently[2] in grsecurity.

Quoted 5 lines

Quoted 2 lines

[1] http://forums.grsecurity.net/viewtopic.php?f=3&t=2290&start=0
[2] http://grsecurity.net/~spender/wireless-infoleak-fix2.patch

Is this fixed differently upstream in the kernel with commit id 42da2f948d949efd0111309f5827bf0298bcc9a4?

Yes, that's the fix for this.