1790419 - hoist mozilla::BinaryPath::Get before OpenBSD sandboxing

Assignee

Description

•

3 years ago

•

Right now on OpenBSD, we have a patch to return early in mozilla::BinaryPath::Get with an hardcoded known value, cf http://cvsweb.openbsd.org/cgi-bin/cvsweb/ports/www/mozilla-firefox/patches/patch-xpcom_build_BinaryPath_h?rev=1.2&content-type=text/x-cvsweb-markup

this is done because the method is called at various stages in the processes lifetime including after starting the sandbox, and the KERN_PROC_ARGV sysctl isnt allowed by pledge(), resulting in the process getting killed (or the ps pledge class needs to be added to the allowed syscalls).

i'd like to improve this by ensuring mozilla::BinaryPath::Get is called before starting the sandboxing, and that the value is cached for later reuses instead of calling again and again the sysctl.

to achieve that, i've added two public members to mozilla::BinaryPath (only in the OpenBSD codepaths) as below (naive implementation for now) here https://searchfox.org/mozilla-central/source/xpcom/build/BinaryPath.h#200:

 #elif defined(__OpenBSD__)
+ private:
+  static char cachedPath[MAXPATHLEN];
+ public:
+  static bool cached;
   static nsresult Get(char aResult[MAXPATHLEN]) {
     int mib[4];
+    if (cached) {
+      if (strlcpy(aResult, cachedPath, MAXPATHLEN) != strlen(cachedPath))
+        return NS_ERROR_FAILURE;
+      return NS_OK;
+    }
     mib[0] = CTL_KERN;
     mib[1] = KERN_PROC_ARGS;
     mib[2] = getpid();
     mib[3] = KERN_PROC_ARGV;
 
     size_t len = 0;
     if (sysctl(mib, 4, nullptr, &len, nullptr, 0) < 0) {
       return NS_ERROR_FAILURE;
     }
 
     auto argv = MakeUnique<const char*[]>(len / sizeof(const char*));
     if (sysctl(mib, 4, argv.get(), &len, nullptr, 0) < 0) {
       return NS_ERROR_FAILURE;
     }
 
+    if (strlcpy(cachedPath, argv[0], MAXPATHLEN) != strlen(argv[0]))
+      return NS_ERROR_FAILURE;
+    cached = true;
     return GetFromArgv0(argv[0], aResult);
   }

and in StartOpenBSDSandbox before calling pledge i force-call the method here https://searchfox.org/mozilla-central/source/dom/ipc/ContentChild.cpp#4927:

+  mozilla::BinaryPath::cached = false;
+  nsresult rv = mozilla::BinaryPath::Get(binaryPath);
+  if (NS_FAILED(rv)) {
+    errx(1, "failed to cache binary path ?");
+  }
   if (NS_WARN_IF(NS_FAILED(OpenBSDUnveilPaths(unveilFile, pledgeFile)))) {
     errx(1, "failed reading/parsing %s", unveilFile.get());
   }

but linking firefox fails:

93:00.90 ld: error: undefined hidden symbol: mozilla::BinaryPath::cached
93:00.91 >>> referenced by BinaryPath.h:207 (/home/landry/src/m-c/xpcom/build/BinaryPath.h:207)
93:00.91 >>>               /usr/obj/m-c/browser/app/nsBrowserApp.o:(mozilla::BinaryPath::Get(char*))
93:00.91 >>> referenced by BinaryPath.h:229 (/home/landry/src/m-c/xpcom/build/BinaryPath.h:229)
93:00.91 >>>               /usr/obj/m-c/browser/app/nsBrowserApp.o:(mozilla::BinaryPath::Get(char*))
93:00.91 ld: error: undefined hidden symbol: mozilla::BinaryPath::cachedPath
93:00.91 >>> referenced by BinaryPath.h:208 (/home/landry/src/m-c/xpcom/build/BinaryPath.h:208)
93:00.91 >>>               /usr/obj/m-c/browser/app/nsBrowserApp.o:(mozilla::BinaryPath::Get(char*))
93:00.91 >>> referenced by BinaryPath.h:227 (/home/landry/src/m-c/xpcom/build/BinaryPath.h:227)
93:00.91 >>>               /usr/obj/m-c/browser/app/nsBrowserApp.o:(mozilla::BinaryPath::Get(char*))

should i do something specific to declare those members as available to callers ? What makes them hidden ? I've done a clobber build without success..

Landry Breuil (:gaston)

Assignee

Comment 1

•

3 years ago

hi :gijs:, can you give me a hint on how to achieve that, or direct me to the right person ? thanks !!

Flags: needinfo?(gijskruitbosch+bugs)

:Gijs (he/him)

Comment 2

•

3 years ago

(In reply to Landry Breuil (:gaston) from comment #1)

hi :gijs:, can you give me a hint on how to achieve that, or direct me to the right person ? thanks !!

I don't know, sorry - maybe either :gcp or :glandium can help.

Flags: needinfo?(mh+mozilla)

Flags: needinfo?(gpascutto)

Flags: needinfo?(gijskruitbosch+bugs)

Gian-Carlo Pascutto [:gcp]

Comment 3

•

3 years ago

Isn't the problem here you're adding static class variables (i.e. globals), but doing so in a .h file instead of a compilation unit (i.e. a .cpp file)?

Flags: needinfo?(gpascutto)

Landry Breuil (:gaston)

Assignee

Comment 4

•

3 years ago

•

Edited

(In reply to Gian-Carlo Pascutto [:gcp] from comment #3)

Isn't the problem here you're adding static class variables (i.e. globals), but doing so in a .h file instead of a compilation unit (i.e. a .cpp file)?

well technically there's no BinaryPath.cpp, the implementation is directly in the header... but if that's acceptable i can declare those statics in any other cpp file ?

Gian-Carlo Pascutto [:gcp]

Comment 5

•

3 years ago

Does pulling them inside the function help? (As the other platforms do)

Landry Breuil (:gaston)

Assignee

Comment 6

•

3 years ago

(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)

Does pulling them inside the function help? (As the other platforms do)

i'll try that, i think i overlooked something with static that made me think this wasnt possible, while this should be perfectly fine... since (to my understanding) that's what the windows implementation does.

Landry Breuil (:gaston)

Assignee

Comment 7

•

3 years ago

Attached patch bug-1790419 — Details — Splinter Review

wip patch, apparently builds (still checking) - will test runtime later, then send things to the phabricator contraption.

Assignee: nobody → landry

David Parks [:handyman]

Updated

•

3 years ago

Severity: -- → S4

Priority: -- → P5

Landry Breuil (:gaston)

Assignee

Comment 8

•

3 years ago

•

Edited

i finally have something that seems to work with proper sandboxing. Strange thing thought, ktracing the processes it seems that there are two subsequent close calls to BinaryPath::Get() in the same process that end up calling the sysctl (eg the value isnt 'cached' yet?) but all the future calls properly return the cached string.

in the ktrace excerpt below, 1st field is the pid, 3rd field is the timing.

 55989 firefox  1663319439.350853 CALL  sysctl(1.55.55989.1<kern.procargs.55989.1>,0,0x7f7ffffcbf88,0,0)
 55989 firefox  1663319439.351275 CALL  sysctl(1.55.55989.1<kern.procargs.55989.1>,0x5fda70dd000,0x7f7ffffcbf88,0,0)
 55989 firefox  1663319439.387726 CALL  sysctl(1.55.55989.1<kern.procargs.55989.1>,0,0x7f7ffffcbd98,0,0)
 55989 firefox  1663319439.387791 CALL  sysctl(1.55.55989.1<kern.procargs.55989.1>,0x5fda70dd000,0x7f7ffffcbd98,0,0)
 34917 firefox  1663319440.855540 CALL  sysctl(1.55.34917.1<kern.procargs.34917.1>,0,0x7f7ffffefaf8,0,0)
 34917 firefox  1663319440.855990 CALL  sysctl(1.55.34917.1<kern.procargs.34917.1>,0xc35dd737000,0x7f7ffffefaf8,0,0)
 34917 firefox  1663319440.922299 CALL  sysctl(1.55.34917.1<kern.procargs.34917.1>,0,0x7f7ffffef828,0,0)
 34917 firefox  1663319440.922395 CALL  sysctl(1.55.34917.1<kern.procargs.34917.1>,0xc35dd737000,0x7f7ffffef828,0,0)
 74251 firefox  1663319442.104561 CALL  sysctl(1.55.74251.1<kern.procargs.74251.1>,0,0x7f7ffffe65e8,0,0)
 74251 firefox  1663319442.105004 CALL  sysctl(1.55.74251.1<kern.procargs.74251.1>,0x3de1ab8f000,0x7f7ffffe65e8,0,0)
 74251 firefox  1663319442.144033 CALL  sysctl(1.55.74251.1<kern.procargs.74251.1>,0,0x7f7ffffe6318,0,0)
 74251 firefox  1663319442.144103 CALL  sysctl(1.55.74251.1<kern.procargs.74251.1>,0x3de1ab8f000,0x7f7ffffe6318,0,0)
 89285 firefox  1663319443.386270 CALL  sysctl(1.55.89285.1<kern.procargs.89285.1>,0,0x7f7ffffe1808,0,0)
 89285 firefox  1663319443.386896 CALL  sysctl(1.55.89285.1<kern.procargs.89285.1>,0x4561a237000,0x7f7ffffe1808,0,0)
 89285 firefox  1663319443.435602 CALL  sysctl(1.55.89285.1<kern.procargs.89285.1>,0,0x7f7ffffe1538,0,0)
 89285 firefox  1663319443.435704 CALL  sysctl(1.55.89285.1<kern.procargs.89285.1>,0x4561a237000,0x7f7ffffe1538,0,0)
 65960 firefox  1663319445.282415 CALL  sysctl(1.55.65960.1<kern.procargs.65960.1>,0,0x7f7ffffed068,0,0)
 65960 firefox  1663319445.282903 CALL  sysctl(1.55.65960.1<kern.procargs.65960.1>,0x7c74b62b000,0x7f7ffffed068,0,0)
 36379 firefox  1663319445.343421 CALL  sysctl(1.55.36379.1<kern.procargs.36379.1>,0,0x7f7fffff2948,0,0)
 36379 firefox  1663319445.343971 CALL  sysctl(1.55.36379.1<kern.procargs.36379.1>,0x33d03679000,0x7f7fffff2948,0,0)
 23628 firefox  1663319445.423643 CALL  sysctl(1.55.23628.1<kern.procargs.23628.1>,0,0x7f7ffffedb48,0,0)
 23628 firefox  1663319445.424231 CALL  sysctl(1.55.23628.1<kern.procargs.23628.1>,0x2c6d2070000,0x7f7ffffedb48,0,0)
 65960 firefox  1663319445.424933 CALL  sysctl(1.55.65960.1<kern.procargs.65960.1>,0,0x7f7ffffecd98,0,0)
 65960 firefox  1663319445.425020 CALL  sysctl(1.55.65960.1<kern.procargs.65960.1>,0x7c74b62b000,0x7f7ffffecd98,0,0)
 36379 firefox  1663319445.501835 CALL  sysctl(1.55.36379.1<kern.procargs.36379.1>,0,0x7f7fffff2678,0,0)
 36379 firefox  1663319445.501963 CALL  sysctl(1.55.36379.1<kern.procargs.36379.1>,0x33d03679000,0x7f7fffff2678,0,0)
 23628 firefox  1663319445.541664 CALL  sysctl(1.55.23628.1<kern.procargs.23628.1>,0,0x7f7ffffed878,0,0)
 23628 firefox  1663319445.541742 CALL  sysctl(1.55.23628.1<kern.procargs.23628.1>,0x2c6d2070000,0x7f7ffffed878,0,0)
 35463 firefox  1663319446.987423 CALL  sysctl(1.55.35463.1<kern.procargs.35463.1>,0,0x7f7ffffe48f8,0,0)
 35463 firefox  1663319446.989237 CALL  sysctl(1.55.35463.1<kern.procargs.35463.1>,0x383a41a4000,0x7f7ffffe48f8,0,0)
 35463 firefox  1663319447.085717 CALL  sysctl(1.55.35463.1<kern.procargs.35463.1>,0,0x7f7ffffe4628,0,0)
 35463 firefox  1663319447.085786 CALL  sysctl(1.55.35463.1<kern.procargs.35463.1>,0x383a41a4000,0x7f7ffffe4628,0,0)

and in the below ktrace with added debugging printfs we see the same:

 55989 firefox  1663319439.351616 GIO   fd 1 wrote 39 bytes
       "returning /home/landry/firefox/firefox
 55989 firefox  1663319439.388108 GIO   fd 1 wrote 39 bytes
       "returning /home/landry/firefox/firefox
 55989 firefox  1663319439.389967 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 55989 firefox  1663319439.390057 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 55989 firefox  1663319439.425212 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 55989 firefox  1663319439.425371 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 55989 firefox  1663319439.425382 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 55989 firefox  1663319439.463311 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 34917 firefox  1663319440.856430 GIO   fd 1 wrote 39 bytes
       "returning /home/landry/firefox/firefox
 34917 firefox  1663319440.922958 GIO   fd 1 wrote 39 bytes
       "returning /home/landry/firefox/firefox
 34917 firefox  1663319440.927905 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 34917 firefox  1663319440.951160 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 55989 firefox  1663319441.090189 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 55989 firefox  1663319441.192553 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 74251 firefox  1663319442.105546 GIO   fd 1 wrote 39 bytes
       "returning /home/landry/firefox/firefox
 74251 firefox  1663319442.144554 GIO   fd 1 wrote 39 bytes
       "returning /home/landry/firefox/firefox
 74251 firefox  1663319442.148540 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 74251 firefox  1663319442.152286 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 74251 firefox  1663319442.152318 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 89285 firefox  1663319443.387610 GIO   fd 1 wrote 39 bytes
       "returning /home/landry/firefox/firefox
 89285 firefox  1663319443.436221 GIO   fd 1 wrote 39 bytes
       "returning /home/landry/firefox/firefox
 89285 firefox  1663319443.444805 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 89285 firefox  1663319443.454538 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox
 89285 firefox  1663319443.454577 GIO   fd 1 wrote 52 bytes
       "returning cached value /home/landry/firefox/firefox

Flags: needinfo?(mh+mozilla)

Landry Breuil (:gaston)

Assignee

Comment 9

•

3 years ago

bah, i thought i was there.... but there's not only KERN_PROC_ARGS, there's apparently KERN_PROC_PID from various places (https://searchfox.org/mozilla-central/search?q=kern_proc_pid) which is also forbidden, and that kills the content processes:

89474 firefox  CALL  sysctl(1.66.1.89474.624.1<kern.proc.pid.89474.624.1>,0x7f7ffffef1f0,0x7f7ffffef488,0,0)
89474 firefox  PLDG  sysctl, "", errno 22 Invalid argument
89474 firefox  PSIG  SIGABRT SIG_DFL
89474 firefox  NAMI  "firefox.core"

so i think i'll give up for now trying to remove the 'ps' pledge class from the content process, but apparently my diff allows me to have a working main process without 'ps' in pledge. back to the drawing board....

Landry Breuil (:gaston)

Assignee

Comment 10

•

3 years ago

oh and of course, going to about:processes without 'ps' in pledge classes kills the main process. duh.

Landry Breuil (:gaston)

Assignee

Comment 11

•

3 years ago

Attached file Bug 1790419 - cache BinaryPath value on OpenBSD r=gcp — Details

Landry Breuil (:gaston)

Assignee

Comment 12

•

3 years ago

there's an issue with my patch - my testing wasn't sufficient and doesn't seem to work depending if firefox was called as a full path or not. Back to the drawing board for more++ testing before updating D157554.

Landry Breuil (:gaston)

Assignee

Comment 13

•

3 years ago

hi :gcp, im still not comfortable with phabricator, can you have a look again at https://phabricator.services.mozilla.com/D157554 now that i've updated it with the working version which should address your comments ? Or it can/should be just landed as-is ?

i'd ask for landing it myself, but last i looked, login on lando still required another MFA via an android app i dont plan to install - its a bit sad since i already have MFA setupped in bmo..

Flags: needinfo?(gpascutto)

Gian-Carlo Pascutto [:gcp]

Comment 14

•

3 years ago

Some minor nits, I can land this for you afterwards.

Flags: needinfo?(gpascutto)

Pulsebot

Comment 15

•

3 years ago

Pushed by gpascutto@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/708bd57a7057 cache BinaryPath value on OpenBSD r=gcp

Marian-Vasile Laza

Comment 16

•

3 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/708bd57a7057

Status: NEW → RESOLVED

Closed: 3 years ago

status-firefox107: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → 107 Branch

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

Updated

•

1 year ago

bug-1790419 3 years ago Landry Breuil (:gaston) 3.16 KB, patch		Details \| Diff \| Splinter Review
Bug 1790419 - cache BinaryPath value on OpenBSD r=gcp 3 years ago Landry Breuil (:gaston) 48 bytes, text/x-phabricator-request		Details \| Review

Bugzilla

hoist mozilla::BinaryPath::Get before OpenBSD sandboxing

Categories

(Core :: Security: Process Sandboxing, enhancement, P5)

Tracking

()

People

(Reporter: gaston, Assigned: gaston)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(2 files)

Description

Comment 1

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Updated

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Comment 13

Comment 14

Comment 15

Comment 16

Updated

Attachment

General

Description

File Name

Content Type