Closed Bug 810581 Opened 7 years ago Closed 7 years ago

Uninitialised value use in InitRndisAddress

Categories

(Core :: IPC, defect, critical)

ARM
Gonk (Firefox OS)
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla20
blocking-basecamp +
Tracking Status
firefox17 --- unaffected
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: jseward, Assigned: vchang)

Details

(Keywords: valgrind, Whiteboard: [adv-main18-])

Attachments

(1 file)

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)
Keywords: valgrind
> 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.
Group: core-security
Severity: normal → critical
blocking-basecamp: --- → ?
vchang, looks like you added this code.  Can you take a look?

Also, is this code shared with desktop?
Assignee: nobody → vchang
blocking-basecamp: ? → +
I don't recall having seen V complain about this on desktop or Android.
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.
Attachment #682979 - Flags: review?(jseward)
Attachment #682979 - Flags: review?(jseward) → review+
Does this need security approval to land?
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
https://hg.mozilla.org/mozilla-central/rev/0a2f6ef08f64
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Whiteboard: [adv-main18-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.