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

RESOLVED FIXED in Firefox 47



Networking: DNS
2 years ago
11 months ago


(Reporter: tedd, Assigned: nwgh)


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


Firefox Tracking Flags

(firefox47 fixed)


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


(1 attachment)



2 years 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->| 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.



2 years ago
Blocks: 1041862


2 years ago
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 "" (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->,

something like the below?:

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

Comment 3

2 years 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]

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
Flags: needinfo?(hurley)
Attachment #8714376 - Flags: review?(mcmanus) → review+
Keywords: sec-low
Last Resolved: 2 years 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]


11 months ago
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.