1229726 - [MDNS] fix conversion from sockaddr to NetAddr

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

struct sockaddr has extra "sa_len" field on OSX and FreeBSD, we cannot memcpy a sockaddr to Netaddr.
https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/mdns/libmdns/MDNSResponderReply.cpp#284

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: bug1229726-fix-for-sockaddr-sa_len.patch

copy sockaddr.sa_family and sockaddr.sa_data separately instead of doing memcpy on sockaddr.

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 8695686 [details] [diff] [review]
bug1229726-fix-for-sockaddr-sa_len.patch

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

::: netwerk/dns/mdns/libmdns/MDNSResponderReply.cpp
@@ +281,5 @@
>    }
>  
>    NetAddr address;
> +  address.raw.family = aAddress->sa_family;
> +  memcpy(&address.raw.data, aAddress->sa_data, sizeof(aAddress->sa_data));

I don't like that sizeof checking the size of the src only - seems like it could end up with an overwrite. probably need to make sure dest is >= size of src. otherwise lgtm

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: bug1229726-fix-for-sockaddr-sa_len.patch

Use static assert to check the size of dest.

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8701294 [details] [diff] [review]
bug1229726-fix-for-sockaddr-sa_len.patch

sorry, wrong patch file.

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: MozReview Request: Bug 1229726 - fix the data copy from sockaddr to NetAddr on OSX/FreeBSD. r=mcmanus.

Review commit: https://reviewboard.mozilla.org/r/28951/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28951/

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8828d3da388f

Comment 7

[Josh Cheng [:josh]]

Hi SC,
POlease raise 2.5 uplift request. Thanks!

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8701353 [details]
MozReview Request: Bug 1229726 - fix the data copy from sockaddr to NetAddr on OSX/FreeBSD. r=mcmanus.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1200132
User impact if declined: User cannot launch presentation request from OS X and FreeBSD. No impact for device that receiving presentation request but buffer copy issue might be potential security hole.
Testing completed: done by manual test.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: N/A

Comment 9

[Josh Cheng [:josh]]

Comment on attachment 8701353 [details]
MozReview Request: Bug 1229726 - fix the data copy from sockaddr to NetAddr on OSX/FreeBSD. r=mcmanus.

Approved for Tv2.5

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8828d3da388f

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/616c4d756b90