Sanitize GitHub action shell cmd to prevent cmd injection #3509 (#3510)

* remove github action scan folder after index-js is built #3499 * save the github action index.js as sechub-scan.cjs * save the github action index.js as sechub-scan.cjs * prevent command injection in sechub-cli.ts * add doc to shell-cmd-sanitizer.ts * use whitelist in github action to prevent command injection * revert action.yml changes temporarily * pr clean up * pr clean up * use child_process execFileSync to pass commands to go client in array * pass process.env to execFileSync in GitHub Action * pass process.env to execFileSync in GitHub Action * update versions used in 01-start.sh github action * protect against shell arguments that are commands in github actions * replace potentially dangerous shell command injection code * use commandExists npm library to check if shell argument is a malicious command * use commandExists npm library to check if shell argument is a malicious command * use commandExists npm library to check if shell argument is a malicious command * fix integration tests * revert info logs to debug * revert info logs to debug
mercedes-benz · Oct 17, 2024 · cdb5c25 · cdb5c25
1 parent 8b5baa1
commit cdb5c25
Show file tree

Hide file tree

Showing 13 changed files with 1,209 additions and 1,042 deletions.
diff --git a/github-actions/scan/__test__/configuration-builder.test.ts b/github-actions/scan/__test__/configuration-builder.test.ts
@@ -6,7 +6,6 @@ import { SecHubConfigurationModelBuilderData } from '../src/configuration-builde
 
 jest.mock('@actions/core');
 
-
 function dumpModel(model: SecHubConfigurationModel){
     const json = JSON.stringify(model, null, 2); // pretty printed output
 

diff --git a/github-actions/scan/__test__/integrationtest/01-start.sh b/github-actions/scan/__test__/integrationtest/01-start.sh
@@ -19,7 +19,7 @@ set -e
 # Example:
 # ```
 # cd $gitRoot/github-actions/scan
-# ./01-start.sh 1.7.0 8443 1.4.0 8444
+# ./01-start.sh 2.2.0 8443 2.0.0 8444
 # ```
 #
 SERVER_VERSION=$1

diff --git a/github-actions/scan/__test__/integrationtest/start_pds.sh b/github-actions/scan/__test__/integrationtest/start_pds.sh
@@ -40,6 +40,8 @@ fi
 
 SCRIPT_DIR="$(dirname -- "$0")"
 
+export PDS_STORAGE_SHAREDVOLUME_UPLOAD_DIR=$SHARED_VOLUME
+
 echo "Start PDS at localhost:${SERVER_PORT}, executable at: ${PATH_TO_EXECUTABLE}"
 # `curl -s --insecure https://localhost:${this.serverPort}/api/anonymous/check/alive`;
 java \
@@ -48,6 +50,5 @@ java \
  -Dpds.config.heartbeat.verbose.logging.enabled=false \
  -Dserver.ssl.key-store="${PATH_TO_CERTIFICATE}" \
  -Dserver.port="${SERVER_PORT}" \
- -Dpds.storage.sharedvolume.upload.dir="$SHARED_VOLUME" \
  -Dpds.config.file="$PDS_CONFIG_FILE" \
  -jar "${PATH_TO_EXECUTABLE}">>"${PATH_TO_LOGFILE}" &
diff --git a/github-actions/scan/__test__/integrationtest/start_sechub_server.sh b/github-actions/scan/__test__/integrationtest/start_sechub_server.sh
@@ -32,9 +32,10 @@ if [ "$SHARED_VOLUME" = "" ]; then
     exit 1
 fi
 
-
 SCRIPT_DIR="$(dirname -- "$0")"
 
+export SECHUB_STORAGE_SHAREDVOLUME_UPLOAD_DIR=$SHARED_VOLUME
+
 echo "Start SecHub server at localhost:${SERVER_PORT}, executable at: ${PATH_TO_EXECUTABLE}"
 # `curl -s --insecure https://localhost:${this.serverPort}/api/anonymous/check/alive`;
 java \
@@ -44,5 +45,4 @@ java \
  -Dsechub.server.debug=true \
  -Dserver.port="${SERVER_PORT}" \
  -Dsechub.integrationtest.ignore.missing.serverproject=true \
- -Dsechub.storage.sharedvolume.upload.dir="$SHARED_VOLUME" \
  -jar "${PATH_TO_EXECUTABLE}">>"${PATH_TO_LOGFILE}" &
diff --git a/github-actions/scan/__test__/sechub-cli.test.ts b/github-actions/scan/__test__/sechub-cli.test.ts
@@ -1,8 +1,8 @@
 // SPDX-License-Identifier: MIT
 
-import * as cli from '../src/sechub-cli';
-import { scan } from '../src/sechub-cli';
-import * as shell from 'shelljs';
+import {extractJobUUID, getReport, scan} from '../src/sechub-cli';
+import {execFileSync} from 'child_process';
+import {sanitize} from "../src/shell-arg-sanitizer";
 
 jest.mock('@actions/core');
 
@@ -18,21 +18,44 @@ const output = `
         other
         `;
 
-jest.mock('shelljs', () => ({
-    exec: jest.fn(() => ({
-        code: 0,
-        stdout: output,
-        stderr: ''
-    }))
+jest.mock('child_process', () => ({
+    execFileSync: jest.fn(() => output)
 }));
 
+jest.mock('../src/shell-arg-sanitizer');
+
 beforeEach(() => {
     jest.clearAllMocks();
 });
 
-describe('sechub-cli', function() {
+describe('scan', function() {
 
-    it('scan - return correct job id', function () {
+    it('sanitizes shell arguments', () => {
+        /* prepare */
+        const context: any = {
+            clientExecutablePath: '/path/to/sechub-cli',
+            configFileLocation: '/path/to/config.json',
+            workspaceFolder: '/path/to/workspace',
+            inputData: {
+                addScmHistory: 'false'
+            }
+        };
+        (sanitize as jest.Mock).mockImplementation((arg) => {
+            return arg;
+        });
+
+        /* execute */
+        scan(context);
+
+        /* test */
+        expect(sanitize).toBeCalledTimes(4);
+        expect(sanitize).toBeCalledWith('/path/to/sechub-cli');
+        expect(sanitize).toBeCalledWith('/path/to/config.json');
+        expect(sanitize).toBeCalledWith('/path/to/workspace');
+        expect(sanitize).toBeCalledWith('');
+    });
+
+    it('return correct job id', function () {
         /* prepare */
         const context: any = {
             clientExecutablePath: '/path/to/sechub-cli',
@@ -51,7 +74,7 @@ describe('sechub-cli', function() {
         expect(context.jobUUID).toEqual('6880e518-88db-406a-bc67-851933e7e5b7');
     });
 
-    it('scan - with addScmHistory flag true - executes SecHub client with -addScmHistory', function () {
+    it('with addScmHistory flag true - executes SecHub client with -addScmHistory', function () {
         /* prepare */
         const context: any = {
             clientExecutablePath: '/path/to/sechub-cli',
@@ -66,11 +89,25 @@ describe('sechub-cli', function() {
         scan(context);
 
         /* test */
-        expect(shell.exec).toBeCalledTimes(1);
-        expect(shell.exec).toBeCalledWith('/path/to/sechub-cli -configfile /path/to/config.json -output /path/to/workspace -addScmHistory scan');
+        expect(execFileSync).toBeCalledTimes(1);
+        expect(execFileSync)
+            .toBeCalledWith(
+                '/path/to/sechub-cli', ['-configfile', '/path/to/config.json', '-output', '/path/to/workspace', '-addScmHistory', 'scan'],
+                {
+                    env: {
+                        SECHUB_SERVER: process.env.SECHUB_SERVER,
+                        SECHUB_USERID: process.env.SECHUB_USERID,
+                        SECHUB_APITOKEN: process.env.SECHUB_APITOKEN,
+                        SECHUB_PROJECT: process.env.SECHUB_PROJECT,
+                        SECHUB_DEBUG: process.env.SECHUB_DEBUG,
+                        SECHUB_TRUSTALL: process.env.SECHUB_TRUSTALL,
+                    },
+                    encoding: 'utf-8'
+                }
+            );
     });
 
-    it('scan - with addScmHistory flag false - executes SecHub client without -addScmHistory', function () {
+    it('with addScmHistory flag false - executes SecHub client without -addScmHistory', function () {
         /* prepare */
         const context: any = {
             clientExecutablePath: '/path/to/sechub-cli',
@@ -85,11 +122,29 @@ describe('sechub-cli', function() {
         scan(context);
 
         /* test */
-        expect(shell.exec).toBeCalledTimes(1);
-        expect(shell.exec).toBeCalledWith('/path/to/sechub-cli -configfile /path/to/config.json -output /path/to/workspace scan');
+        expect(execFileSync).toBeCalledTimes(1);
+        expect(execFileSync)
+            .toBeCalledWith(
+                '/path/to/sechub-cli', ['-configfile', '/path/to/config.json', '-output', '/path/to/workspace', '', 'scan'],
+                {
+                    env: {
+                        SECHUB_SERVER: process.env.SECHUB_SERVER,
+                        SECHUB_USERID: process.env.SECHUB_USERID,
+                        SECHUB_APITOKEN: process.env.SECHUB_APITOKEN,
+                        SECHUB_PROJECT: process.env.SECHUB_PROJECT,
+                        SECHUB_DEBUG: process.env.SECHUB_DEBUG,
+                        SECHUB_TRUSTALL: process.env.SECHUB_TRUSTALL,
+                    },
+                    encoding: 'utf-8'
+                }
+            );
     });
 
-    it('extractJobUUID - returns job uuid from sechub client output snippet', function () {
+});
+
+describe('extractJobUUID', function () {
+
+    it('returns job uuid from sechub client output snippet', function () {
 
         const output = `
         WARNING: Configured to trust all - means unknown service certificate is accepted. Don't use this in production!
@@ -104,28 +159,28 @@ describe('sechub-cli', function() {
         `;
 
         /* execute */
-        const jobUUID= cli.extractJobUUID(output);
+        const jobUUID= extractJobUUID(output);
 
         /* test */
         expect(jobUUID).toEqual('6880e518-88db-406a-bc67-851933e7e5b7');
     });
-    
-    it('extractJobUUID - returns job uuid from string with "job: xxxx"', function () {
-        
+
+    it('returns job uuid from string with "job: xxxx"', function () {
+
         const output = `
         The uuid for job:1234
         can be extracted
         `;
 
         /* execute */
-        const jobUUID= cli.extractJobUUID(output);
+        const jobUUID= extractJobUUID(output);
 
         /* test */
         expect(jobUUID).toEqual('1234');
     });
 
-    it('extractJobUUID - returns empty string when no job id is available', function () {
-        
+    it('returns empty string when no job id is available', function () {
+
         const output = `
         WARNING: Configured to trust all - means unknown service certificate is accepted. Don't use this in production!
         2024-03-08 13:58:18 (+01:00) Zipping folder: __test__/integrationtest/test-sources (/home/xyzgithub-actions/scan/__test__/integrationtest/test-sources)
@@ -135,9 +190,34 @@ describe('sechub-cli', function() {
         `;
 
         /* execute */
-        const jobUUID= cli.extractJobUUID(output);
+        const jobUUID= extractJobUUID(output);
 
         /* test */
         expect(jobUUID).toEqual('');
     });
+});
+
+describe('getReport', function () {
+
+    it('sanitizes shell arguments', () => {
+        /* prepare */
+        const context: any = {
+            clientExecutablePath: '/path/to/sechub-cli',
+            projectName: 'project-name',
+        };
+        (sanitize as jest.Mock).mockImplementation((arg) => {
+            return arg;
+        });
+
+        /* execute */
+        getReport('job-uuid', 'json', context);
+
+        /* test */
+        expect(sanitize).toBeCalledTimes(4);
+        expect(sanitize).toBeCalledWith('/path/to/sechub-cli');
+        expect(sanitize).toBeCalledWith('job-uuid');
+        expect(sanitize).toBeCalledWith('project-name');
+        expect(sanitize).toBeCalledWith('json');
+    });
+
 });
diff --git a/github-actions/scan/__test__/shell-arg-sanitizer.test.ts b/github-actions/scan/__test__/shell-arg-sanitizer.test.ts
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: MIT
+
+import * as shellArgSanitizer from '../src/shell-arg-sanitizer';
+
+describe('sanitize', () => {
+    test.each([
+        ['rm -rf /; echo hacked'], // Command chaining
+        ['echo $(whoami)'], // Command substitution
+        ['cat /etc/passwd | grep root'], // Piping
+        ['touch /tmp/test && ls /tmp'], // Logical AND
+        ['echo hello > /tmp/test'], // Redirection
+        ['`reboot`'], // Backticks
+        ['$(reboot)'], // Subshell
+        ['; reboot'], // Semicolon
+        ['| reboot'], // Pipe
+        ['& reboot'], // Background process
+        ['> /dev/null'], // Redirection to null
+        ['< /dev/null'], // Input redirection
+        ['|| reboot'], // Logical OR
+        ['&& reboot'], // Logical AND
+        ['$(< /etc/passwd)'], // Command substitution with input redirection
+        ['$(cat /etc/passwd)'], // Command substitution with cat
+        ['$(echo hello > /tmp/test)'], // Command substitution with redirection
+        ['$(touch /tmp/test && ls /tmp)'], // Command substitution with logical AND
+        ['$(cat /etc/passwd | grep root)'], // Command substitution with pipe
+        ['$(rm -rf /; echo hacked)'],
+        ['kill'],
+        ['sleep'],
+        ['shutdown'],
+        ['reboot'],
+        ['halt'],
+        ['ps'],
+        ['top'],
+        ['killall'],
+        ['pkill'],
+        ['pgrep'],
+        ['chown'],
+        ['chmod'],
+        ['chgrp'],
+        ['passwd'],
+        ['su'],
+        ['sudo'],
+        ['chsh'],
+        ['chfn'],
+        ['chroot']
+    ])(
+        '%s throws CommandInjectionError',
+        (arg) => {
+            /* test */
+            expect(() => shellArgSanitizer.sanitize(arg)).toThrow(/Command injection detected in shell argument:/);
+        }
+    );
+
+    test.each([
+        ['/path/to/sechub-cli'],
+        ['-configfile'],
+        ['/path/to/config.json'],
+        ['-output'],
+        ['/path/to/workspace'],
+        ['-addScmHistory'],
+        ['scan'],
+        ['-jobUUID'],
+        ['-project'],
+        ['--reportformat'],
+        ['json'],
+        ['getReport']
+    ])(
+        'does not throw CommandInjectionError for safe shell argument: %s',
+        (arg) => {
+            /* test */
+            expect(() => shellArgSanitizer.sanitize(arg)).not.toThrow();
+    });
+
+    it('removes whitespaces', function () {
+        /* prepare */
+        const arg = '  /path/to/sechub-cli   ';
+
+        /* execute */
+        const sanitized = shellArgSanitizer.sanitize(arg);
+
+        /* test */
+        expect(sanitized).toEqual('/path/to/sechub-cli');
+    });
+});
diff --git a/github-actions/scan/action.yml b/github-actions/scan/action.yml
@@ -105,7 +105,7 @@ runs:
         git clone --no-checkout --branch=${{ inputs.branch }} https://github.com/mercedes-benz/sechub.git
         cd sechub
         git config core.sparseCheckout true
-             
+        
         echo "github-actions/scan/" >> .git/info/sparse-checkout
         
         git checkout