Warn about security pitfalls of `find -exec` in favor of -execdir

[rpdelaney]

For new checks and feature suggestions

• [x] shellcheck.net (i.e. the latest commit) currently gives no useful warnings about this
• [x] I searched through https://github.com/koalaman/shellcheck/issues and didn't find anything related

Here's a snippet or screenshot that shows the problem:

#!/usr/bin/env sh
find /tmp -path /tmp/umsp/passwd -exec /bin/rm \;

Here's what shellcheck currently says:

No issues detected!

Here's what I wanted or expected to see:

Line 2:
find /tmp -path /tmp/umsp/passwd -exec /bin/rm
                                 ^-- SCXXXX: -exec is insecure. Use -execdir.

Rationale

shellcheck already warns about problems with -exec and filenames in SC2156.

But there is another problem with -exec. The manpage for GNU find has cryptic warnings:

-exec command ; ...There are unavoidable security problems surrounding use of the -exec action; you should use the -execdir option instead.

-execdir command ; -execdir command {} +

...This a much more secure method for invoking commands, as it avoids race conditions during resolution of the paths to the matched files...

Later:

There are security problems inherent in the behaviour that the POSIX standard specifies for find, which therefore cannot be fixed. For example, the -exec action is inherently insecure, and -execdir should be used instead.

The GNU Finding Files manual has a full explanation.

Arguably, -execdir is not serviceable in all contexts, and shellcheck cannot be expected to know when the user's threat model includes these kinds of timing attacks by hostile users on the system. But we often warn about such problems anyway, such as in SC2156, and allow the user to disable the warning if it is not relevant.

[lordfolken]

There is another reason to advise against -exec: Find aborts on first failure of an -exec command, but it itself exits with return code 0. Small example:

touch 1 2 3
find
.
./1
./2
./3
find ./ -exec cat "{}" \;
cat: ./: Is a directory
echo $?
0

[mcandre]

find ... -exec ... "{}" + is suitable when the subprocessing command is able to handle an arbitrary number of arguments. The plus sign enables batching, but more importantly, it fixes find's exit code to more accurately reflect whether all the subprocesses succeeded. This keeps larger scripts and CI/CD pipelines honest about any errors, instead of obfuscating them like \; does.

However, the subprocessing command may not necessarily handle arbitrary length ARGV. Some CLI applications are poorly designed.

In that case, then yet another safe and secure option is find ... -print0 | xargs -0 ..., however POSIX absurdly refuses to admit these flags into the standard. Fringe implementations of findutils are likely to neglect to implement these important flags.

For loops are right out for processing lists of file paths and other data sets that may exhibit spaces within lines. In POSIX sh at least, due to how shell tends to mismanage spaces during loop iteration.

Yet another solution is this arcane IFS, read snippet posted on Stack Overflow:

https://stackoverflow.com/questions/36373424/read-filenames-with-embedded-whitespace-into-an-array-in-a-shell-script/36375034#36375034

It's not for the faint of heart. It's not easy to write from memory. It's not easy to integrate into larger scripting systems, especially with the literal newline there. But these three represent the safest way to process results with spaces, in a shel implementation independent way. One benefit of the crazy IFS read snippet, is that the user has the option to further post-process results, such as filtering through grep -v..., before forwarding the final list to downstream processing.