12
\$\begingroup\$

After growing tired of all the ways in which a file loop can be broken (find -print | while read) or unreadable (find -exec with complex commands), I think I've managed to build a find template which can handle any and all files which could possibly exist on a Linux system (not so famous last words). Can you find a way to break it, by changing either the test_ variables or the environment? For example, is it possible to mess with file descriptor 9 outside the script so that it won't work?

The only requirement is "sanity." In other words, test_file_name and test_dir_path cannot contain \0 or /, test_file_path cannot contain \0 (or be more than 1 level deep, since mkdir for the sake of the test is run without -p), and /bin/bash must be a stable version of Bash 4.

#!/bin/bash
# Filenames can contain *any* character except only null (\0) and slash (/);
# here's some general rules to handle them:
#
# $'...' can be used to create human readable strings with escape sequences.
#
# ' -- ' in commands is necessary to separate arguments from filenames, since
# filenames can start with '--', and would therefore be handled as parameters.
# To handle parameters properly (like GNU tools) use `getopt`.
#
# `find` doesn't support this syntax, so we use `readlink` to get an absolute
# path which by definition starts with slash.
#
# The "$()" construct strips trailing newlines, so we have to add a different
# character and then strip it outside the "$()" construct.
#
# `IFS=` is necessary to avoid that any characters in IFS are stripped from
# the start and end of $path.
#
# '-r' avoids interpreting backslash in filenames specially.
#
# '-d '' splits filenames by the null character.
#
# '-print0' separates find output by null characters.
#
# Variables inside '$()' have to be quoted just like outside this construct.
#
# Use process substitution with "<(" instead of pipes to avoid broken pipes.
#
# Use file descriptor 9 for data storage instead of standard input to avoid
# greedy commands like `cat` eating all of it.

set -o errexit
set -o nounset
set -o noclobber

test_file_name=$'--$`\! *@ \a\b\e\E\f\r\t\v\\\"\' \n'
test_dir_path="$test_file_name"
test_file_path="${test_dir_path}/${test_file_name}"

mkdir -- "$test_dir_path"
touch -- "$test_file_path"

absolute_dir_path_x="$(readlink -fn -- "$test_dir_path"; echo x)"
absolute_dir_path="${absolute_dir_path_x%x}"

exec 9< <( find "$absolute_dir_path" -type f -print0 )
while IFS= read -r -d '' -u 9
do
    file_path="$(readlink -fn -- "$REPLY"; echo x)"
    file_path="${file_path%x}"
    echo "START${file_path}END"
done

rm -- "$test_file_path"
rmdir -- "$test_dir_path"
\$\endgroup\$
  • \$\begingroup\$ Either nobody here knows bash, or nobody can break it. ;) \$\endgroup\$ – Steven Jeuris Mar 23 '11 at 10:59
  • 2
    \$\begingroup\$ If you're talking about gnu find, you hardly ever need find | while read, or for or xargs and the like. Read the manpage for find, especially the part about -exec, -execdir, -ok and -okdir. Find already iterates over the the result, so you don't need an iterator to catch and rethrow the results to some other command or program. \$\endgroup\$ – user unknown Mar 24 '11 at 5:48
  • 2
    \$\begingroup\$ I haven't found a bug, so have my sole nitpick so far: printf "%q\n" "$file_path" is nicer than echo (and there's an unbalanced quote in your comments). \$\endgroup\$ – Tobu Mar 25 '11 at 20:55
  • \$\begingroup\$ @Tobu: I disagree with the printf statement (do you have a reference or more elaborate reasoning?), but thanks for the quote fix. \$\endgroup\$ – l0b0 Mar 30 '11 at 11:02
  • \$\begingroup\$ It's been 11 days, and a week of bounty, with no submissions showing functional errors in the code. As user unknown pointed out, there are more elegant solutions in case you only want to run a single command on each file within a separate subshell. But at this point I guess it's safe to say that this is a really safe way to loop over any and all files without tripping over names, executing arbitrary strings, prematurely terminating, or ending up with an undocumentable picket fence. Thanks to everyone who tried! \$\endgroup\$ – l0b0 Mar 30 '11 at 11:17
13
+50
\$\begingroup\$

This code is vulnerable to TOCTOU. There is a tiny gap between the time that "plain files" are read from the process substitution (find -type f ...) and the time that readlink(1) is called on those filenames.

An attacker could create a program that waits for your program to run and then quickly deletes one of those files found by find(1) and replaces it with a symlink to somewhere else. readlink(1) will then dutifully return the target of that symlink and this is the path that will be output. The target could be outside $absolute_dir_path and a file of any type (directory, device node, ...).

Eg:

#!/bin/bash

set -o errexit
set -o nounset
set -o noclobber

# setup directory containing two plain files
# hardcode absolute_dir_path to /tmp/dir for simplicity
mkdir -p /tmp/dir
rm -f /tmp/dir/a /tmp/dir/b
touch /tmp/dir/a /tmp/dir/b

# emulate OP's find loop, but with inserted actions
# performed by an attacker (in real attack these would
# happen in an external program).
exec 9< <( find /tmp/dir -type f -print0 )
while IFS= read -r -d '' -u 9
do
        file_path_x="$(readlink -fn -- "$REPLY"; echo x)"
        file_path="${file_path_x%x}"
        ls -l "${file_path}"
        # attacker jumps in here and does:
        rm /tmp/dir/b
        ln -s /etc/passwd /tmp/dir/b
done

Output:

-rw-r--r-- 1 martin martin 0 2011-03-31 10:56 /tmp/dir/a
-rw-r--r-- 1 root root 2119 2011-03-28 11:35 /etc/passwd

The risk can be mitigated by only seeking files in directories beneath which untrusted users do not have write access. Directories such as /tmp and /var/tmp are problematic though and this is hard to solve. See the source of (eg) tmpreaper for some ideas.

\$\endgroup\$
  • \$\begingroup\$ Excellent observation. I guess one fix would be to avoid the readlink altogether (which would work since filenames would start with /tmp/dir in any case), and make sure that none of the commands within the loop traverse symbolic links. \$\endgroup\$ – l0b0 Apr 5 '11 at 10:55
  • \$\begingroup\$ Thanks! That's a definite improvement, but an attacker could substitute a directory or named pipe or... (or even just delete the file). That's less obviously exploitable but could still cause problems with whatever processes the supposed list of plan files (aim to "fail fast, fail hard", don't rely on pre-emptive tests). \$\endgroup\$ – Martin Carpenter Apr 7 '11 at 15:23
2
\$\begingroup\$
find . -type f -execdir echo START{}END ";"
START./--$`\! *@

        \"' 
/--$`\! *@

        \"' 
END

In which cases to you fail with find?

find . -type f  -execdir md5sum {} ";"
\d41d8cd98f00b204e9800998ecf8427e  ./--$`\\! *@

        \\"' \n/--$`\\! *@

        \\"' \n

(changed from -exec to -execdir after useful hint).

\$\endgroup\$
  • \$\begingroup\$ What do you mean? \$\endgroup\$ – l0b0 Mar 24 '11 at 7:57
  • \$\begingroup\$ I mean, your solution solves no problem. \$\endgroup\$ – user unknown Mar 24 '11 at 8:14
  • \$\begingroup\$ How to loop over files is asked several times per day in different SO sites, and half-assed solutions are suggested every time. This was a solution which worked for heavily unit-tested code, so I thought it might be generally safe. -exec is nice if you want to run a single command, but my solution was also supposed to be readable if you have more complex commands to run for each file. \$\endgroup\$ – l0b0 Mar 24 '11 at 8:42
  • \$\begingroup\$ find . -type f -exec complexScript.sh {} ";" \$\endgroup\$ – user unknown Mar 24 '11 at 8:45
  • 2
    \$\begingroup\$ complexScript.sh is run in a separate subshell - You'd need to add parameter passing + parsing if you want any context from the source script. Also, for some reason nobody suggests using -execdir even though the man page says "There are unavoidable security problems surrounding use of the -exec action; you should use the -execdir option instead." \$\endgroup\$ – l0b0 Mar 24 '11 at 8:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.