If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Potential buffer overrun in hardware/libhardware_legacy/wifi/wifi.c update_ctrl_interface()

RESOLVED INCOMPLETE

Status

()

Core
Widget: Gonk
RESOLVED INCOMPLETE
3 years ago
a year ago

People

(Reporter: jseward, Assigned: jseward)

Tracking

({csectype-bounds, sec-high})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

When starting up FxOS on Nexus5, Valgrind reports uses of strstr on
undefined values in update_ctrl_interface.  On looking at how 
|pbuf| is used, I don't see any place where the string in |pbuf|
is zero terminated before it is given to strstr.  So I guess that
Valgrind is complaining about the second call to strstr falling
off the end of the string.
This is gecko in the b2g world.  I don't think I have the right component
for this, but not sure what I should set it to.
Created attachment 8551746 [details]
Valgrind complaints
(In reply to Julian Seward [:jseward] from comment #1)
> This is gecko in the b2g world.

Duh, sorry.  Obviously not gecko.  I just meant, this is in the
b2g tree.
Possibly related to bug 1000982 - crash in strstr | libhardware_legacy.so@0x2481
Created attachment 8551773 [details] [diff] [review]
a possible fix

This patch ensures that |pbuf| contains no undefined bytes and is
always zero terminated.  Stops Valgrind complaining.  No idea if it is
really correct -- I don't know what steps I should take to test it.

To whom should I offer it up for review?
Fabrice do you know who knows stuff about this code and could look at the patch?

It would also be good to know how much of a security problem this could be in practice.
Flags: needinfo?(fabrice)
I think Michael has some experience with upstream bugs.
Flags: needinfo?(fabrice) → needinfo?(mwu)
Buffer overflow sounds bad, so I'm going to mark this sec-high.
Keywords: csectype-bounds, sec-high
This happens also on Flame (flame-kk).
Attachment #8551773 - Flags: review?(mwu)
Comment on attachment 8551773 [details] [diff] [review]
a possible fix

Review of attachment 8551773 [details] [diff] [review]:
-----------------------------------------------------------------

::: wifi/wifi.c
@@ +350,5 @@
>      }
>  
> +    /* Make sure pbuf is zero terminated.  Since pbuf_size > sb.st_size,
> +       this shouldn't overwrite any data read from srcfd. */
> +    pbuf[pbuf_size-1] = '\0';

I think that you should still use malloc instead of calloc, and put the null at pbuf[nread].

Then you don't need the +1 either. No strstr calls are made after inserting the new value, and the null is only needed for the strstr call.

Comment 11

3 years ago
I don't think this is a particularly bad bug - this would happen if we read a wifi configuration file that was tampered with, but neither apps nor the user are normally allowed to directly modify the wifi config files. A user would need root to do so. Worth fixing though.
Flags: needinfo?(mwu)

Comment 12

3 years ago
Comment on attachment 8551773 [details] [diff] [review]
a possible fix

Review of attachment 8551773 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with dhylands - Setting pbuf[nread] to null should be enough.

BTW, unrelated to your patch, the read logic in this function looks wrong to me - read might only read part of the file, and we need to loop it to ensure that the entire file is read. TEMP_FAILURE_RETRY isn't enough.

::: wifi/wifi.c
@@ +331,4 @@
>      if (stat(config_file, &sb) != 0)
>          return -1;
>  
> +    size_t pbuf_size = sb.st_size + PROPERTY_VALUE_MAX + 1;

PROPERTY_VALUE_MAX actually accounts for the null string terminator, so real property values can only be up to PROPERTY_VALUE_MAX - 1. So the + 1 shouldn't be necessary.
Attachment #8551773 - Flags: review?(mwu)
Julian, is there progress on the patch?
Flags: needinfo?(jseward)
Will try to get this wrapped up early next week.
Flags: needinfo?(jseward)
Julian, any progress here?  Should we look for somebody else to work on this?  It has been sitting around for a few months. Thanks.
Assignee: nobody → jseward
Flags: needinfo?(jseward)
I am not seeing these errors in recent b2g runs any more.  And it looks
as if the bug got fixed.  Somewhere.  Upstream?  It would be nice to be
able to track down the original changeset and/or bug report, as a cross-check,
but I can't figure out how.  Do you know?  Anyway, what I see from git blame is:

363 cac4f50c (Tapas Kumar Kundu     2014-07-02 06:43:30 -0700 363)     if(nread < sb.st_size)
364 cac4f50c (Tapas Kumar Kundu     2014-07-02 06:43:30 -0700 364)       pbuf[nread]='\0';
365 cac4f50c (Tapas Kumar Kundu     2014-07-02 06:43:30 -0700 365)     else
366 cac4f50c (Tapas Kumar Kundu     2014-07-02 06:43:30 -0700 366)       pbuf[sb.st_size]='\0';
367 cac4f50c (Tapas Kumar Kundu     2014-07-02 06:43:30 -0700 367)     ALOGD("value read from %s file is %s", config_file, pbuf);
368 cac4f50c (Tapas Kumar Kundu     2014-07-02 06:43:30 -0700 368) 

which afaics does provide the required zero termination.
Flags: needinfo?(jseward)
https://www.codeaurora.org/cgit/quic/la/platform/hardware/libhardware_legacy/commit/?id=cac4f50c
Ah, this is more complex than it seems.  A tree configured for flame-kk has a
wifi.c that contains the fix shown in comment 16.  But a tree configured for
nexus-5 doesn't, and I still see the Valgrind complaints.

What is the right thing to do?  Should we put the comment 16 fix into the
nexus-5 version of wifi.c?  Looking at the link in comment 17, it's pretty
clear that the lack of buffer termination caused a segfault at some point.
Created attachment 8610538 [details] [diff] [review]
bug1123636-2.diff

The comment 17 patch, as it applies to our tree.
Attachment #8551773 - Attachment is obsolete: true
Flags: needinfo?(mwu)
Duplicate of this bug: 989308
Per some digging by MWu, it appears that the entire function
update_ctrl_interface has been removed in Android 5.1 upstream.
Closing, therefore.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(mwu)
Resolution: --- → INCOMPLETE

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.