Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge develop into master for release #3529

Merged
merged 14 commits into from
Oct 18, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
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
hamidonos authored Oct 17, 2024
commit cdb5c2548c6fab6c003e84df286cb1f9887101b7
1 change: 0 additions & 1 deletion github-actions/scan/__test__/configuration-builder.test.ts
Original file line number Diff line number Diff line change
@@ -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

2 changes: 1 addition & 1 deletion github-actions/scan/__test__/integrationtest/01-start.sh
Original file line number Diff line number Diff line change
@@ -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
3 changes: 2 additions & 1 deletion github-actions/scan/__test__/integrationtest/start_pds.sh
Original file line number Diff line number Diff line change
@@ -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}" &
Original file line number Diff line number Diff line change
@@ -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}" &
132 changes: 106 additions & 26 deletions github-actions/scan/__test__/sechub-cli.test.ts
Original file line number Diff line number Diff line change
@@ -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');
});

});
84 changes: 84 additions & 0 deletions github-actions/scan/__test__/shell-arg-sanitizer.test.ts
Original file line number Diff line number Diff line change
@@ -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');
});
});
2 changes: 1 addition & 1 deletion github-actions/scan/action.yml
Original file line number Diff line number Diff line change
@@ -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
Loading