diff --git a/nvm.sh b/nvm.sh index c07cc171..e59cf3f2 100755 --- a/nvm.sh +++ b/nvm.sh @@ -115,47 +115,62 @@ nvm_get_latest() { nvm_echo "${NVM_LATEST_URL##*/}" } +# Every argument is passed through as a literal argv element so that untrusted, +# mirror-supplied version strings in the URLs are never re-parsed by the shell +# (which would allow command substitution / OS command injection). nvm_download() { - if nvm_has "curl"; then - local CURL_COMPRESSED_FLAG="" - local CURL_HEADER_FLAG="" - local sanitized_header - - if [ -n "${NVM_AUTH_HEADER:-}" ]; then - sanitized_header=$(nvm_sanitize_auth_header "${NVM_AUTH_HEADER}") - CURL_HEADER_FLAG="--header \"Authorization: ${sanitized_header}\"" - fi - - if nvm_curl_use_compression; then - CURL_COMPRESSED_FLAG="--compressed" - fi - local NVM_DOWNLOAD_ARGS - NVM_DOWNLOAD_ARGS='' - for arg in "$@"; do - NVM_DOWNLOAD_ARGS="${NVM_DOWNLOAD_ARGS} \"$arg\"" - done - eval "curl -q --fail ${CURL_COMPRESSED_FLAG:-} ${CURL_HEADER_FLAG:-} ${NVM_DOWNLOAD_ARGS}" - elif nvm_has "wget"; then - # Emulate curl with wget - ARGS=$(nvm_echo "$@" | command sed " - s/--progress-bar /--progress=bar / - s/--compressed // - s/--fail // - s/-L // - s/-I /--server-response / - s/-s /-q / - s/-sS /-nv / - s/-o /-O / - s/-C - /-c / - ") - - if [ -n "${NVM_AUTH_HEADER:-}" ]; then - sanitized_header=$(nvm_sanitize_auth_header "${NVM_AUTH_HEADER}") - ARGS="${ARGS} --header \"Authorization: ${sanitized_header}\"" - fi - # shellcheck disable=SC2086 - eval wget $ARGS + local sanitized_header + sanitized_header='' + if [ -n "${NVM_AUTH_HEADER:-}" ]; then + sanitized_header="$(nvm_sanitize_auth_header "${NVM_AUTH_HEADER}")" fi + + local NVM_DOWNLOADER + NVM_DOWNLOADER='' + if nvm_has "curl"; then + NVM_DOWNLOADER='curl' + set -- -q --fail "$@" + if nvm_curl_use_compression; then + set -- --compressed "$@" + fi + elif nvm_has "wget"; then + NVM_DOWNLOADER='wget' + # Emulate curl with wget + local NVM_DOWNLOAD_WGET_COUNT + NVM_DOWNLOAD_WGET_COUNT=$# + local NVM_DOWNLOAD_WGET_SKIP + NVM_DOWNLOAD_WGET_SKIP=0 + local NVM_DOWNLOAD_WGET_ARG + for NVM_DOWNLOAD_WGET_ARG in "$@"; do + if [ "${NVM_DOWNLOAD_WGET_SKIP}" = '1' ]; then + NVM_DOWNLOAD_WGET_SKIP=0 + continue + fi + case "${NVM_DOWNLOAD_WGET_ARG}" in + '--progress-bar') set -- "$@" '--progress=bar' ;; + '--compressed') : ;; + '--fail') : ;; + '-L') : ;; + '-I') set -- "$@" '--server-response' ;; + '-s') set -- "$@" '-q' ;; + '-sS') set -- "$@" '-nv' ;; + '-o') set -- "$@" '-O' ;; + '-C') NVM_DOWNLOAD_WGET_SKIP=1; set -- "$@" '-c' ;; + *) set -- "$@" "${NVM_DOWNLOAD_WGET_ARG}" ;; + esac + done + shift "${NVM_DOWNLOAD_WGET_COUNT}" + fi + + if [ -z "${NVM_DOWNLOADER}" ]; then + return 0 + fi + + if [ -n "${NVM_AUTH_HEADER:-}" ]; then + set -- "$@" --header "Authorization: ${sanitized_header}" + fi + + "${NVM_DOWNLOADER}" "$@" } nvm_sanitize_auth_header() { diff --git a/test/fast/Unit tests/nvm_download no eval injection b/test/fast/Unit tests/nvm_download no eval injection new file mode 100755 index 00000000..8e6b3d49 --- /dev/null +++ b/test/fast/Unit tests/nvm_download no eval injection @@ -0,0 +1,86 @@ +#!/bin/sh + +OLDPATH="$PATH" +WORK="$PWD/nvm_download-noeval-work.$$" +TEST_BIN="$WORK/bin" +ARGV_LOG="$WORK/argv.log" +PROOF="$WORK/nvm_injection_proof" + +cleanup() { + unset -f die cleanup + rm -rf "$WORK" + export PATH="$OLDPATH" +} +die () { echo "$@" ; cleanup ; exit 1; } + +\. ../../../nvm.sh + +OLDPATH="$PATH" + +mkdir -p "$TEST_BIN" + +# fake curl/wget: record each received argument verbatim, then succeed +{ + echo '#!/bin/sh' + echo ': > "$ARGV_LOG"' + echo 'for a in "$@"; do printf "%s\n" "$a" >> "$ARGV_LOG"; done' + echo 'exit 0' +} > "$TEST_BIN/curl" +chmod +x "$TEST_BIN/curl" +cp "$TEST_BIN/curl" "$TEST_BIN/wget" + +export ARGV_LOG +export PATH="$TEST_BIN:$OLDPATH" + +# given a url containing command-substitution syntax (mirror-supplied) +INJECT_URL="http://example.test/v1\$(touch ${PROOF})/x.tar.gz" + +# when nvm_download is invoked (curl path) +rm -f "$PROOF" +nvm_download "$INJECT_URL" -o - || die 'nvm_download (curl) returned nonzero on injection url' +# then the substitution must not have executed +[ ! -e "$PROOF" ] || die "command injection fired via curl path: proof file was created" +# and curl must have received the url as one literal argument +grep -Fxq "$INJECT_URL" "$ARGV_LOG" || die "curl did not receive the url as a single literal argument; got: $(cat "$ARGV_LOG")" + +# given a normal download (curl path) +URL="https://nodejs.org/dist/index.tab" +FILE="$WORK/target" +# when invoked with -L -s URL -o FILE +nvm_download -L -s "$URL" -o "$FILE" || die 'nvm_download (curl) returned nonzero on normal url' +# then the url, -o, and output file are passed through verbatim +grep -Fxq "$URL" "$ARGV_LOG" || die "curl did not receive the url; got: $(cat "$ARGV_LOG")" +grep -Fxqe "-o" "$ARGV_LOG" || die "curl did not receive -o; got: $(cat "$ARGV_LOG")" +grep -Fxq "$FILE" "$ARGV_LOG" || die "curl did not receive the output file; got: $(cat "$ARGV_LOG")" +# and curl-style flags are passed through verbatim (curl keeps -s; it is not translated to wget's -q) +grep -Fxqe "-s" "$ARGV_LOG" || die "curl did not receive -s verbatim; got: $(cat "$ARGV_LOG")" +rm -f "$FILE" + +# given curl is unavailable (the wget-path calls run with PATH limited to our +# fake wget, so neither the fake nor the system curl is found) +rm -f "$TEST_BIN/curl" + +# when nvm_download is invoked with the injection url (wget path) +rm -f "$PROOF" +( PATH="$TEST_BIN"; export PATH; nvm_download "$INJECT_URL" -o - ) || die 'nvm_download (wget) returned nonzero on injection url' +# then the substitution must not have executed +[ ! -e "$PROOF" ] || die "command injection fired via wget path: proof file was created" +grep -Fxq "$INJECT_URL" "$ARGV_LOG" || die "wget did not receive the url as a single literal argument; got: $(cat "$ARGV_LOG")" + +# when invoked with -L -C - --progress-bar URL -o FILE (wget path) +( PATH="$TEST_BIN"; export PATH; nvm_download -L -C - --progress-bar "$URL" -o "$FILE" ) || die 'nvm_download (wget) returned nonzero on normal url' +# then flags are translated to wget equivalents +grep -Fxqe "-c" "$ARGV_LOG" || die "wget did not translate -C - to -c; got: $(cat "$ARGV_LOG")" +grep -Fxqe "--progress=bar" "$ARGV_LOG" || die "wget did not translate --progress-bar; got: $(cat "$ARGV_LOG")" +grep -Fxqe "-O" "$ARGV_LOG" || die "wget did not translate -o to -O; got: $(cat "$ARGV_LOG")" +grep -Fxqe "-L" "$ARGV_LOG" && die "wget should drop -L; got: $(cat "$ARGV_LOG")" +grep -Fxqe "-C" "$ARGV_LOG" && die "wget should not pass -C through; got: $(cat "$ARGV_LOG")" +grep -Fxqe "-" "$ARGV_LOG" && die "wget should drop the lone - after -C; got: $(cat "$ARGV_LOG")" + +# when invoked with -s (wget path) +( PATH="$TEST_BIN"; export PATH; nvm_download -L -s "$URL" -o "$FILE" ) || die 'nvm_download (wget) returned nonzero on -s url' +# then -s becomes -q +grep -Fxqe "-q" "$ARGV_LOG" || die "wget did not translate -s to -q; got: $(cat "$ARGV_LOG")" + +cleanup +echo "nvm_download no eval injection: passed"