Skip to content
Permalink
Browse files Browse the repository at this point in the history
Fix shell injection bug
Fix a shell injection bug where an attacker controlling the "tag" value
in the code (i.e. the value of `process.env.GITHUB_REF` or of the
`with: tag` input value) can run arbitrary (*) shell commands.

Also, update the Action's branding, changing the Action icon from
"git-commit" to "tag".

(*): provided it is within the limits of the GitHub Actions environment
  • Loading branch information
ericcornelissen committed Oct 24, 2020
1 parent 7455eac commit 9f30756
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 10 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Expand Up @@ -9,6 +9,11 @@ Versioning].

- _No changes yet_

## [1.0.1] - 2020-10-24

- Update Action icon.
- Fix shell injection vulnerability.

## [1.0.0] - 2020-10-22

- Run the Action to get the git tag annotation of the current tag.
Expand Down
2 changes: 1 addition & 1 deletion action.yml
Expand Up @@ -16,5 +16,5 @@ runs:
main: 'lib/index.js'

branding:
icon: 'git-commit'
icon: 'tag'
color: 'gray-dark'
22 changes: 15 additions & 7 deletions src/main.js
@@ -1,20 +1,28 @@
const core = require('@actions/core');
const { exec } = require('child_process');

// Based on https://stackoverflow.com/a/22827128
function escapeShellArg(arg) {
return arg.replace(/'/g, `'\\''`);
}

function main() {
try {
let tag = process.env.GITHUB_REF;
if (core.getInput('tag')) {
tag = `refs/tags/${core.getInput('tag')}`;
}

exec(`git for-each-ref --format='%(contents)' ${tag}`, (err, stdout) => {
if (err) {
core.setFailed(err);
} else {
core.setOutput('git-tag-annotation', stdout);
}
});
exec(
`git for-each-ref --format='%(contents)' '${escapeShellArg(tag)}'`,
(err, stdout) => {
if (err) {
core.setFailed(err);
} else {
core.setOutput('git-tag-annotation', stdout);
}
},
);
} catch (error) {
core.setFailed(error.message);
}
Expand Down
15 changes: 13 additions & 2 deletions test/main.test.js
Expand Up @@ -23,7 +23,7 @@ it.each([
main();

expect(child_process.exec).toHaveBeenCalledWith(
`git for-each-ref --format='%(contents)' refs/tags/${tag}`,
`git for-each-ref --format='%(contents)' 'refs/tags/${tag}'`,
expect.any(Function),
);
});
Expand All @@ -46,7 +46,7 @@ it.each([

expect(core.getInput).toHaveBeenCalledTimes(2);
expect(child_process.exec).toHaveBeenCalledWith(
`git for-each-ref --format='%(contents)' refs/tags/${tag}`,
`git for-each-ref --format='%(contents)' 'refs/tags/${tag}'`,
expect.any(Function),
);
});
Expand Down Expand Up @@ -89,3 +89,14 @@ it('sets an error if exec fails', () => {
expect(core.setOutput).not.toHaveBeenCalled();
expect(core.setFailed).toHaveBeenCalledTimes(1);
});

it('escapes malicious values from the input', () => {
core.getInput.mockReturnValue(`'; $(cat /etc/shadow)`);

main();

expect(child_process.exec).toHaveBeenCalledWith(
"git for-each-ref --format='%(contents)' 'refs/tags/'\\''; $(cat /etc/shadow)'",
expect.any(Function),
);
});

0 comments on commit 9f30756

Please sign in to comment.