505277 - strncpy without null termination in nsWifiScannerUnix

Status: VERIFIED FIXED

(Core :: DOM: Geolocation, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

nsWifiScannerUnix calls strncpy when setting ap->mSsid without null-terminating the buffer.  This means that if |buffer| contains a string longer than 31 characters, ap->mSsid will be missing its null-terminator:
   strncpy(ap->mSsid, buffer, 32);
and anything that reads from ap->mSsid will get arbitrary data past the end of the string out of memory.

Instead, this code should probably use the SetSsid method that other wifi scanner implementations use, which handles the null-termination correctly.

(Likewise, for good code hygiene, the code below that assigns directly to ap->mSignal should probably use ap->SetSignal() instead.)

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

untested -- my vm doesn't have wifi hardware exposed.  i can give it a go tomorrow.

Comment 2

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/02913e3a7eaf

Comment 3

[Doug Turner (:dougt)]

(btw, asked bc on irc.mozilla.org #qa tested last night)

Comment 4

[Doug Turner (:dougt)]

s/tested/to test

Comment 5

[Samuel Sidler (old account; do not CC)]

Comment on attachment 389834 [details] [diff] [review]
patch v.1

Not for 1.9.1.2.

Comment 6

[Samuel Sidler (old account; do not CC)]

Comment on attachment 389834 [details] [diff] [review]
patch v.1

Approved for 1.9.1.3. a=ss

Comment 7

[Samuel Sidler (old account; do not CC)]

Doug: Can you get this landed for 1.9.1.3? Code freeze is tonight...

Comment 8

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/45436bc0b7fd

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed based on check-ins into trunk and 1.9.1 branch.

Comment 10

[Daniel Veditz [:dveditz]]

Not sure this is much of a security problem, maybe we're leaking a bit of memory to someone? Is some part of our app using the data assuming it's sanitized or not too long and causing problems elsewhere?