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

Quote input strings before constructing a command line #1098

Merged
merged 3 commits into from
Oct 30, 2019

Conversation

qiluo-msft
Copy link
Contributor

@qiluo-msft qiluo-msft commented Oct 17, 2019

Quote input strings from function arguments before constructing a command line, so prevent of the bash script input injection.

man dash shows below section, which indicates the double quotes and escaping is enough for general use.

Double Quotes
 Enclosing characters within double quotes preserves the literal meaning of all characters except dollarsign ($), backquote (`), and backslash (\).  The backslash inside double quotes is historically weird, and serves to quote only the following characters:
       $ ` " \ <newline>.
 Otherwise it remains literal.

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>
@qiluo-msft qiluo-msft marked this pull request as ready for review October 18, 2019 04:46
@pavel-shirshov
Copy link
Contributor

pavel-shirshov commented Oct 18, 2019

We need some description for this PR #Resolved

@pavel-shirshov
Copy link
Contributor

pavel-shirshov commented Oct 19, 2019

sonicbld@5de366018e18:~$ cat /sys/class/net/"$(echo hi)"/operstate    
cat: /sys/class/net/hi/operstate: No such file or directory

I still can execute commands even with quotes #ByDesign

Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>
because system() and popen() will use `sh` internally and `sh` is
symlink of `dash` on Debian
static inline std::string shellquote(const std::string& str)
{
static const std::regex re("([$`\"\\\n])");
return "\"" + std::regex_replace(str, re, "\\$1") + "\"";
Copy link
Contributor

Choose a reason for hiding this comment

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

How can we be sure this regex is enough for everything?
Should we better create functions to check the input parameters and run them?
Our input parameters types are easy:

  1. ip address
  2. network mask
  3. network device
  4. number
  5. string enum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update the PR comment at the top with answer.
I agree case-by-case protection is also possible, this one is for general purpose.

Copy link
Contributor

@stcheng stcheng left a comment

Choose a reason for hiding this comment

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

thanks

@qiluo-msft qiluo-msft merged commit d4ccdc3 into sonic-net:master Oct 30, 2019
@qiluo-msft qiluo-msft deleted the qiluo/quotedcmd branch October 30, 2019 01:34
@banagiri
Copy link
Contributor

@qiluo-msft in the api removeHostVlanMember, is the cmds having cmds as input instead of inner? or is this expected ?

cmds << BASH_CMD " -c " << shellquote(cmds.str());
instead of
cmds << BASH_CMD " -c " << shellquote(inner.str());

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

Successfully merging this pull request may close these issues.

6 participants