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

fix(security): bypass allowed cmds #41

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

asdfzxcvbn
Copy link

using control operators, you can use disallowed commands. see screenshot below for example.

this fix simply creates a list of all known operators that can be used and checks the input for them.

image

@mhkarimi1383
Copy link
Contributor

Hi,
Using a temp docker container (for each console request) witch has docker client cli that for running command is safer than doing this

@asdfzxcvbn
Copy link
Author

Using a temp docker container (for each console request) witch has docker client cli that for running command is safer than doing this

yeah, maybe safer, but definitely not as performant. i doubt you can do any real damage with only 4 commands, anyway. blocking control operators is probably the best solution.

@louislam louislam added the question Further information is requested label Nov 16, 2023
Comment on lines +228 to +229
} else if (knownOperators.some(operator => input.includes(operator))) {
throw new Error("Control operators are not allowed.");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like if any string contain this characters, it also throws the error.

But actually, since it is a bit powerful, I want to completely turn off this feature by default and use an env var to turn on/off this feature.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like if any string contain this characters, it also throws the error.

yeah, thats what i had to do since spaces aren't required. for example, ls ;echo hi also works, so just checking every item in cmdParts wouldn't work.

But actually, since it is a bit powerful, I want to completely turn off this feature by default and use an env var to turn on/off this feature.

yep, that'd be even better!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants