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
Reported-by: Brad Spengler <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
include/net/iw_handler.h | 1 -
net/wireless/wext-core.c | 26 ++------------------------
2 files changed, 2 insertions(+), 25 deletions(-)
diff --git a/include/net/iw_handler.h b/include/net/iw_handler.h
index 3afdb21..6c81f29 100644
--- a/include/net/iw_handler.h
+++ b/include/net/iw_handler.h
@@ -277,7 +277,6 @@
#define IW_DESCR_FLAG_EVENT 0x0002 /* Generate an event on SET */
#define IW_DESCR_FLAG_RESTRICT 0x0004 /* GET : request is ROOT only */
/* SET : Omit payload from generated iwevent */
-#define IW_DESCR_FLAG_NOMAX 0x0008 /* GET : no limit on request size */
/* Driver level flags */
#define IW_DESCR_FLAG_WAIT 0x0100 /* Wait for driver event */
diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
index 0ef17bc..55b1fd9 100644
--- a/net/wireless/wext-core.c
+++ b/net/wireless/wext-core.c
@@ -82,7 +82,6 @@ static const struct iw_ioctl_description standard_ioctl[] = {
.header_type = IW_HEADER_TYPE_POINT,
.token_size = sizeof(struct iw_priv_args),
.max_tokens = 16,
- .flags = IW_DESCR_FLAG_NOMAX,
},
[IW_IOCTL_IDX(SIOCSIWSTATS)] = {
.header_type = IW_HEADER_TYPE_NULL,
@@ -134,7 +133,6 @@ static const struct iw_ioctl_description standard_ioctl[] = {
.token_size = sizeof(struct sockaddr) +
sizeof(struct iw_quality),
.max_tokens = IW_MAX_AP,
- .flags = IW_DESCR_FLAG_NOMAX,
},
[IW_IOCTL_IDX(SIOCSIWSCAN)] = {
.header_type = IW_HEADER_TYPE_POINT,
@@ -146,7 +144,6 @@ static const struct iw_ioctl_description standard_ioctl[] = {
.header_type = IW_HEADER_TYPE_POINT,
.token_size = 1,
.max_tokens = IW_SCAN_MAX_DATA,
- .flags = IW_DESCR_FLAG_NOMAX,
},
[IW_IOCTL_IDX(SIOCSIWESSID)] = {
.header_type = IW_HEADER_TYPE_POINT,
@@ -737,28 +734,9 @@ static int ioctl_standard_iw_point(struct iw_point *iwp, unsigned int cmd,
return -EFAULT;
/* Save user space buffer size for checking */
user_length = iwp->length;
-
- /* Don't check if user_length > max to allow forward
- * compatibility. The test user_length < min is
- * implied by the test at the end.
- */
-
- /* Support for very large requests */
- if ((descr->flags & IW_DESCR_FLAG_NOMAX) &&
- (user_length > descr->max_tokens)) {
- /* Allow userspace to GET more than max so
- * we can support any size GET requests.
- * There is still a limit : -ENOMEM.
- */
- extra_size = user_length * descr->token_size;
-
- /* Note : user_length is originally a __u16,
- * and token_size is controlled by us,
- * so extra_size won't get negative and
- * won't overflow...
- */
- }
}
+ /* Support for very large requests */
+ extra_size = max(extra_size, iwp->length * descr->token_size);
/* kzalloc() ensures NULL-termination for essid_compat. */
extra = kzalloc(extra_size, GFP_KERNEL);
--
1.7.1
--
Kees Cook
Ubuntu Security Team
On Fri, Aug 27, 2010 at 02:02:41PM -0700, Kees Cook wrote:
> 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.
> 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.
> [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
Signed-off-by: Jean Tourrilhes <[email protected]>
diff -u -p wext.j2.c wext.c
--- wext.j2.c 2010-08-27 14:17:26.000000000 -0700
+++ wext.c 2010-08-27 14:19:33.000000000 -0700
@@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
goto out;
}
- 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;
}
Hi Jean,
On Fri, Aug 27, 2010 at 02:22:54PM -0700, Jean Tourrilhes wrote:
> On Fri, Aug 27, 2010 at 02:02:41PM -0700, Kees Cook wrote:
> > 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.
>
> > 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.
I did not, no. Since I couldn't reproduce the original problem (I lacked
the hardware to test ath5k, and all the other drivers correctly managed
iwp->length).
> > [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.
> - 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)) {
The comment should probably be clarified -- it's the caller's iwp->length
that may be causing problems (when combined with a driver that forgets to
adjusted iwp->length downward). Regardless, the above patch would appear to
limit the copy_to_user to only the kzalloced region.
Thanks!
-Kees
--
Kees Cook
Ubuntu Security Team
On Fri, Aug 27, 2010 at 02:43:57PM -0700, Kees Cook wrote:
> Hi Jean,
>
> The comment should probably be clarified -- it's the caller's iwp->length
> that may be causing problems
Ha ! I see. It would be for regular iwpoint queries, not for
extended NOMAX queries (scan is a extended NOMAX query).
Note that I don't like the idea of reducing the mallocated
size, especially with regular queries, as I know that some driver may
expect a fixed size in extra and may memcpy to it without double
checking.
> Regardless, the above patch would appear to limit the copy_to_user
> to only the kzalloced region.
I'm glad you like it.
> Thanks!
>
> -Kees
Regards,
Jean
On Fri, Aug 27, 2010 at 2:22 PM, Jean Tourrilhes <[email protected]> wrote:
> On Fri, Aug 27, 2010 at 02:02:41PM -0700, Kees Cook wrote:
>> 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.
>
>> 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.
>
>> [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.j2.c 2010-08-27 14:17:26.000000000 -0700
> +++ wext.c 2010-08-27 14:19:33.000000000 -0700
> @@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
> goto out;
> }
>
> - 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
On Fri, Aug 27, 2010 at 03:35:19PM -0700, Luis R. Rodriguez wrote:
> >
> > diff -u -p wext.j2.c wext.c
> > --- wext.j2.c 2010-08-27 14:17:26.000000000 -0700
> > +++ wext.c 2010-08-27 14:19:33.000000000 -0700
> > @@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
> > goto out;
> > }
> >
> > - 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.
> Luis
Regards,
Jean
On Fri, Aug 27, 2010 at 3:39 PM, Jean Tourrilhes <[email protected]> wrote:
> On Fri, Aug 27, 2010 at 03:35:19PM -0700, Luis R. Rodriguez wrote:
>> >
>> > diff -u -p wext.j2.c wext.c
>> > --- wext.j2.c 2010-08-27 14:17:26.000000000 -0700
>> > +++ wext.c 2010-08-27 14:19:33.000000000 -0700
>> > @@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
>> > goto out;
>> > }
>> >
>> > - 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.
Got it, ah so it would be Cc: [email protected] [2.6.27+]. To get this
trickled in we first need it for wireless-testing.git, and provide
links / patches to the backport of the patch for each kernel. Once it
gets merged into Linus' tree the stable team can apply the respective
backported patches.
Luis
On Fri, 2010-08-27 at 14:22 -0700, Jean Tourrilhes wrote:
> 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,
> @@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
> goto out;
> }
>
> - 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;
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!!)
johannes
---
net/wireless/wext-core.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
--- wireless-testing.orig/net/wireless/wext-core.c 2010-08-30 10:35:33.000000000 +0200
+++ wireless-testing/net/wireless/wext-core.c 2010-08-30 10:46:45.000000000 +0200
@@ -738,26 +738,23 @@ static int ioctl_standard_iw_point(struc
/* Save user space buffer size for checking */
user_length = iwp->length;
- /* Don't check if user_length > max to allow forward
- * compatibility. The test user_length < min is
- * implied by the test at the end.
- */
-
/* Support for very large requests */
- if ((descr->flags & IW_DESCR_FLAG_NOMAX) &&
- (user_length > descr->max_tokens)) {
+ if (descr->flags & IW_DESCR_FLAG_NOMAX) {
/* Allow userspace to GET more than max so
* we can support any size GET requests.
- * There is still a limit : -ENOMEM.
- */
- extra_size = user_length * descr->token_size;
-
- /* Note : user_length is originally a __u16,
+ * There is still a limit: 64k records of
+ * token_size (since a u16 is used).
+ *
+ * Note: user_length is originally a u16,
* and token_size is controlled by us,
* so extra_size won't get negative and
* won't overflow...
*/
- }
+ if (user_length > descr->max_tokens) {
+ extra_size = user_length * descr->token_size;
+ } else
+ user_length = min_t(int, user_length,
+ descr->max_tokens);
}
/* kzalloc() ensures NULL-termination for essid_compat. */
On Mon, 2010-08-30 at 10:47 +0200, Johannes Berg wrote:
> On Fri, 2010-08-27 at 14:22 -0700, Jean Tourrilhes wrote:
>
> > 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,
>
>
> > @@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
> > goto out;
> > }
> >
> > - 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;
>
> 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!!)
err, forgot quilt refresh, here's the right patch:
---
net/wireless/wext-core.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
--- wireless-testing.orig/net/wireless/wext-core.c 2010-08-30 10:35:33.000000000 +0200
+++ wireless-testing/net/wireless/wext-core.c 2010-08-30 10:57:38.000000000 +0200
@@ -738,26 +738,23 @@ static int ioctl_standard_iw_point(struc
/* Save user space buffer size for checking */
user_length = iwp->length;
- /* Don't check if user_length > max to allow forward
- * compatibility. The test user_length < min is
- * implied by the test at the end.
- */
-
/* Support for very large requests */
- if ((descr->flags & IW_DESCR_FLAG_NOMAX) &&
- (user_length > descr->max_tokens)) {
+ if (descr->flags & IW_DESCR_FLAG_NOMAX) {
/* Allow userspace to GET more than max so
* we can support any size GET requests.
- * There is still a limit : -ENOMEM.
- */
- extra_size = user_length * descr->token_size;
-
- /* Note : user_length is originally a __u16,
+ * There is still a limit: 64k records of
+ * token_size (since a u16 is used).
+ *
+ * Note: user_length is originally a u16,
* and token_size is controlled by us,
* so extra_size won't get negative and
* won't overflow...
*/
- }
+ if (user_length > descr->max_tokens)
+ extra_size = user_length * descr->token_size;
+ } else
+ user_length = min_t(int, user_length,
+ descr->max_tokens);
}
/* kzalloc() ensures NULL-termination for essid_compat. */
On Mon, 2010-08-30 at 10:58 +0200, Johannes Berg wrote:
> > > @@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
> > > goto out;
> > > }
> > >
> > > - 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;
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.
> > 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!!)
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
Subject: wireless extensions: fix kernel heap content leak
From: Johannes Berg <[email protected]>
Wireless extensions have an unfortunate, undocumented
requirement which requires drivers to always fill
iwp->length when returning a successful status. When
a driver doesn't do this, it leads to a kernel heap
content leak when userspace offers a larger buffer
than would have been necessary.
Arguably, this is a driver bug, as it should, if it
returns 0, fill iwp->length, even if it separately
indicated that the buffer contents was not valid.
However, we can also remove this requirement and
avoid this class of driver bugs by setting the iwp
length to max_tokens, which then reflects how big
the buffer is that the driver may fill, regardless
of how big the userspace buffer is.
To illustrate the point, this patch also fixes a
corresponding cfg80211 bug (since this requirement
isn't documented nor was ever pointed out by anyone
during code review, I don't trust all drivers nor
all cfg80211 handlers to implement it correctly).
Signed-off-by: Johannes Berg <[email protected]>
---
net/wireless/wext-compat.c | 3 +++
net/wireless/wext-core.c | 16 ++++++++++++++++
2 files changed, 19 insertions(+)
--- wireless-testing.orig/net/wireless/wext-core.c 2010-08-30 11:29:26.000000000 +0200
+++ wireless-testing/net/wireless/wext-core.c 2010-08-30 11:57:41.000000000 +0200
@@ -782,6 +782,22 @@ static int ioctl_standard_iw_point(struc
}
}
+ if (IW_IS_GET(cmd) && !(descr->flags & IW_DESCR_FLAG_NOMAX)) {
+ /*
+ * If this is a GET, but not NOMAX, it means that the extra
+ * data is not bounded by userspace, but by max_tokens. Thus
+ * set the length to max_tokens. This matches the extra data
+ * allocation.
+ * The driver should fill it with the number of tokens it
+ * provided, and it may check iwp->length rather than having
+ * knowledge of max_tokens. If the driver doesn't change the
+ * iwp->length, this ioctl just copies back max_token tokens
+ * filled with zeroes. Hopefully the driver isn't claiming
+ * them to be valid data.
+ */
+ iwp->length = descr->max_tokens;
+ }
+
err = handler(dev, info, (union iwreq_data *) iwp, extra);
iwp->length += essid_compat;
--- wireless-testing.orig/net/wireless/wext-compat.c 2010-08-30 11:49:22.000000000 +0200
+++ wireless-testing/net/wireless/wext-compat.c 2010-08-30 11:49:29.000000000 +0200
@@ -1420,6 +1420,9 @@ int cfg80211_wext_giwessid(struct net_de
{
struct wireless_dev *wdev = dev->ieee80211_ptr;
+ data->flags = 0;
+ data->length = 0;
+
switch (wdev->iftype) {
case NL80211_IFTYPE_ADHOC:
return cfg80211_ibss_wext_giwessid(dev, info, data, ssid);
From: Johannes Berg <[email protected]>
Wireless extensions have an unfortunate, undocumented
requirement which requires drivers to always fill
iwp->length when returning a successful status. When
a driver doesn't do this, it leads to a kernel heap
content leak when userspace offers a larger buffer
than would have been necessary.
Arguably, this is a driver bug, as it should, if it
returns 0, fill iwp->length, even if it separately
indicated that the buffer contents was not valid.
However, we can also at least avoid the memory content
leak if the driver doesn't do this by setting the iwp
length to max_tokens, which then reflects how big the
buffer is that the driver may fill, regardless of how
big the userspace buffer is.
To illustrate the point, this patch also fixes a
corresponding cfg80211 bug (since this requirement
isn't documented nor was ever pointed out by anyone
during code review, I don't trust all drivers nor
all cfg80211 handlers to implement it correctly).
Cc: [email protected] [all the way back]
Signed-off-by: Johannes Berg <[email protected]>
---
Tested all four cases now (both hunks of the patch present/not present)
and reproduced the problem in the unpatched case.
net/wireless/wext-compat.c | 3 +++
net/wireless/wext-core.c | 16 ++++++++++++++++
2 files changed, 19 insertions(+)
--- wireless-testing.orig/net/wireless/wext-core.c 2010-08-30 12:04:57.000000000 +0200
+++ wireless-testing/net/wireless/wext-core.c 2010-08-30 12:10:35.000000000 +0200
@@ -782,6 +782,22 @@ static int ioctl_standard_iw_point(struc
}
}
+ if (IW_IS_GET(cmd) && !(descr->flags & IW_DESCR_FLAG_NOMAX)) {
+ /*
+ * If this is a GET, but not NOMAX, it means that the extra
+ * data is not bounded by userspace, but by max_tokens. Thus
+ * set the length to max_tokens. This matches the extra data
+ * allocation.
+ * The driver should fill it with the number of tokens it
+ * provided, and it may check iwp->length rather than having
+ * knowledge of max_tokens. If the driver doesn't change the
+ * iwp->length, this ioctl just copies back max_token tokens
+ * filled with zeroes. Hopefully the driver isn't claiming
+ * them to be valid data.
+ */
+ iwp->length = descr->max_tokens;
+ }
+
err = handler(dev, info, (union iwreq_data *) iwp, extra);
iwp->length += essid_compat;
--- wireless-testing.orig/net/wireless/wext-compat.c 2010-08-30 12:04:57.000000000 +0200
+++ wireless-testing/net/wireless/wext-compat.c 2010-08-30 12:11:32.000000000 +0200
@@ -1420,6 +1420,9 @@ int cfg80211_wext_giwessid(struct net_de
{
struct wireless_dev *wdev = dev->ieee80211_ptr;
+ data->flags = 0;
+ data->length = 0;
+
switch (wdev->iftype) {
case NL80211_IFTYPE_ADHOC:
return cfg80211_ibss_wext_giwessid(dev, info, data, ssid);
On Mon, Aug 30, 2010 at 11:59:01AM +0200, Johannes Berg wrote:
>
> 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.
Correct, Kees pointed out that my comment was bogus and the
e-mail I sent after the patch corrected myself on that point :
------------------------------------
> The comment should probably be clarified -- it's the caller's iwp->length
> that may be causing problems
Ha ! I see. It would be for regular iwpoint queries, not for
extended NOMAX queries (scan is a extended NOMAX query).
------------------------------------
> 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").
Yes, I had arrived at the same conclusion (not that my patch
did fix the issue).
> 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....
I actually like your patch better than mine, it's closer to
the original intent of the API. Go for it ;-)
> johannes
Thanks a lot for the second pair of eyes.
Jean
On Mon, 2010-08-30 at 10:40 -0700, Jean Tourrilhes wrote:
> Ha ! I see. It would be for regular iwpoint queries, not for
> extended NOMAX queries (scan is a extended NOMAX query).
Yeah, that took a while to sink in here too ..
> 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.
And thank you for looking over my patch. I guess the real bug is still
in cfg80211 there.
Initially I considered setting iwp->length to zero, rather than
max_tokens. What do you think about doing that? The reason I decided to
use max_tokens was that somebody might look at the length value to know
how much to copy (which is OK only in NOMAX queries); but that would be
a more severe driver bug ... can't really make up my mind. We just copy
back zeroes anyway.
johannes
Hi Johannes,
On Mon, Aug 30, 2010 at 12:24:54PM +0200, Johannes Berg wrote:
> --- wireless-testing.orig/net/wireless/wext-compat.c 2010-08-30 12:04:57.000000000 +0200
> +++ wireless-testing/net/wireless/wext-compat.c 2010-08-30 12:11:32.000000000 +0200
> @@ -1420,6 +1420,9 @@ int cfg80211_wext_giwessid(struct net_de
> {
> struct wireless_dev *wdev = dev->ieee80211_ptr;
>
> + 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.
-Kees
--
Kees Cook
Ubuntu Security Team
On Mon, 2010-08-30 at 11:03 -0700, Kees Cook wrote:
> Hi Johannes,
>
> On Mon, Aug 30, 2010 at 12:24:54PM +0200, Johannes Berg wrote:
> > --- wireless-testing.orig/net/wireless/wext-compat.c 2010-08-30 12:04:57.000000000 +0200
> > +++ wireless-testing/net/wireless/wext-compat.c 2010-08-30 12:11:32.000000000 +0200
> > @@ -1420,6 +1420,9 @@ int cfg80211_wext_giwessid(struct net_de
> > {
> > struct wireless_dev *wdev = dev->ieee80211_ptr;
> >
> > + 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.
Yes, I was, very easily, by doing an SIOCGIWESSID while unassociated,
with a large iwq->length set by userspace. I did CC stable on my patch,
but we can amend the commit by a CVE if so desired.
johannes