1545504 - Replace uses of select() or ensure array bounds checks

Status: RESOLVED FIXED

(Core :: General, defect, P2)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This is a followup to bug 1516325. We have a few other uses of select that could cause problems as far as the array bounds for fd_set; they're generally not security-sensitive because they're specific to Linux (where glibc has bounds checks, so we'll crash safely) or apply to file descriptors allocated early (so the values will normally be in bounds) or in code that we're reasonably sure is dead in our usage. However, these should be fixed to avoid crashes and possible future security bugs.

I'm filing this bug as security-sensitive because bug 1516325 isn't on release yet, and landing these fixes is going to make it even more obvious than it already is what's going on there.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Status: I have patches, but I need to clean them up (and, at this point, rebase them); it's a little delicate because of the connection to a security bug and there always seem to be higher priorities….

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

The selects in WebRTC's video_capture subtree were independently fixed in bug 1590984 after they were encountered in the wild (and caught via glibc's bounds checks). I'll upload the other patches.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1545504 - Use poll() instead of select() on the X11 socket.

These are single-fd polls of the X server socket, which in practice will
be much smaller than FD_SETSIZE, but it's cleaner to just not have the
fixed-size array in the first place.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1545504 - Strengthen bounds check in WebRTC PhysicalSocketServer.

PhysicalSocketServer isn't currently used by Mozilla's WebRTC
integration, but just in case, let's make sure that this array index is
bounds-checked in actual use, not just in debug builds (which tend to
never see realistic test conditions).

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

https://hg.mozilla.org/integration/autoland/rev/44b5b561e937
https://hg.mozilla.org/integration/autoland/rev/2b079067648b

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/44b5b561e937
https://hg.mozilla.org/mozilla-central/rev/2b079067648b

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Sounds like this fix can ride the trains, but feel free to nominate for uplift if you feel strongly otherwise.