Improper usage of ReadBytes in mozilla::net::NetAddr

RESOLVED FIXED in Firefox 47

Status

()

Core
Networking: DNS
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: tedd, Assigned: nwgh)

Tracking

(Blocks: 1 bug, {sec-low})

unspecified
mozilla47
sec-low
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [adv-main47+][post-critsmash-triage])

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Pickle::ReadBytes[1] does not allocate memory and copies the data out of the serialized object, instead it just returns a pointer into the serialized object and it is up to the caller to do something with that data (usually use memcpy() to copy it to a persistent location).

When unserializing mozilla::net::NetAddr[2][3], ReadBytes is not called properly. NetAddr is defined here[4] with the |raw| union element defined as follows:

> union NetAddr {
>  struct {
>    uint16_t family;
>    char data[14];
>  } raw;
>  ...
> };

|&aResult->raw.data| returns a pointer to the |data| buffer and casts it to a const char **, now when ReadBytes is called, it will deference that pointer pointer and write a pointer, which points to the serialized data, at the destination.

Which means that the first four or eight bytes of |data| correspond to a pointer and not the actual raw address data.

I file this bug as a security bug out of precaution, since depending on the usage of the NetAddr object, it could lead to info leak.
If this is not the case, I don't see why this should be a security bug.

[1] https://dxr.mozilla.org/mozilla-central/rev/c5da92c5b4906369dee83629f81d647226ac1038/ipc/chromium/src/base/pickle.cc#468
[2] https://dxr.mozilla.org/mozilla-central/rev/6764bc656c1d146962d53710d734c2ac87c2306f/netwerk/ipc/NeckoMessageUtils.h#111
[3] https://dxr.mozilla.org/mozilla-central/rev/6764bc656c1d146962d53710d734c2ac87c2306f/netwerk/ipc/NeckoMessageUtils.h#125
[4] https://dxr.mozilla.org/mozilla-central/rev/c5da92c5b4906369dee83629f81d647226ac1038/netwerk/dns/DNS.h#89
(Reporter)

Updated

a year ago
Blocks: 1041862
Component: IPC → Networking: DNS
Is there any reason why an AF_UNSPEC NetAddr would be being passed over IPC, or can that case just be removed?  (For that matter, why would an AF_UNSPEC address exist in the first place?  And does the data mean anything in that case?)

I looked for uses of "raw.data" (just that text, but NetAddr::raw is an anonymous struct, so accesses probably have that syntax) and found only the one in MDNSResponderReply.cpp, copying *to* it from an actual struct sockaddr.  So this probably doesn't need to be a private bug, but someone who knows Necko better should make that call.
Flags: needinfo?(mcmanus)
Group: core-security → network-core-security
(In reply to Jed Davis [:jld] from comment #1)
> Is there any reason why an AF_UNSPEC NetAddr would be being passed over IPC,
> or can that case just be removed? 

I don't think that can be removed - generally you would set unspec on it before passing to some kind of resolution function and it would come back indicating v4/v6 with the rest filled out.. or something like that.

.. 

this comes from bug 648878 - so asking nick to take ownership

..

tedd I want to confirm I understand what you're saying..

instead of
     return aMsg->ReadBytes(aIter,
                 reinterpret_cast<const char**> &aResult->raw.data),
                 sizeof(aResult->raw.data));

something like the below?:

const char *tmp;
if (aMsg->Readbytes(aIter, &tmp, sizeof(aResult->raw.data))) {
 memcpy (&aResult->RawData, tmp, sizeof(aResult->raw.data));
 return true;
}
return false;
Flags: needinfo?(mcmanus) → needinfo?(hurley)
(Reporter)

Comment 3

a year ago
:mcmanus, yes exactly, ReadBytes returns a pointer and checks that the given size fits inside the serialized object (to make sure the other end actually supplied enough data).
Created attachment 8714376 [details] [diff] [review]
patch

Must be "old bugs come back to bite me" day.

This one doesn't seem too serious - I'm almost certain we never ship around AF_LOCAL sockets (at least, we didn't when I wrote the original patch, that could've changed). I'm not sure we ship AF_UNSPEC around, though I suspect this would've bitten us already if we did.
Attachment #8714376 - Flags: review?(mcmanus)
Assignee: nobody → hurley
Status: NEW → ASSIGNED
Flags: needinfo?(hurley)
Attachment #8714376 - Flags: review?(mcmanus) → review+
Keywords: sec-low
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f8cd81f16a73
https://hg.mozilla.org/mozilla-central/rev/f8cd81f16a73
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Group: network-core-security → core-security-release
Duplicate of this bug: 661158
Whiteboard: [adv-main47+]
Whiteboard: [adv-main47+] → [adv-main47+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.