Allow for seccomp filtering of socket(AF_INET/AF_INET_6) calls on Linux when using UNIX Domain Sockets for SOCKS Proxy

RESOLVED FIXED in Firefox 61

Status

()

P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: richard, Assigned: richard)

Tracking

52 Branch
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [tor][tor 22794][necko-triaged])

Attachments

(1 attachment, 1 obsolete attachment)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180208173149
(Assignee)

Updated

a year ago
Priority: -- → P1
Whiteboard: [tor][tor 22794]
Posted patch 1441327_00.patch (obsolete) — Splinter Review
This patch is prerequisite for blocking network socket creation via seccomp policy in the sandboxed content process.  Currently, with seccomp policy in place to have socket(AF_INET,...) and friends return an error, SOCKS proxy connection fails (even when configured to use a UNIX Domain Socket) because the code first creates an AF_INET socket, and then later on replaces it with the socket type actually required.  Creation failure for this initial dummy socket is not recoverable.

The attached patch adds additional logic for Linux and macOS builds to first check to see if the provided SOCKS proxy host begins with "file://" and if so, assumes it is a path to a UNIX Domain Socket and opens an AF_LOCAL socket with the provided path, rather than the requested family.  Similar logic already exists for named pipes on Windows.

For past discussion about this issue, see tor trac ticket #22794: https://trac.torproject.org/projects/tor/ticket/22794

The attached patch passes all mochitests for linux, linux64 and macOS (apart from 1 which seems to be an infastructure or test bug which has two bugs filed with it): https://treeherder.mozilla.org/testview.html?repo=try&revision=19e83acb73284b03899c788550b34f5efe359bfe
(Assignee)

Updated

a year ago
Attachment #8954195 - Flags: review?(honzab.moz)
Attachment #8954195 - Flags: review?(daniel)
Comment on attachment 8954195 [details] [diff] [review]
1441327_00.patch

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

not a linux expert
Attachment #8954195 - Flags: review?(honzab.moz)
some general guidelines:
- 8 lines context for your patches
- keep consistent with mozilla code formatting
- add comments to your code
Moving this issue to "Core: Networking" for now, which seems the most appropriate component for this.
Component: Untriaged → Networking
Product: Firefox → Core

Updated

a year ago
Assignee: nobody → richard
Whiteboard: [tor][tor 22794] → [tor][tor 22794][necko-triaged]

Updated

a year ago
Priority: P1 → P2
Richard, is this new patch meant to be reviewed?  If so, please set the review flag to r? on the attachment (perhaps with daniel@haxx.se as reviewer).

(Just didn't want this getting lost.)
Flags: needinfo?(richard)
(Assignee)

Updated

a year ago
Attachment #8956956 - Flags: review?(daniel)
Attachment #8956956 - Flags: review?(daniel) → review+
(Assignee)

Updated

a year ago
Flags: needinfo?(richard)
Keywords: checkin-needed

Comment 8

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd4d107aa919
Allow for seccomp filtering of socket(AF_INET/AF_INET_6) calls on Linux when using UNIX Domain Sockets for SOCKS Proxy. r=bagder
Keywords: checkin-needed

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fd4d107aa919
Status: UNCONFIRMED → RESOLVED
Last Resolved: a year ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Comment 10

a year ago
Comment on attachment 8956956 [details] [diff] [review]
updated patch based on feedback and increased context to 8 lines

Approval Request Comment
[Feature/Bug causing the regression]: none
[User impact if declined]: Tor Browser and other browsers based on Firefox 60 ESR would need to ship this patch on their own if it can't make it into the next ESR. Similar functionality is already available on Windows and this patch adds the missing bits for *NIX.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: It's been a while on Nightly now.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: The patch is not complicated and basically just covering the missing *NIX part by generalizing the already available Windows related code.
[String changes made/needed]: none
Attachment #8956956 - Flags: approval-mozilla-beta?
Comment on attachment 8956956 [details] [diff] [review]
updated patch based on feedback and increased context to 8 lines

This feels like it can ride the trains.
Attachment #8956956 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.