6 ms·

Please DO NOT mix string interpolation and command execution, especially when a command is processed through the shell. Whatever your language, use a list-based

by wahern 2y ago

Please DO NOT mix string interpolation and command execution, especially when a command is processed through the shell. Whatever your language, use a list-based or array-based execution API that passes arguments straight through to execv(2), execvp(2), etc, bypassing the shell.

echelon 2y ago
SQL injection on steroids.
- rat87 2y ago
  Only if you are getting input from untrusted users
  - remus 2y ago
    imo it's best to just avoid it altogether. Requirements change, and what was once a trusted input can become untrusted input.
    - rat87 2y ago
      Generally you shouldn't be passing random data from the web to shell scripts. Maybe I haven't done the right type of work but having to deal with fidlg bits it's much more likely not passing it to be shell will cause issues (with stuff like executable paths)
  - brookst 2y ago
    Or if your trusted users are fallible and could be tricked into providing unsafe inputs.
paulddraper 2y ago
Tbf this input does not need escaping. But at the very least the shell is unnecessary here.
tasuki 2y ago
Why not?
- bulatb 2y ago
  Any time you send commands and data down a single channel, user input that's intended to be data can be misinterpreted as a command. For example, if your program wants to: run("program --option '{user_input}' > file") to save some input to a file, and the user's input is: '; bad_command # then when run() sends that string to the shell, the shell will run: program --option ''; bad_command #' > file Most languages have something like a safe_exec() that separates the shape of the command from the values of the options, executing "program" with the options and the user_input in the arguments array as data. Skipping the shell step, which would just be building an exec call anyway, removes the opportunity for users to confuse it into doing something else. The list-based API alternative they recommend might look like this: safe_exec(["program", "--option", user_input], stdout="file") and it would always exec "program" with argv[1] == "--option" && argv[2] == user_input. If the user_input happens to be: '; bad_command # ...well, then, the user can enjoy the contents of their file.
  - tasuki 2y ago
    Yes of course. But why would you expect me to run shell commands with random person's input? Also: safe_exec(["rm", user_input]) This isn't safe either! Despite clearly saying "safe_exec"!
    - bulatb 2y ago
      Yeah, "safe_exec" is a useless name without context. But the context was you need to call a program from another program. Many people would call system() or whatever because usually it's obvious and easy, and the pitfalls are less so. Shelling out is not the only option. People are just saying not to use that option. Better ones won't save you if you purposely do something stupid. They will save you if the user wants to trick you into doing something else.
      - 2y ago
gray_-_wolf 2y ago
Another approach is to have powerful enough language that allows you to guard against the shell injection. I wrote a syntax form allowing to do this: (sh "cat " file " >" output) With file being bound to "foo'bar" and output to "x", it is automatically translated into cat 'foo'\''bar' >'x' This gives you the flexibility to use shell (sometimes it just is the most concise way) while being safe against injection. I believe for example in rust you should be able to do the same.
- delusional 2y ago
  How do you know which shell you're escaping for? You could query the system, but now you end up implementing escaping for every shell out there.
  - gray_-_wolf 2y ago
    Good question. I care only about POSIX compatible shells, so the escaping just follows the POSIX rules. In practice that means it works on any actually used system except windows, which is fine with me.
crazygringo 2y ago
For anything involving file paths, user input, etc. -- yes of course. It's not even a question because they would need to be escaped otherwise which nobody wants to do. But for a simple example like this where it's inserting a date which has known properties, it seems fine, and is much more readable.
latexr 2y ago
Why would they even change the language and the commands in the example? It confuses and undermines the point. Just say “use `git switch -c my-new-branch` for interactive usage and `git switch --create my-new-branch` in scripts”. It makes no sense to introduce other unexplained information.
matklad 2y ago
Was waiting for this comment :P The API used handles string interpolation correctly: the string literal is parsed at compile time, and the interpolated arguments are never concatenated or escaped, and end up directly as an element of arv array passed to a child. See https://github.com/tigerbeetle/tigerbeetle/blob/7053ecd2137ace2a14069cc212b0502fe06d3d24/src/shell.zig#L679-L774 https://github.com/tigerbeetle/tigerbeetle/blob/7053ecd2137a...
- chubot 2y ago
  This approach creates an odd mini language, which is incomplete: comptime assert(std.mem.indexOfScalar(u8, cmd, '\'') == null); // Quoting isn't supported yet. comptime assert(std.mem.indexOfScalar(u8, cmd, '"') == null); But you can do correct interpolation with simple shell variables, rather than generating shell code strings: $ today='foo; bar' sh -c 'argv git switch --create "release-$today" origin/main' ['git', 'switch', '--create', 'release-foo; bar', 'origin/main'] So that is a test that we can use a plain shell string, without any shell injection bug. (argv is my command to test quoting: python -c 'import sys; print(sys.argv)' "$@" ) Note that there's no escaping function needed, because we're not generating any shell code. We're generating an argv array for `/bin/sh` instead. --- So by invoking with an env var, you can easily create a correct API that uses plain shell git switch --create "release-$today" rather than git switch --create release-{today} # what language is this? It's not obvious If you don't want to use the env var, you can also use git switch --create "release-$1" And invoke with ['sh', '-c', shell_string, 'unused-arg0', today_string] With this approach, you don't need 1. any kind of shell escaping 2. any analyzing of pseudo-shell strings, which can't contain quotes Because you are not generating any shell code. The shell code is constant.
gorgoiler 2y ago
Miniature, in-line sh scripts are also fine as long as you use the provided parameter substitution. If you’re averse to this: q(“select x where y = ‘“ + v + “‘“) And instead do this: q(“select x where y = %s”, v) Then you should be averse to this: x(“foo --option ‘“ + v + “‘“) And instead do this: x(‘foo --option “$1”’, v) This is particularly useful when it’s expedient to have one thing piping into another. Like it or not the sh DSL for pipes is excellent compared to doing things natively with execve() and pipe(), just as doing group by and count is far more concise in SQL than doing so natively. Most SQL libraries give you something like q. Writing your own x is as simple as calling sh correctly. In Python, for example: def x(script, *args): run([“sh”, “-c”, script, “--“, *args])
- pwdisswordfishz 2y ago
  SyntaxError: invalid character '“' (U+201C)
- kbenson 2y ago
  Neither of those are equivalent to variable binding, which is what most SQL libraries provide, specifically because they don't actually solve the problem since they're still doing string substitution. Putting a double quotes in $1 in your "good" execute example will allow you break out of what's expected and then you're Bobby Tables. Your python example at the bottom is correct, in that each separate element is more correct in that it allows each arg to be passed as an element, so there's no option to break out through quoting characters. SQL binds are like that in most ljbraries, even if they don't look like it. The parser knows a single item below there so if it passes it along as such. You cannot escape it in the same way.
  - gorgoiler 2y ago
    I don’t really follow. My “good” example and the code at the bottom are the same. sh is smarter than just doing string interpolation and ”$1” is passed on as a single argument, no matter what: > run(["sh", "-c", 'echo "$1"', "--", 'a"']) a” Whereas if it were simple string interpolation, you’d see this: > run(["sh", "-c", 'echo "a""') --: 1: Syntax error: Unterminated quoted string It’s the same special casing that gets "$@" right.

by wahern 2y ago

echelon 2y ago

SQL injection on steroids.

rat87 2y ago
Only if you are getting input from untrusted users
- remus 2y ago
  imo it's best to just avoid it altogether. Requirements change, and what was once a trusted input can become untrusted input.
  - rat87 2y ago
    Generally you shouldn't be passing random data from the web to shell scripts. Maybe I haven't done the right type of work but having to deal with fidlg bits it's much more likely not passing it to be shell will cause issues (with stuff like executable paths)
- brookst 2y ago
  Or if your trusted users are fallible and could be tricked into providing unsafe inputs.

paulddraper 2y ago

Tbf this input does not need escaping. But at the very least the shell is unnecessary here.

tasuki 2y ago

Why not?

bulatb 2y ago
Any time you send commands and data down a single channel, user input that's intended to be data can be misinterpreted as a command. For example, if your program wants to: run("program --option '{user_input}' > file") to save some input to a file, and the user's input is: '; bad_command # then when run() sends that string to the shell, the shell will run: program --option ''; bad_command #' > file Most languages have something like a safe_exec() that separates the shape of the command from the values of the options, executing "program" with the options and the user_input in the arguments array as data. Skipping the shell step, which would just be building an exec call anyway, removes the opportunity for users to confuse it into doing something else. The list-based API alternative they recommend might look like this: safe_exec(["program", "--option", user_input], stdout="file") and it would always exec "program" with argv[1] == "--option" && argv[2] == user_input. If the user_input happens to be: '; bad_command # ...well, then, the user can enjoy the contents of their file.
- tasuki 2y ago
  Yes of course. But why would you expect me to run shell commands with random person's input? Also: safe_exec(["rm", user_input]) This isn't safe either! Despite clearly saying "safe_exec"!
  - bulatb 2y ago
    Yeah, "safe_exec" is a useless name without context. But the context was you need to call a program from another program. Many people would call system() or whatever because usually it's obvious and easy, and the pitfalls are less so. Shelling out is not the only option. People are just saying not to use that option. Better ones won't save you if you purposely do something stupid. They will save you if the user wants to trick you into doing something else.
    - 2y ago

gray_-_wolf 2y ago

Another approach is to have powerful enough language that allows you to guard against the shell injection. I wrote a syntax form allowing to do this: (sh "cat " file " >" output) With file being bound to "foo'bar" and output to "x", it is automatically translated into cat 'foo'\''bar' >'x' This gives you the flexibility to use shell (sometimes it just is the most concise way) while being safe against injection. I believe for example in rust you should be able to do the same.

delusional 2y ago
How do you know which shell you're escaping for? You could query the system, but now you end up implementing escaping for every shell out there.
- gray_-_wolf 2y ago
  Good question. I care only about POSIX compatible shells, so the escaping just follows the POSIX rules. In practice that means it works on any actually used system except windows, which is fine with me.

crazygringo 2y ago

For anything involving file paths, user input, etc. -- yes of course. It's not even a question because they would need to be escaped otherwise which nobody wants to do. But for a simple example like this where it's inserting a date which has known properties, it seems fine, and is much more readable.

latexr 2y ago

Why would they even change the language and the commands in the example? It confuses and undermines the point. Just say “use `git switch -c my-new-branch` for interactive usage and `git switch --create my-new-branch` in scripts”. It makes no sense to introduce other unexplained information.

matklad 2y ago

Was waiting for this comment :P The API used handles string interpolation correctly: the string literal is parsed at compile time, and the interpolated arguments are never concatenated or escaped, and end up directly as an element of arv array passed to a child. See https://github.com/tigerbeetle/tigerbeetle/blob/7053ecd2137ace2a14069cc212b0502fe06d3d24/src/shell.zig#L679-L774 https://github.com/tigerbeetle/tigerbeetle/blob/7053ecd2137a...

chubot 2y ago
This approach creates an odd mini language, which is incomplete: comptime assert(std.mem.indexOfScalar(u8, cmd, '\'') == null); // Quoting isn't supported yet. comptime assert(std.mem.indexOfScalar(u8, cmd, '"') == null); But you can do correct interpolation with simple shell variables, rather than generating shell code strings: $ today='foo; bar' sh -c 'argv git switch --create "release-$today" origin/main' ['git', 'switch', '--create', 'release-foo; bar', 'origin/main'] So that is a test that we can use a plain shell string, without any shell injection bug. (argv is my command to test quoting: python -c 'import sys; print(sys.argv)' "$@" ) Note that there's no escaping function needed, because we're not generating any shell code. We're generating an argv array for `/bin/sh` instead. --- So by invoking with an env var, you can easily create a correct API that uses plain shell git switch --create "release-$today" rather than git switch --create release-{today} # what language is this? It's not obvious If you don't want to use the env var, you can also use git switch --create "release-$1" And invoke with ['sh', '-c', shell_string, 'unused-arg0', today_string] With this approach, you don't need 1. any kind of shell escaping 2. any analyzing of pseudo-shell strings, which can't contain quotes Because you are not generating any shell code. The shell code is constant.

gorgoiler 2y ago

Miniature, in-line sh scripts are also fine as long as you use the provided parameter substitution. If you’re averse to this: q(“select x where y = ‘“ + v + “‘“) And instead do this: q(“select x where y = %s”, v) Then you should be averse to this: x(“foo --option ‘“ + v + “‘“) And instead do this: x(‘foo --option “$1”’, v) This is particularly useful when it’s expedient to have one thing piping into another. Like it or not the sh DSL for pipes is excellent compared to doing things natively with execve() and pipe(), just as doing group by and count is far more concise in SQL than doing so natively. Most SQL libraries give you something like q. Writing your own x is as simple as calling sh correctly. In Python, for example: def x(script, *args): run([“sh”, “-c”, script, “--“, *args])

pwdisswordfishz 2y ago
SyntaxError: invalid character '“' (U+201C)
kbenson 2y ago
Neither of those are equivalent to variable binding, which is what most SQL libraries provide, specifically because they don't actually solve the problem since they're still doing string substitution. Putting a double quotes in $1 in your "good" execute example will allow you break out of what's expected and then you're Bobby Tables. Your python example at the bottom is correct, in that each separate element is more correct in that it allows each arg to be passed as an element, so there's no option to break out through quoting characters. SQL binds are like that in most ljbraries, even if they don't look like it. The parser knows a single item below there so if it passes it along as such. You cannot escape it in the same way.
- gorgoiler 2y ago
  I don’t really follow. My “good” example and the code at the bottom are the same. sh is smarter than just doing string interpolation and ”$1” is passed on as a single argument, no matter what: > run(["sh", "-c", 'echo "$1"', "--", 'a"']) a” Whereas if it were simple string interpolation, you’d see this: > run(["sh", "-c", 'echo "a""') --: 1: Syntax error: Unterminated quoted string It’s the same special casing that gets "$@" right.