1066570 - b2g NFC uninitialized memory bug

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[banty]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0
Build ID: 20140716183446

Steps to reproduce:

While auditing the NFC service I came across a potential uninitialized pointer in NfcMessageHandler::GeneralResponse. 

bool
NfcMessageHandler::GeneralResponse(const Parcel& aParcel, EventOptions& aOptions)
{
  const char* type; //type can go uninitalized due to missing default 
  NS_ENSURE_TRUE(!mPendingReqQueue.IsEmpty(), false);
  int pendingReq = mPendingReqQueue[0];
  mPendingReqQueue.RemoveElementAt(0);

  switch (pendingReq) {
    case NfcRequest::WriteNDEFReq:
      type = kWriteNDEFResponse;
      break;
    case NfcRequest::MakeReadOnlyNDEFReq:
      type = kMakeReadOnlyNDEFResponse;
      break;q
    case NfcRequest::ConnectReq:
      type = kConnectResponse;
      break;
    case NfcRequest::CloseReq:
      type = kCloseResponse;
      break;
  }


Actual results:

The switch statement has no default case. If the attacker can send arbitrary NFC messages they could pass over the switch leaving "const char* type" uninitialized. The variable is then accessed here, "aOptions.mType = NS_ConvertUTF8toUTF16(type).

Could lead to DoS and maybe info leak? Working on proof of concept...


Expected results:

The switch statement should have a default case that catches non-existent request types.

Comment 1

[Frederik Braun [:freddy]]

Sandip, Ken: Can you help with verifying and triage?

Comment 2

[Frederik Braun [:freddy]]

CCing a few more people from the NFC team.
Wesley and Howie, can either you help us get some engineers to verify this?

Comment 3

[Stephanie Ouillon [:arroway] (needinfo me)]

Yoshi, it's look like you wrote the code, could you have a look at it?

Comment 4

[Stephanie Ouillon [:arroway] (needinfo me)]

After a quick search, it's look like to me it's filtered before: http://dxr.mozilla.org/mozilla-central/source/dom/nfc/gonk/NfcMessageHandler.cpp#42

Comment 5

[banty]

looks like Stephanie is correct. 

mPendingReqQueue only adds requests on the client side and Marshall() doesn't give
any opportunity to add an invalid type.

this won't triggerable in real world but may want to add default handling in switch statement just
incase an exploitable condition is born later

Comment 6

[banty]

mPendingReqQueue only adds requests on the server* side

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb. 9]

Attachment: Patch

Comment 8

[Dimi Lee [:dimi]]

Comment on attachment 8493533 [details] [diff] [review]
Patch

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

::: dom/nfc/gonk/NfcMessageHandler.cpp
@@ +131,5 @@
>        type = kCloseResponse;
>        break;
> +    default:
> +     CHROMIUM_LOG("Nfcd, unknown general response %d", pendingReq);
> +     return false;

Nits. indention

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb. 9]

Attachment: Patch v2.

nits addressed.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb. 9]

https://hg.mozilla.org/integration/b2g-inbound/rev/34247879cf78

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb. 9]

forgot to paste try
https://tbpl.mozilla.org/?tree=Try&rev=5d146097e2f9

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/34247879cf78