From 5e55ab826c463b77f5e80eaeb3805f0dad750427 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Wed, 4 Nov 2026 12:45:25 +1300 Subject: [PATCH 1/5] Reset supplementary groups in `Falcon::Service::Virtual#assume_privileges` when dropping privileges from root. --- lib/falcon/service/virtual.rb | 12 +++++-- releases.md | 4 +++ test/falcon/service/virtual.rb | 57 ++++++++++++++++++++++++++++++++-- 3 files changed, 68 insertions(+), 5 deletions(-) diff --git a/lib/falcon/service/virtual.rb b/lib/falcon/service/virtual.rb index 80006ba..ee86a45 100644 --- a/lib/falcon/service/virtual.rb +++ b/lib/falcon/service/virtual.rb @@ -5,6 +5,7 @@ require "async/service/generic" require "console" +require "etc" module Falcon module Service @@ -29,12 +30,19 @@ def assume_privileges(path) env["PWD"] = File.dirname(path) stat = File.stat(path) + user = Etc.getpwuid(stat.uid) + + # Changing the real/effective uid and gid does not touch the supplementary group list, so a process started as root would otherwise retain root's supplementary groups. `initgroups` requires root, so skip it when running unprivileged (in which case `change_privilege` is a no-op anyway): + if Process.euid == 0 + Process.initgroups(user.name, stat.gid) + end Process::GID.change_privilege(stat.gid) Process::UID.change_privilege(stat.uid) - home = Etc.getpwuid(stat.uid).dir - env["HOME"] = home + env["HOME"] = user.dir + env["USER"] = user.name + env["LOGNAME"] = user.name return env end diff --git a/releases.md b/releases.md index f171001..454fa80 100644 --- a/releases.md +++ b/releases.md @@ -1,5 +1,9 @@ # Releases +## Unreleased + + - `Falcon::Service::Virtual#assume_privileges` now resets supplementary groups with `Process.initgroups` before dropping privileges when running as root, and sets `USER` and `LOGNAME` alongside `HOME` in the child environment. + ## v0.57.0 - Update the Envoy cluster example to use dedicated CDS and EDS services from `async-service-supervisor-envoy` v0.5. diff --git a/test/falcon/service/virtual.rb b/test/falcon/service/virtual.rb index 8d14ec2..ac558a8 100644 --- a/test/falcon/service/virtual.rb +++ b/test/falcon/service/virtual.rb @@ -29,10 +29,36 @@ end with "#assume_privileges" do - it "builds a child environment with path privileges" do - stat = File.stat(path) - user = Etc.getpwuid(stat.uid) + let(:stat) {File.stat(path)} + let(:user) {Etc.getpwuid(stat.uid)} + + def record_privilege_changes(euid) + calls = [] + + mock(Process) do |mock| + mock.replace(:euid){euid} + mock.replace(:initgroups) do |*arguments| + calls << [:initgroups, *arguments] + end + end + + mock(Process::GID) do |mock| + mock.replace(:change_privilege) do |*arguments| + calls << [:gid, *arguments] + end + end + + mock(Process::UID) do |mock| + mock.replace(:change_privilege) do |*arguments| + calls << [:uid, *arguments] + end + end + return calls + end + + it "builds a child environment with path privileges" do + expect(Process).to receive(:euid).and_return(1000) expect(Process::GID).to receive(:change_privilege).with(stat.gid) expect(Process::UID).to receive(:change_privilege).with(stat.uid) @@ -48,6 +74,31 @@ expect(env["PATH"]).to be == "/bin" expect(env["PWD"]).to be == File.dirname(path) expect(env["HOME"]).to be == user.dir + expect(env["USER"]).to be == user.name + expect(env["LOGNAME"]).to be == user.name + end + + it "resets supplementary groups before dropping privileges when running as root" do + calls = record_privilege_changes(0) + + service.assume_privileges(path) + + expect(calls).to be == [ + [:initgroups, user.name, stat.gid], + [:gid, stat.gid], + [:uid, stat.uid], + ] + end + + it "does not reset supplementary groups when running unprivileged" do + calls = record_privilege_changes(1000) + + service.assume_privileges(path) + + expect(calls).to be == [ + [:gid, stat.gid], + [:uid, stat.uid], + ] end end From f8e7d02e1cfade430fadc590b1acdc7ab15a209d Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Wed, 4 Nov 2026 13:13:17 +1300 Subject: [PATCH 2/5] Refuse to run an application as root or from a directory owned by a different user in `Falcon::Service::Virtual#assume_privileges`. --- lib/falcon/service/virtual.rb | 20 +++++++++++++++++++- releases.md | 1 + test/falcon/service/virtual.rb | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/lib/falcon/service/virtual.rb b/lib/falcon/service/virtual.rb index ee86a45..1086fd5 100644 --- a/lib/falcon/service/virtual.rb +++ b/lib/falcon/service/virtual.rb @@ -15,8 +15,12 @@ module Service # A virtual host is an application bound to a specific authority (essentially a hostname). The virtual controller manages multiple hosts and allows a single server to host multiple applications easily. class Virtual < Async::Service::Generic # Drop privileges according to the user and group of the specified path. + # + # The path must be owned by a non-root user who also owns the containing directory. Running an application as root is refused: `File.stat` follows symbolic links, and `bundle exec` evaluates the application's `gems.rb` before the target is loaded, so a user who controls the application directory could otherwise execute arbitrary code as root. The containing directory must have the same owner as the file, since whoever owns the directory can replace the file with arbitrary content. + # # @parameter path [String] The path to the application directory. # @returns [Hash] The environment to use for the spawned process. + # @raises [ArgumentError] If the path is owned by root, or the containing directory is owned by a different user. def assume_privileges(path) # Process.exec / Process.spawn don't replace the environment but instead update it, so we need to clear out any existing BUNDLE_ variables using `nil` values, which will cause them to be removed from the child environment: env = ENV.to_h do |key, value| @@ -27,9 +31,23 @@ def assume_privileges(path) end end - env["PWD"] = File.dirname(path) + directory = File.dirname(path) + env["PWD"] = directory + # `File.stat` follows symbolic links, so this is the owner of the file that will actually be loaded: stat = File.stat(path) + + if stat.uid == 0 + raise ArgumentError, "Refusing to run #{path} as root!" + end + + # The owner of the directory can replace the file, so it must be the same user: + directory_stat = File.stat(directory) + + if directory_stat.uid != stat.uid + raise ArgumentError, "Refusing to run #{path} as the containing directory is owned by a different user!" + end + user = Etc.getpwuid(stat.uid) # Changing the real/effective uid and gid does not touch the supplementary group list, so a process started as root would otherwise retain root's supplementary groups. `initgroups` requires root, so skip it when running unprivileged (in which case `change_privilege` is a no-op anyway): diff --git a/releases.md b/releases.md index 454fa80..7eb7035 100644 --- a/releases.md +++ b/releases.md @@ -3,6 +3,7 @@ ## Unreleased - `Falcon::Service::Virtual#assume_privileges` now resets supplementary groups with `Process.initgroups` before dropping privileges when running as root, and sets `USER` and `LOGNAME` alongside `HOME` in the child environment. + - `Falcon::Service::Virtual#assume_privileges` now refuses to run an application whose configuration file is owned by root, or whose containing directory is owned by a different user than the file. ## v0.57.0 diff --git a/test/falcon/service/virtual.rb b/test/falcon/service/virtual.rb index ac558a8..24c342a 100644 --- a/test/falcon/service/virtual.rb +++ b/test/falcon/service/virtual.rb @@ -57,6 +57,40 @@ def record_privilege_changes(euid) return calls end + # Replace `File.stat` so that ownership checks can be exercised without changing real file ownership. + # @parameter owners [Hash(String, Integer)] The uid to report for each path. + def mock_file_owners(owners) + stat_class = Struct.new(:uid, :gid) + + mock(File) do |mock| + mock.replace(:stat) do |path| + stat_class.new(owners.fetch(path), owners.fetch(path)) + end + end + end + + it "refuses to run an application owned by root" do + calls = record_privilege_changes(0) + mock_file_owners(path => 0, File.dirname(path) => 0) + + expect do + service.assume_privileges(path) + end.to raise_exception(ArgumentError, message: be =~ /Refusing to run .* as root/) + + expect(calls).to be(:empty?) + end + + it "refuses to run an application whose directory is owned by a different user" do + calls = record_privilege_changes(0) + mock_file_owners(path => 1000, File.dirname(path) => 1001) + + expect do + service.assume_privileges(path) + end.to raise_exception(ArgumentError, message: be =~ /owned by a different user/) + + expect(calls).to be(:empty?) + end + it "builds a child environment with path privileges" do expect(Process).to receive(:euid).and_return(1000) expect(Process::GID).to receive(:change_privilege).with(stat.gid) From fc6b9ce8fb7c258124e67b344814296aab580fe6 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Wed, 4 Nov 2026 13:13:51 +1300 Subject: [PATCH 3/5] Raise `ArgumentError` when the owner of the application has no passwd entry in `Falcon::Service::Virtual#assume_privileges`. --- lib/falcon/service/virtual.rb | 6 ++++-- releases.md | 2 +- test/falcon/service/virtual.rb | 11 +++++++++++ 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/lib/falcon/service/virtual.rb b/lib/falcon/service/virtual.rb index 1086fd5..833dc0e 100644 --- a/lib/falcon/service/virtual.rb +++ b/lib/falcon/service/virtual.rb @@ -20,7 +20,7 @@ class Virtual < Async::Service::Generic # # @parameter path [String] The path to the application directory. # @returns [Hash] The environment to use for the spawned process. - # @raises [ArgumentError] If the path is owned by root, or the containing directory is owned by a different user. + # @raises [ArgumentError] If the path is owned by root, the containing directory is owned by a different user, or the owner has no passwd entry. def assume_privileges(path) # Process.exec / Process.spawn don't replace the environment but instead update it, so we need to clear out any existing BUNDLE_ variables using `nil` values, which will cause them to be removed from the child environment: env = ENV.to_h do |key, value| @@ -48,7 +48,9 @@ def assume_privileges(path) raise ArgumentError, "Refusing to run #{path} as the containing directory is owned by a different user!" end - user = Etc.getpwuid(stat.uid) + unless user = Etc.getpwuid(stat.uid) + raise ArgumentError, "No passwd entry for uid #{stat.uid} (owner of #{path})!" + end # Changing the real/effective uid and gid does not touch the supplementary group list, so a process started as root would otherwise retain root's supplementary groups. `initgroups` requires root, so skip it when running unprivileged (in which case `change_privilege` is a no-op anyway): if Process.euid == 0 diff --git a/releases.md b/releases.md index 7eb7035..5df6f2f 100644 --- a/releases.md +++ b/releases.md @@ -3,7 +3,7 @@ ## Unreleased - `Falcon::Service::Virtual#assume_privileges` now resets supplementary groups with `Process.initgroups` before dropping privileges when running as root, and sets `USER` and `LOGNAME` alongside `HOME` in the child environment. - - `Falcon::Service::Virtual#assume_privileges` now refuses to run an application whose configuration file is owned by root, or whose containing directory is owned by a different user than the file. + - `Falcon::Service::Virtual#assume_privileges` now refuses to run an application whose configuration file is owned by root, whose containing directory is owned by a different user than the file, or whose owner has no passwd entry. ## v0.57.0 diff --git a/test/falcon/service/virtual.rb b/test/falcon/service/virtual.rb index 24c342a..c31d76e 100644 --- a/test/falcon/service/virtual.rb +++ b/test/falcon/service/virtual.rb @@ -91,6 +91,17 @@ def mock_file_owners(owners) expect(calls).to be(:empty?) end + it "refuses to run an application whose owner has no passwd entry" do + calls = record_privilege_changes(0) + expect(Etc).to receive(:getpwuid).with(stat.uid).and_return(nil) + + expect do + service.assume_privileges(path) + end.to raise_exception(ArgumentError, message: be =~ /No passwd entry for uid #{stat.uid}/) + + expect(calls).to be(:empty?) + end + it "builds a child environment with path privileges" do expect(Process).to receive(:euid).and_return(1000) expect(Process::GID).to receive(:change_privilege).with(stat.gid) From 8982c4a4a927e307da9bee5982b2f74bf950fa1b Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Wed, 4 Nov 2026 13:16:06 +1300 Subject: [PATCH 4/5] Clarify the documentation and comments for `Falcon::Service::Virtual#assume_privileges`. --- lib/falcon/service/virtual.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/falcon/service/virtual.rb b/lib/falcon/service/virtual.rb index 833dc0e..997b54d 100644 --- a/lib/falcon/service/virtual.rb +++ b/lib/falcon/service/virtual.rb @@ -18,7 +18,9 @@ class Virtual < Async::Service::Generic # # The path must be owned by a non-root user who also owns the containing directory. Running an application as root is refused: `File.stat` follows symbolic links, and `bundle exec` evaluates the application's `gems.rb` before the target is loaded, so a user who controls the application directory could otherwise execute arbitrary code as root. The containing directory must have the same owner as the file, since whoever owns the directory can replace the file with arbitrary content. # - # @parameter path [String] The path to the application directory. + # When running as root, the supplementary group list is reset with `Process.initgroups(user.name, stat.gid)`, which yields the user's `/etc/group` memberships plus the file's group; the user's primary group from the passwd entry is only included if it is the file's group or is also listed in `/etc/group`. + # + # @parameter path [String] The path to the application's `falcon.rb` configuration file. # @returns [Hash] The environment to use for the spawned process. # @raises [ArgumentError] If the path is owned by root, the containing directory is owned by a different user, or the owner has no passwd entry. def assume_privileges(path) @@ -52,7 +54,7 @@ def assume_privileges(path) raise ArgumentError, "No passwd entry for uid #{stat.uid} (owner of #{path})!" end - # Changing the real/effective uid and gid does not touch the supplementary group list, so a process started as root would otherwise retain root's supplementary groups. `initgroups` requires root, so skip it when running unprivileged (in which case `change_privilege` is a no-op anyway): + # Changing the real/effective uid and gid does not touch the supplementary group list, so a process started as root would otherwise retain root's supplementary groups. `initgroups` requires root, so skip it when running unprivileged; in that case `change_privilege` only succeeds if the file is owned by the current user (and group), otherwise it raises `EPERM`: if Process.euid == 0 Process.initgroups(user.name, stat.gid) end @@ -68,7 +70,7 @@ def assume_privileges(path) end # Spawn an application instance from the specified path. - # @parameter path [String] The path to the application directory. + # @parameter path [String] The path to the application's `falcon.rb` configuration file. # @parameter container [Async::Container::Generic] The container to spawn into. # @parameter options [Options] The options which are passed to `exec`. def spawn(path, container, **options) From 1f8bac906dd549f39ce07059d4be51e0fd3bdfaf Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 5 Nov 2026 23:00:15 +1300 Subject: [PATCH 5/5] Shorten the `assume_privileges` documentation. --- lib/falcon/service/virtual.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/falcon/service/virtual.rb b/lib/falcon/service/virtual.rb index 997b54d..93f6acf 100644 --- a/lib/falcon/service/virtual.rb +++ b/lib/falcon/service/virtual.rb @@ -16,9 +16,7 @@ module Service class Virtual < Async::Service::Generic # Drop privileges according to the user and group of the specified path. # - # The path must be owned by a non-root user who also owns the containing directory. Running an application as root is refused: `File.stat` follows symbolic links, and `bundle exec` evaluates the application's `gems.rb` before the target is loaded, so a user who controls the application directory could otherwise execute arbitrary code as root. The containing directory must have the same owner as the file, since whoever owns the directory can replace the file with arbitrary content. - # - # When running as root, the supplementary group list is reset with `Process.initgroups(user.name, stat.gid)`, which yields the user's `/etc/group` memberships plus the file's group; the user's primary group from the passwd entry is only included if it is the file's group or is also listed in `/etc/group`. + # The path and its containing directory must be owned by the same non-root user, since that user's code is executed with the resulting privileges. # # @parameter path [String] The path to the application's `falcon.rb` configuration file. # @returns [Hash] The environment to use for the spawned process. @@ -54,7 +52,7 @@ def assume_privileges(path) raise ArgumentError, "No passwd entry for uid #{stat.uid} (owner of #{path})!" end - # Changing the real/effective uid and gid does not touch the supplementary group list, so a process started as root would otherwise retain root's supplementary groups. `initgroups` requires root, so skip it when running unprivileged; in that case `change_privilege` only succeeds if the file is owned by the current user (and group), otherwise it raises `EPERM`: + # Changing the uid and gid does not touch the supplementary groups, which would otherwise remain root's. `initgroups` requires root, so skip it when running unprivileged: if Process.euid == 0 Process.initgroups(user.name, stat.gid) end