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

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Widget: Gonk, defect)

Description

[Julian Seward [:jseward]]

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.

Comment 1

[Julian Seward [:jseward]]

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.

Comment 2

[Julian Seward [:jseward]]

Attachment: Valgrind complaints

Comment 3

[Julian Seward [:jseward]]

(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.

Comment 4

[Julian Seward [:jseward]]

Possibly related to bug 1000982 - crash in strstr | libhardware_legacy.so@0x2481

Comment 5

[Julian Seward [:jseward]]

Attachment: 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?

Comment 6

[Andrew McCreight [:mccr8]]

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.

Comment 7

[[:fabrice] Fabrice Desré]

I think Michael has some experience with upstream bugs.

Comment 8

[Andrew McCreight [:mccr8]]

Buffer overflow sounds bad, so I'm going to mark this sec-high.

Comment 9

[Julian Seward [:jseward]]

This happens also on Flame (flame-kk).

Comment 10

[Dave Hylands [:dhylands]]

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

[Michael Wu [:mwu]]

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.

Comment 12

[Michael Wu [:mwu]]

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.

Comment 13

[Matt Wobensmith [:mwobensmith][:matt:]]

Julian, is there progress on the patch?

Comment 14

[Julian Seward [:jseward]]

Will try to get this wrapped up early next week.

Comment 15

[Andrew McCreight [:mccr8]]

Julian, any progress here?  Should we look for somebody else to work on this?  It has been sitting around for a few months. Thanks.

Comment 16

[Julian Seward [: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.

Comment 17

[:gerard-majax]

https://www.codeaurora.org/cgit/quic/la/platform/hardware/libhardware_legacy/commit/?id=cac4f50c

Comment 18

[Julian Seward [:jseward]]

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.

Comment 19

[Julian Seward [:jseward]]

Attachment: bug1123636-2.diff

The comment 17 patch, as it applies to our tree.

Comment 21

[Julian Seward [:jseward]]

Per some digging by MWu, it appears that the entire function
update_ctrl_interface has been removed in Android 5.1 upstream.
Closing, therefore.