Fix the unit test and open a giant hole everywhere

Re:Fix the unit test and open a giant hole everywhere by theptip on Saturday December 25, 2021 @02:38AM (#29681274)

I agree. Starting a subprocess is fine. Using an existing tool (mkdir) was probably a better approach than reimplementing it.

The error: you should never pass unescaped/untrusted input into a subshell.

In Python, it’s the difference between the safe:

    def make_dir(path):
      subprocess.run([“mkdir”, “-p”, path])

Instead of:

    def make_dir_unsafe(path):
      subprocess.run(“mkdir -p “ + path, shell=True)

The latter is easier to type because you don’t need to use the annoying list syntax, but it’s unsafe. If you are writing a library function a subshell is not a sound approach.

[ Parent | on HN ]

Re:Fix the unit test and open a giant hole everywhere by blorgle on Saturday December 25, 2021 @04:09AM (#29681681)

You shouldn't pass unescaped/untrusted input into a subshell but your suggestion doesn't solve for every case. It protects against ";" but not path traversal with "../../../../../foo/bar".

[ Parent | on HN ]

Re:Fix the unit test and open a giant hole everywhere by yjftsjthsd-h on Saturday December 25, 2021 @11:15AM (#29684419)

You'd have to cover that if you wrote your own library function, though; it's attack surface, but not uniquely so.

[ Parent | on HN ]

Re:Fix the unit test and open a giant hole everywhere by Noe2097 on Saturday December 25, 2021 @05:09AM (#29681955)

Hmmm. In Python, I would leverage the dedicated utility of the standard library:

  os.makedirs(path)

[ Parent | on HN ]

Re:Fix the unit test and open a giant hole everywhere by theptip on Saturday December 25, 2021 @03:10PM (#29686524)

Yes, agreed. I was using Python code to more explicitly illustrate the subprocess vs shell distinction in the general case, not to provide a solution to the specific problem of making a directory.

[ Parent | on HN ]

Re:Fix the unit test and open a giant hole everywhere by im3w1l on Saturday December 25, 2021 @04:41AM (#29681838)

    subprocess.run([“mkdir”, “-p”, path])

This does not work for all values of path. And I'm not just referring to blorgle's complaint. I'm being deliberately vague to make you pause for a second and think.

[ Parent | on HN ]

Re:Fix the unit test and open a giant hole everywhere by ynik on Saturday December 25, 2021 @06:48AM (#29682494)

You seem to be getting downvoted for being too vague, so I will post the solution:

The snippet breaks for paths starting with a minus, as mkdir will interpret those as another option.

The solution is:

    subprocess.run(["mkdir", "-p", "--", path])

[ Parent | on HN ]

Re:Fix the unit test and open a giant hole everywhere by Kinrany on Saturday December 25, 2021 @05:40AM (#29682096)

Does `path = "$(rm -rf /)"` work in this context?

[ Parent | on HN ]

Re:Fix the unit test and open a giant hole everywhere by bstpierre on Saturday December 25, 2021 @07:24AM (#29682717)

No, because this isn’t being run through the shell, it’s directly exec’ing the program. So you’re just going to get a dir named “$(rm -rf /)”.

[ Parent | on HN ]

Re:Fix the unit test and open a giant hole everywhere by im3w1l on Saturday December 25, 2021 @07:33AM (#29682765)

It's funny how easy it is to miss characters with special meaning. In this case it's the slash. You would get a directory named "$(rm -rf " with a subdirectory named ")"

[ Parent | on HN ]

Re:Fix the unit test and open a giant hole everywhere by pabs3 on Saturday December 25, 2021 @03:23AM (#29681470)

I like to do this to avoid list syntax:

    def make_dir(path):
      subprocess.run('mkdir -p'.split() + [path])

[ Parent | on HN ]

Re:Fix the unit test and open a giant hole everywhere by loloquwowndueo on Saturday December 25, 2021 @07:14AM (#29682659)

So you like typing more and making your program harder to read to avoid using two apostrophes and two commas.

[ Parent | on HN ]

Re:Fix the unit test and open a giant hole everywhere by cowsandmilk on Saturday December 25, 2021 @08:37AM (#29683255)

If you have a command with lots of arguments, parent’s approach makes a lot of sense. 20 arguments that you can copy out of the shell into your program. And when you want to make changes and test via the shell, your copy and paste is also easy.

I’ve seen bugs where some forgot to add quotes and commas to split some arguments and the program did the wrong thing. Parent’s approach avoids that. However, things like spaces in quoted strings are not correctly handled.

[ Parent | on HN ]

Re:Fix the unit test and open a giant hole everywhere by loloquwowndueo on Saturday December 25, 2021 @11:15AM (#29684416)

Naively splitting commands using split() will bite you in the ass eventually. Use shlex.split as the other comment says, or just stop being lazy and craft the argument list carefully, once, and be done with it. Shortcuts here aren’t worth the risk.

[ Parent | on HN ]

Re:Fix the unit test and open a giant hole everywhere by div72 on Saturday December 25, 2021 @09:46AM (#29683712)

I am not exactly sure if parent's code execute the way you think it does. path is provided as an argument to mkdir -p, even if you set path to be a string with 20 arguments copied from your shell, it'll get passed as one.

I assume you meant something like subprocess.run(['mkdir', '-p', '--', *path.split()]), which in that case using shlex.split will fix your problems with quoted strings.

[ Parent | on HN ]