Grep on /etc/init/* shows many exec on non fully qualified binaries as shown in the attachment.

Deleted: exec_binary.txt 4.8 KB

exec_binary.txt 4.8 KB View Download

Hm, it seems that we should only allow whitelisted environment variables to be passed (if we need any at all)?

So I've taken a look at the upstart source code and it allows callers to set arbitrary command environment variables indeed. PATH is probably just the tip of the iceberg since environment variables can influence shell behavior and many system tools consume obscure environment variables (a quick search turned up MODULECONF as a potentially interesting target).

Let me try what happens if I just disable all environment. A cursory search turned up one instance where things would break: https://chromium.git.corp.google.com/chromiumos/platform2/+/acb5e27c7fba97d918783efbf8f879d192c6b299/arc/network/init/arc-network.conf#47

in the sync today, we agreed we can't disable env vars in general, even via the dbus API (as we use that in some places).

but, it should be possible to only allow changing env vars that were explicitly whitelisted in the upstart scripts themselves (via the "env" keyword).  all the uses off the top of my head would be covered by that, and we can easily extend it as needed on a per-service basis.

Who should be the owner of this bug?

hmm, from looking at init scripts, the "env" keyword might be overly permissive for attackers :/.  we have some scripts that use it as a way of setting a shared constant across stanzas.

https://chromium.googlesource.com/chromiumos/third_party/adhd/+/release-R66-10452.B/init/cras.conf
env CRAS_SOCKET_DIR=/run/cras

i think we'll need to define a CrOS-specific keyword here so init scripts can whitelist specific vars that they want/expect to be overridden by dbus (vs being overridden in /etc/init/*.override files that reside on the rootfs).

I agree with what Mike says in #11. I was planning to run an experiment with just log output showing where we actually pass env variables to get a better idea. Didn't get around to doing that since I'm still busy with  issue 818138 .

A possible minimalist approach for whitelisting could be to only let variables through that carry a distinguished prefix, such as CHROMEOS_. That'll require variable name changes though.

OK, so I've added logging to print out whatever environment variables we receive. I don't see any passed env variables when booting to the login screen in a VM.

Another thing to consider: There is this concept of instance variables in upstart (see http://upstart.ubuntu.com/cookbook/#instance). For example, our servod.conf declares PORT as an instance variable. Maybe we can restrict to allow environment variables that are referenced in the instance stanza only? We'd have to find a way to determine which vars are referenced by the instance stanza argument.

I think the conclusion here is that it's probably best to add a keyword to whitelist environment variables to allow via IPC. Name suggestions? "import" maybe? I'll try and take a stab at this, but might take until Monday. So if someone else wants to pick it up, please do so.

One more complication: com.ubuntu.Upstart.EmitEvent also accepts environment variables but isn't processed in the context of a job. We might be able to handle this correctly though if the environment is merely passed through to job transition handling triggered by the event and not used elsewhere.

OK, here's a first stab at a patch: https://chromium-review.googlesource.com/#/c/chromiumos/overlays/chromiumos-overlay/+/957732

This doesn't address the upstart_emit_event call yet (which I believe passes environment variables through to job creation as well). Neither does it fix up the existing usage of deliberately imported environment variables.

And here's confirmation that upstart_emit_event is problematic as well. For example, the following will execute whatever job is declared to start on foobarbaz and will pass the environment through to the script:

initctl emit foobarbaz PATH=/fakepath:$PATH

So we'll need filtering on that path as well.

New version of the CL now filters event environment as well: https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/957732

I still need to update a couple init scripts to not break stuff (the last try run shows ARC++ broken as expected).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/f3e73ac1927931bd8cbb6514bdd2a60bf934ac34

commit f3e73ac1927931bd8cbb6514bdd2a60bf934ac34
Author: Mattias Nissler <mnissler@chromium.org>
Date: Wed Apr 04 14:32:12 2018

sys-apps/upstart: Support import declarations

Add a local patch that allows upstart jobs to declare environment
variables they'd like to import into the job's environment.

BUG= chromium:818032 
TEST=Builds and boots.

Change-Id: I0a20ea333d9921a1290cfaa0b9131e348438da61
Reviewed-on: https://chromium-review.googlesource.com/957732
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>

[modify] https://crrev.com/f3e73ac1927931bd8cbb6514bdd2a60bf934ac34/sys-apps/upstart/upstart-1.2.ebuild
[add] https://crrev.com/f3e73ac1927931bd8cbb6514bdd2a60bf934ac34/sys-apps/upstart/files/upstart-1.2-import-env.patch
[rename] https://crrev.com/f3e73ac1927931bd8cbb6514bdd2a60bf934ac34/sys-apps/upstart/upstart-1.2-r22.ebuild

is this fixed or is the patch in comment 19 only part of the solution?

Not fixed yet, more changes in progress.

Adding code reviewers to CC so they can see context.

Adding more potential reviewers for https://chromium-review.googlesource.com/c/chromiumos/third_party/hdctools/+/1016902

+jrbarnette since this is labstation functionality.

> When invoking an Upstart job, an unprivileged process can specify a PATH environment [ ... ]

Why are we allowing unprivileged processes to start or stop jobs in
the first place?  Last I knew, that required privilege.

You can whitelist users for start/stop access with a dbus configuration fragment.

> You can whitelist users for start/stop access with a dbus configuration fragment.

Where, when, and why have we done that?  In any event, the whitelist
can be restricted to specific users for specific jobs, right?

Re comments #25-#27:

Here are examples where Chrome starts a service: https://cs.corp.google.com/chromeos_public/src/platform2/authpolicy/etc/dbus-1/org.chromium.AuthPolicy.conf?rcl=f6adced95b66d10465bfcfc9a07a8f5a345a212a&l=36
https://cs.corp.google.com/chromeos_internal/src/private-overlays/project-cfm-private/chromeos-base/mri_package/files/org.chromium.MediaPerception.conf?rcl=dd880aaac0d6dbb67c837ca05b27c703d6d5f842&l=25

We had allowed these in the security reviews under the assumption that this just allows chronos to start these specific jobs (which is conceptually OK), but nobody had noticed the environment variables loophole.

That is not what led to this solution though. The problem is that even deprivileged root processes *(uid=0) that are running in a sandbox and don't have capabilities can use env vars to break out and gain full privilege by starting whatever other job they can manipulate with suitable env vars to run a binary of the attacker's choice. See  issue 817920  for more details, the scenario described there is breaking out of the crash_sender sandbox. Some stuff will have to keep running as root, i.e. uid=0 for various reasons, but within a sandbox. The upstart change closes the sandbox escape via DBus calls to upstart.

From a design perspective, upstart allowing to inject arbitrary environment variables into jobs via start/stop and event IPC is just a huge footgun. I can relate to the sentiment of rather not changing upstart if we don't have to. We've previously discussed this point with vapier@ and the security team and came to the conclusion that making the upstart change is the best option we could come up with. Note that I'd happily work with upstream to contribute this change (or something else that solves the problem), but upstream is dead.

> Why are we allowing unprivileged processes to start or stop jobs in the first place?  Last I knew, that required privilege.

i think you're conflating the word "privilege" here.  "having privileges" is not the same thing as "being root" (which itself is imprecise because you can be root, e.g. uid=0, and still lack many privs based on active capabilities or other security models such as selinux).

so in a sense, as mnissler@ highlighted, we grant access ("privilege") to many non-root accounts to send upstart signals/events including stopping/starting services.  such as the "chronos" user so that Chrome browser can trigger a restricted set of things on the system.  this, in and of itself, isn't a bug but quite desirable.  what would the alternative be ?

in most cases, we are actually restricting access to these to specific users.

> Some stuff will have to keep running as root, i.e. uid=0 for various reasons, but within a sandbox.

Oy.  Yeah, changing upstart does seem to be the inevitable consequence
of fixing that problem.


> Note that I'd happily work with upstream to contribute this change (or something else that solves the problem), but upstream is dead.

<sigh> I was vaguely worried about that.

I'll echo the comment from vapier@ on the CL:  We need to update the man
pages/any documentation for Upstart.  I went looking to read the docs on
the existing ways that Upstart handles environment variables, and discovered
I no longer have man pages anywhere immediately convenient; all there is is
online documents for something that isn't _our_ version of Upstart.  The devs
already have enough problems learning/understanding Upstart even _with_ the
documentation; without docs, disaster will ensue.

Regarding documentation: Ah, I had actually missed Mike's comment regarding updating the docs. Makes sense though, I'll send a separate CL for that. I'm slightly doubtful about man pages and docs from the package being particularly useful though as they're not very discoverable right now. 

FWIW, I had considered logging a warning to syslog with a pointer to a markdown page explaining rationale and giving advice. It turns out though that several jobs receive variables that aren't actually relevant for the job and should not be imported. Thus, triggering log output on environment variables that get passed but don't get imported would just lead to log spam. If anyone has a better idea on how to give developers a better chance to become aware of what's going on, I'm all ears.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/ec26153358de9d7bbf6a83f6648c5e94370e0020

commit ec26153358de9d7bbf6a83f6648c5e94370e0020
Author: Mattias Nissler <mnissler@chromium.org>
Date: Mon Apr 23 13:35:57 2018

servod.conf: Explicitly import environment variables

Declare the environment variables intended to be received from the
event starting the upstart job. This is in preparation to change
upstart to not import environment variables from events by default.

BUG= chromium:818032 
TEST=Compiles and passes tests, start start_servo doesn't complain about missing config file.

Change-Id: Icd481c7134c0609844db324c817eedd3d63c011d
Reviewed-on: https://chromium-review.googlesource.com/1016902
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Richard Barnette <jrbarnette@google.com>

[modify] https://crrev.com/ec26153358de9d7bbf6a83f6648c5e94370e0020/chromeos/servod.conf

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/e145eae5d43d0d3cbbdf89fce81ac43a6e95c903

commit e145eae5d43d0d3cbbdf89fce81ac43a6e95c903
Author: Mattias Nissler <mnissler@chromium.org>
Date: Mon Apr 23 13:35:56 2018

Add import declarations in various init jobs.

This adds import declarations for environment variables that are
actually expected to be supplied externally for starting jobs. This is
a preparation step that will allow us to stop importing undeclared
environment variables.

BUG= chromium:818032 
TEST=Image boots and passes tests.

Change-Id: Idc101eb34502c20d0ed73fbab9c9d209c5eb3534
Reviewed-on: https://chromium-review.googlesource.com/1013986
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>

[modify] https://crrev.com/e145eae5d43d0d3cbbdf89fce81ac43a6e95c903/timberslide/init/timberslide.conf
[modify] https://crrev.com/e145eae5d43d0d3cbbdf89fce81ac43a6e95c903/arc/setup/etc/arc-boot-continue.conf
[modify] https://crrev.com/e145eae5d43d0d3cbbdf89fce81ac43a6e95c903/arc/setup/etc/arc-sensor.conf
[modify] https://crrev.com/e145eae5d43d0d3cbbdf89fce81ac43a6e95c903/arc/network/init/arc-network.conf
[modify] https://crrev.com/e145eae5d43d0d3cbbdf89fce81ac43a6e95c903/arc/scripts/arc-stale-directory-remover.conf

> as they're not very discoverable right now

Discoverability is definitely a problem.  I'd start with making
sure that the man pages are installed in the chroot.  I'm pretty
sure they used to be; they're not any more.

the version/doc sync is already a problem for people ... they look at the latest public upstart docs which reflect a much newer version and offers syntax our version doesn't.  so i was hoping we could generate+host our own copies for devs to refer to instead.

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/overlays/overlay-loonix-private/+/2e4e3029948f9c628b061dfb1ce5f3d105189b8a

commit 2e4e3029948f9c628b061dfb1ce5f3d105189b8a
Author: Mattias Nissler <mnissler@chromium.org>
Date: Thu May 03 05:20:43 2018

Even when we saw the warning message, the issue was still really hard to understand, and I had to manually bisect test results to figure out when the issue started happening (b/79225734), and find CL:957732 (then I could cc mnissler).

Also, I did not understand that this was just a warning (not clear if the env variable was actually ignored or not).

Could we include a link to some public bug in the warning message (or at least Google-restricted bug, not this bug that is even more tightly restricted) with some instructions on how to fix it? Even a link to the CL would already be useful (at least we'd know who to cc).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/3a73295dee9e14cdeb18157a875dfbeffef0140d

commit 3a73295dee9e14cdeb18157a875dfbeffef0140d
Author: Mattias Nissler <mnissler@chromium.org>
Date: Sat May 05 03:37:30 2018

hammerd: Declare imported environment variables in hammerd.conf

This declares AT_BOOT and UPDATE_IF to be imported from the command
or event triggering the hammerd.conf job. This is in preparation to
lock down automatic environment propagation in upstart.

BUG= chromium:818032 , b/79225734
TEST=Builds and passes tests.

Change-Id: Id9e8fff27ab8b018b7f64ddd24b59352694d6bdb
Reviewed-on: https://chromium-review.googlesource.com/1043745
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Nicolas Boichat <drinkcat@chromium.org>

[modify] https://crrev.com/3a73295dee9e14cdeb18157a875dfbeffef0140d/hammerd/init/hammerd.conf

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/a4f4f5337daa23a5da8b317cd9c263aba45cb487

commit a4f4f5337daa23a5da8b317cd9c263aba45cb487
Author: Mattias Nissler <mnissler@chromium.org>
Date: Mon May 07 22:36:02 2018

hammerd: Declare imported environment variables in hammerd.conf

This declares AT_BOOT and UPDATE_IF to be imported from the command
or event triggering the hammerd.conf job. This is in preparation to
lock down automatic environment propagation in upstart.

BUG= chromium:818032 , b/79225734
TEST=Builds and passes tests.

Change-Id: Id9e8fff27ab8b018b7f64ddd24b59352694d6bdb
Reviewed-on: https://chromium-review.googlesource.com/1043745
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Nicolas Boichat <drinkcat@chromium.org>
(cherry picked from commit 3a73295dee9e14cdeb18157a875dfbeffef0140d)
Reviewed-on: https://chromium-review.googlesource.com/1048665
Commit-Queue: Nicolas Boichat <drinkcat@chromium.org>
Tested-by: Nicolas Boichat <drinkcat@chromium.org>

[modify] https://crrev.com/a4f4f5337daa23a5da8b317cd9c263aba45cb487/hammerd/init/hammerd.conf

Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Not fixed yet.

The following revision refers to this bug:
  https://chromium.googlesource.com/aosp/platform/system/connectivity/shill/+/6320093e02cb050ed7eda203c165d6372d1b2934

commit 6320093e02cb050ed7eda203c165d6372d1b2934
Author: Mattias Nissler <mnissler@chromium.org>
Date: Fri May 11 14:02:14 2018

shill: Import CHROMEOS_USER in init job.

Add an import declaration in shill-start-user-session to indicate the
CHROMEOS_USER variable is purposely imported from the event or command
that triggers the job. This is in preparation to restrict propagation
of environment variables into the job environment.

BUG= chromium:818032 
TEST=No log message about undeclared imported variables on login.

Change-Id: I71bbce0941c1ebe964fcbeb99f0dd2274559d1e4
Reviewed-on: https://chromium-review.googlesource.com/1043765
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Kevin Cernekee <cernekee@chromium.org>

[modify] https://crrev.com/6320093e02cb050ed7eda203c165d6372d1b2934/init/shill-start-user-session.conf

The following revision refers to this bug:
  https://chromium.googlesource.com/aosp/platform/system/connectivity/shill/+/73174fa7bb2b666b3cdfb70ef82b2974d77ab2b4

commit 73174fa7bb2b666b3cdfb70ef82b2974d77ab2b4
Author: Mattias Nissler <mnissler@chromium.org>
Date: Fri May 11 14:02:14 2018

shill: Import variables in shill init job.

Whitelist a bunch of parameters that the job allows to pass in via
upstart commands and events.

BUG= chromium:818032 
TEST=Starting shill with the parameters in question doesn't generate undeclared import log messages.

Change-Id: I5825c12ad4c160e15c35f48f9ed1308cadee59fc
Reviewed-on: https://chromium-review.googlesource.com/1049560
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Kevin Cernekee <cernekee@chromium.org>

[modify] https://crrev.com/73174fa7bb2b666b3cdfb70ef82b2974d77ab2b4/init/shill.conf.in

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/f76a0a892777abafece097c444c3e36d035ce906

commit f76a0a892777abafece097c444c3e36d035ce906
Author: Mattias Nissler <mnissler@chromium.org>
Date: Fri May 11 16:30:47 2018

sys-apps/upstart: Restrict environment import.

Change upstart to only import externally supplied variables into the
job environment if the job explicitly declares them using the import
statement.

BUG= chromium:818032 
TEST=Image boots and passes tests.
CQ-DEPEND=CL:1013986

Change-Id: I44b453605232562099c1c1d8a4b3b1a1ddc00b5b
Reviewed-on: https://chromium-review.googlesource.com/1013931
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>

[rename] https://crrev.com/f76a0a892777abafece097c444c3e36d035ce906/sys-apps/upstart/upstart-1.2-r25.ebuild
[modify] https://crrev.com/f76a0a892777abafece097c444c3e36d035ce906/sys-apps/upstart/files/upstart-1.2-import-env.patch

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/d57f78b8ad319d81ceee66392bf8be7ca4b5c498

commit d57f78b8ad319d81ceee66392bf8be7ca4b5c498
Author: Mattias Nissler <mnissler@chromium.org>
Date: Wed May 16 19:41:32 2018

sys-apps/upstart: Document job environment mechanics

Document the recently-added import stanza and its effect and clarify
the construction of the job environment in general.

BUG= chromium:818032 
TEST=Builds.

Change-Id: I94bb230a3318fe45bdc03a1ab639cf6ba299c973
Reviewed-on: https://chromium-review.googlesource.com/1023398
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[rename] https://crrev.com/d57f78b8ad319d81ceee66392bf8be7ca4b5c498/sys-apps/upstart/upstart-1.2-r26.ebuild
[modify] https://crrev.com/d57f78b8ad319d81ceee66392bf8be7ca4b5c498/sys-apps/upstart/files/upstart-1.2-import-env.patch

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/aa71b600eb33750e980c1218f05ece47b9dbd8c2

commit aa71b600eb33750e980c1218f05ece47b9dbd8c2
Author: Mattias Nissler <mnissler@chromium.org>
Date: Wed May 16 19:41:33 2018

sys-apps/ifupdown: Remove

The source code for the version corresponding to the ebuild is no
longer availablev via mirrors, so the package is broken. Given that,
chances are pretty good that noone is actively using this and it can
be removed :-D

BUG= chromium:818032 
TEST=None

Change-Id: Ia42b5d3eac5da518262ddec3197735fcb5cb7049
Reviewed-on: https://chromium-review.googlesource.com/1032934
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>
Reviewed-by: Ian Coolidge <icoolidge@google.com>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[delete] https://crrev.com/d57f78b8ad319d81ceee66392bf8be7ca4b5c498/sys-apps/ifupdown/Manifest
[delete] https://crrev.com/d57f78b8ad319d81ceee66392bf8be7ca4b5c498/sys-apps/ifupdown/ifupdown-0.8.18-r1.ebuild
[delete] https://crrev.com/d57f78b8ad319d81ceee66392bf8be7ca4b5c498/sys-apps/ifupdown/files/network-interface.conf
[delete] https://crrev.com/d57f78b8ad319d81ceee66392bf8be7ca4b5c498/sys-apps/ifupdown/files/ifupdown_0.8.18_assume_linux.patch
[delete] https://crrev.com/d57f78b8ad319d81ceee66392bf8be7ca4b5c498/sys-apps/ifupdown/metadata.xml
[delete] https://crrev.com/d57f78b8ad319d81ceee66392bf8be7ca4b5c498/sys-apps/ifupdown/ifupdown-0.8.18.ebuild

Time to call this fixed.

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/vendor/intel-wifi-fw-dump/+/9f8e02ffcb939d033e1801519257b21779e79da1

commit 9f8e02ffcb939d033e1801519257b21779e79da1
Author: Eric Caruso <ejcaruso@google.com>
Date: Wed May 23 04:57:01 2018

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot