810581 - Uninitialised value use in InitRndisAddress

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Julian Seward [:jseward]]

Happens on every startup of b2g.  Reported error below.

Reading the code for InitRndisAddress() in gecko/ipc/netd/Netd.cpp, I
could not see how address[1 .. kEthernetAddressLength-1] is supposed
to be set to any defined value, before being handed to sprintf.  There
is

  address[0] = 0x02;
  length = strlen(serialno);
  for (i = 0; i < length; i++) {
    address[i % (kEthernetAddressLength - 1) + 1] ^= serialno[i];
  }

but the loop merely xors serialno[] entries into uninitialised
address[] entries, leaving them containing garbage.

AFAICS, the junk address[] entries are sprintf'd into mac[] and
written into some fd, so is potentially serious.

Conditional jump or move depends on uninitialised value(s)
   at 0x4848154: __vfprintf (vfprintf.c:661)
   by 0x4846A05: sprintf (sprintf.c:59)
   by 0x5A8662F: mozilla::ipc::InitNetdIOThread() (Netd.cpp:71)
   by 0x56764C7: RunnableFunction<void (*)(), Tuple0>::Run() (tuple.h:439)
   by 0x5ADB1F5: MessageLoop::RunTask(Task*) (message_loop.cc:333)
   by 0x5ADBD9F: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (message_loop.cc:341)
   by 0x5ADE04B: MessageLoop::DoWork() (message_loop.cc:441)
   by 0x5AF60B1: base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) (message_pump_libevent.cc:311)
   by 0x5ADB1A3: MessageLoop::RunInternal() (message_loop.cc:215)
   by 0x5ADB1AD: MessageLoop::RunHandler() (message_loop.cc:208)
   by 0x5ADB281: MessageLoop::Run() (message_loop.cc:182)
   by 0x5AE74B9: base::Thread::ThreadMain() (thread.cc:156)
 Uninitialised value was created by a stack allocation
   at 0x5A86526: mozilla::ipc::InitNetdIOThread() (Netd.cpp:292)

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> but the loop merely xors serialno[] entries into uninitialised
> address[] entries, leaving them containing garbage.
> 
> AFAICS, the junk address[] entries are sprintf'd into mac[] and
> written into some fd, so is potentially serious.

Let's first lock and get some eyes on this, to be safe.

Comment 2

[Doug Turner (:dougt)]

vchang, looks like you added this code.  Can you take a look?

Also, is this code shared with desktop?

Comment 3

[Julian Seward [:jseward]]

I don't recall having seen V complain about this on desktop or Android.

Comment 4

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch, initial the local variable

The original souce code of InitRndisAddress() comes from  
http://androidxref.com/4.0.4/xref/frameworks/base/services/java/com/android/server/usb/UsbDeviceManager.java#initRndisAddress. The local variable address is declared and is initialized with default value zero using "new" in java.  
     int address[] = new int[ETH_ALEN];
In C or C++, the value of local variable comes from stack and may not be initialized to default value. Add memset to initialize local variable address and prevent adding garbage in mac address.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Does this need security approval to land?

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Vincent and I discussed this on IRC. Since this is b2g only, it doesn't affect any currently shipping code. I will uplift this to beta/aurora once it hits m-c.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a2f6ef08f64

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0a2f6ef08f64

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/190fd00356fd
https://hg.mozilla.org/releases/mozilla-beta/rev/a7469f3bb513