jenkins-bot has submitted this change and it was merged. ( 
https://gerrit.wikimedia.org/r/396570 )

Change subject: shell: Run firejail inside limit.sh, make NO_EXECVE work
......................................................................


shell: Run firejail inside limit.sh, make NO_EXECVE work

NO_EXECVE doesn't work because limit.sh needs to execute the main
command, and does so through the execve syscall. Eventually we should be
able to replace limit.sh with firejail functionality entirely (T179021),
but in the meantime we can run firejail inside limit.sh.

We also need to stop firejail from running the command in a bash shell
via --shell=none, since that shell would also use the execve syscall.

Bug: T182489
Change-Id: I3fc8ad2f9e5eb5bf13b49d0bccd6094668a5ec55
---
M includes/shell/Command.php
M includes/shell/FirejailCommand.php
M tests/phpunit/includes/shell/FirejailCommandTest.php
3 files changed, 21 insertions(+), 17 deletions(-)

Approvals:
  Gergő Tisza: Looks good to me, approved
  jenkins-bot: Verified



diff --git a/includes/shell/Command.php b/includes/shell/Command.php
index 998b3ed..2f0ea42 100644
--- a/includes/shell/Command.php
+++ b/includes/shell/Command.php
@@ -36,7 +36,7 @@
        use LoggerAwareTrait;
 
        /** @var string */
-       private $command = '';
+       protected $command = '';
 
        /** @var array */
        private $limits = [
@@ -269,9 +269,10 @@
         * String together all the options and build the final command
         * to execute
         *
+        * @param string $command Already-escaped command to run
         * @return array [ command, whether to use log pipe ]
         */
-       protected function buildFinalCommand() {
+       protected function buildFinalCommand( $command ) {
                $envcmd = '';
                foreach ( $this->env as $k => $v ) {
                        if ( wfIsWindows() ) {
@@ -291,7 +292,7 @@
                }
 
                $useLogPipe = false;
-               $cmd = $envcmd . trim( $this->command );
+               $cmd = $envcmd . trim( $command );
 
                if ( is_executable( '/bin/bash' ) ) {
                        $time = intval( $this->limits['time'] );
@@ -335,7 +336,7 @@
 
                $profileMethod = $this->method ?: wfGetCaller();
 
-               list( $cmd, $useLogPipe ) = $this->buildFinalCommand();
+               list( $cmd, $useLogPipe ) = $this->buildFinalCommand( 
$this->command );
 
                $this->logger->debug( __METHOD__ . ": $cmd" );
 
diff --git a/includes/shell/FirejailCommand.php 
b/includes/shell/FirejailCommand.php
index 79f679d..0338b53 100644
--- a/includes/shell/FirejailCommand.php
+++ b/includes/shell/FirejailCommand.php
@@ -59,10 +59,10 @@
        /**
         * @inheritDoc
         */
-       protected function buildFinalCommand() {
+       protected function buildFinalCommand( $command ) {
                // If there are no restrictions, don't use firejail
                if ( $this->restrictions === 0 ) {
-                       return parent::buildFinalCommand();
+                       return parent::buildFinalCommand( $command );
                }
 
                if ( $this->firejail === false ) {
@@ -122,6 +122,10 @@
 
                if ( $this->hasRestriction( Shell::NO_EXECVE ) ) {
                        $seccomp[] = 'execve';
+                       // Normally firejail will run commands in a bash shell,
+                       // but that won't work if we ban the execve syscall, so
+                       // run the command without a shell.
+                       $cmd[] = '--shell=none';
                }
 
                if ( $seccomp ) {
@@ -136,11 +140,10 @@
                        $cmd[] = '--net=none';
                }
 
-               list( $fullCommand, $useLogPipe ) = parent::buildFinalCommand();
-
                $builtCmd = implode( ' ', $cmd );
 
-               return [ "$builtCmd -- $fullCommand", $useLogPipe ];
+               // Prefix the firejail command in front of the wanted command
+               return parent::buildFinalCommand( "$builtCmd -- {$command}" );
        }
 
 }
diff --git a/tests/phpunit/includes/shell/FirejailCommandTest.php 
b/tests/phpunit/includes/shell/FirejailCommandTest.php
index c9db74f..fab14ca 100644
--- a/tests/phpunit/includes/shell/FirejailCommandTest.php
+++ b/tests/phpunit/includes/shell/FirejailCommandTest.php
@@ -29,38 +29,38 @@
                // @codingStandardsIgnoreStart
                $env = "'MW_INCLUDE_STDERR=;MW_CPU_LIMIT=180; 
MW_CGROUP='\'''\''; MW_MEM_LIMIT=307200; MW_FILE_SIZE_LIMIT=102400; 
MW_WALL_CLOCK_LIMIT=180; MW_USE_LOG_PIPE=yes'";
                // @codingStandardsIgnoreEnd
-               $limit = "$IP/includes/shell/limit.sh";
+               $limit = "/bin/bash '$IP/includes/shell/limit.sh'";
                $profile = "--profile=$IP/includes/shell/firejail.profile";
                $default = '--noroot --seccomp=@default --private-dev';
                return [
                        [
                                'No restrictions',
-                               'ls', 0, "/bin/bash '$limit' ''\''ls'\''' $env"
+                               'ls', 0, "$limit ''\''ls'\''' $env"
                        ],
                        [
                                'default restriction',
                                'ls', Shell::RESTRICT_DEFAULT,
-                               "firejail --quiet $profile $default -- 
/bin/bash '$limit' ''\''ls'\''' $env"
+                               "$limit 'firejail --quiet $profile $default -- 
'\''ls'\''' $env"
                        ],
                        [
                                'no network',
                                'ls', Shell::NO_NETWORK,
-                               "firejail --quiet $profile --net=none -- 
/bin/bash '$limit' ''\''ls'\''' $env"
+                               "$limit 'firejail --quiet $profile --net=none 
-- '\''ls'\''' $env"
                        ],
                        [
                                'default restriction & no network',
                                'ls', Shell::RESTRICT_DEFAULT | 
Shell::NO_NETWORK,
-                               "firejail --quiet $profile $default --net=none 
-- /bin/bash '$limit' ''\''ls'\''' $env"
+                               "$limit 'firejail --quiet $profile $default 
--net=none -- '\''ls'\''' $env"
                        ],
                        [
                                'seccomp',
                                'ls', Shell::SECCOMP,
-                               "firejail --quiet $profile --seccomp=@default 
-- /bin/bash '$limit' ''\''ls'\''' $env"
+                               "$limit 'firejail --quiet $profile 
--seccomp=@default -- '\''ls'\''' $env"
                        ],
                        [
                                'seccomp & no execve',
                                'ls', Shell::SECCOMP | Shell::NO_EXECVE,
-                               "firejail --quiet $profile 
--seccomp=@default,execve -- /bin/bash '$limit' ''\''ls'\''' $env"
+                               "$limit 'firejail --quiet $profile --shell=none 
--seccomp=@default,execve -- '\''ls'\''' $env"
                        ],
                ];
        }
@@ -75,7 +75,7 @@
                        ->params( $params )
                        ->restrict( $flags );
                $wrapper = TestingAccessWrapper::newFromObject( $command );
-               $output = $wrapper->buildFinalCommand();
+               $output = $wrapper->buildFinalCommand( $wrapper->command );
                $this->assertEquals( $expected, $output[0], $desc );
        }
 

-- 
To view, visit https://gerrit.wikimedia.org/r/396570
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I3fc8ad2f9e5eb5bf13b49d0bccd6094668a5ec55
Gerrit-PatchSet: 4
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Legoktm <[email protected]>
Gerrit-Reviewer: GergÅ‘ Tisza <[email protected]>
Gerrit-Reviewer: Legoktm <[email protected]>
Gerrit-Reviewer: MaxSem <[email protected]>
Gerrit-Reviewer: Tim Starling <[email protected]>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to