brew.sh: Don't allow system tmp dirs as prefixes #4397

woodruffw · 2018-06-30T13:47:05Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew tests with your changes locally?

This explicitly forbids the use of the system's temporary directory as an installation prefix, as we perform checks against the temporary directory when doing binary relocation.

See: Homebrew/homebrew-core#28877 (comment)

ilovezfs · 2018-06-30T13:51:44Z

Library/Homebrew/brew.sh

+Your HOMEBREW_PREFIX is in the system temporary directory, which Homebrew
+uses to store downloads and builds. You can resolve this by installing Homebrew to
+either the standard prefix (/usr/local/) or to a non-standard prefix that is not
+the system temporary directory.


s/the/in the/

MikeMcQuaid · 2018-06-30T15:26:28Z

Library/Homebrew/brew.sh

+  if [[ $(realpath "${HOMEBREW_PREFIX}") = /private/tmp/* ]]
+  then
+    odie <<EOS
+Your HOMEBREW_PREFIX is in the system temporary directory, which Homebrew


Maybe worth just saying e.g. /private/tmp? Also, should we do this on Linux or with a different directory?

I thought that the connection between /tmp and /private/tmp might confuse some users (Apple links /tmp to /private/tmp for FHS compliance), so I just went with "system temporary directory." I can change it/add those directories to the message if you think it'll improve the error, though.

The relocation code at fault isn't used on Linux, so Linuxbrew users might not encounter this problem at all. I'll confirm that in a bit with some local testing.

MikeMcQuaid · 2018-06-30T15:26:40Z

Library/Homebrew/brew.sh

+    odie <<EOS
+Your HOMEBREW_PREFIX is in the system temporary directory, which Homebrew
+uses to store downloads and builds. You can resolve this by installing Homebrew to
+either the standard prefix (/usr/local/) or to a non-standard prefix that is not


(/usr/local) maybe?

ilovezfs · 2018-06-30T15:35:45Z

The underlying issue specifically affects HOMEBREW_TEMP regardless of whether it's the default /tmp or some other path (and if HOMEBREW_TEMP isn't /tmp then /tmp is unaffected) so I'm wondering if the check and the messaging should check the value of HOMEBREW_TEMP for a non-default value and handle that case correctly too.

woodruffw · 2018-06-30T15:58:21Z

so I'm wondering if the check and the messaging should check the value of HOMEBREW_TEMP for a non-default value and handle that case correctly too.

Yeah, I'll update it to handle this. It gets a little complicated, though, thanks to the /tmp <-> /private/tmp link. Would anybody mind if I added a realpath to the assignment of HOMEBREW_TEMP in config.rb, so that we don't have to worry about handling all of the possible permutations of users installing in /tmp and /private/tmp?

Edit: Actually, I guess HOMEBREW_TEMP doesn't even exist in brew.sh's environment, since we don't set it until config.rb. In that case, I'll set it (with fallback) in bin/brew.

MikeMcQuaid · 2018-07-01T16:28:23Z

bin/brew

@@ -72,7 +73,7 @@ then

  FILTERED_ENV=()
  # Filter all but the specific variables.
-  for VAR in HOME SHELL PATH TERM COLUMNS LOGNAME USER CI TRAVIS SSH_AUTH_SOCK SUDO_ASKPASS \
+  for VAR in HOME SHELL PATH TEMP TERM COLUMNS LOGNAME USER CI TRAVIS SSH_AUTH_SOCK SUDO_ASKPASS \


Why do we want to add this?

Oh, I misread this code. I thought this was explicitly whitelisting HOMEBREW_TEMP, but I guess it would clobber it instead.

MikeMcQuaid · 2018-07-01T16:28:35Z

Library/Homebrew/config.rb

@@ -39,7 +39,7 @@
 HOMEBREW_LOGS = Pathname.new(ENV["HOMEBREW_LOGS"] || "~/Library/Logs/Homebrew/").expand_path

 # Must use /tmp instead of $TMPDIR because long paths break Unix domain sockets
-HOMEBREW_TEMP = Pathname.new(ENV.fetch("HOMEBREW_TEMP", "/tmp"))
+HOMEBREW_TEMP = Pathname.new(ENV.fetch("HOMEBREW_TEMP", "/tmp")).realpath


This will never be unset/set to /tmp now.

We provide a /private/tmp fallback in bin/brew, so this is no longer necessary.

woodruffw · 2018-07-01T16:50:57Z

Looks like the test failure is coming from brew update-test, which uses HOMEBREW_TEMP via mktemp to test brew update.

Any thoughts on how best to resolve this? I could add another environment variable allowing the behavior in the PR to be overridden for testing purposes, or change brew update-test to avoid HOMEBREW_TEMP, off the top of my head. Neither of those is particularly clean, though 😞

MikeMcQuaid · 2018-07-01T16:53:01Z

change brew update-test to avoid HOMEBREW_TEMP

I'd vote on doing this; no reason it can't act in the current directory (as a developer, testing command) if it's cleaned up afterwards.

MikeMcQuaid · 2018-07-01T16:53:43Z

bin/brew

@@ -21,6 +21,7 @@ symlink_target_directory() {
 BREW_FILE_DIRECTORY="$(quiet_cd "${0%/*}/" && pwd -P)"
 HOMEBREW_BREW_FILE="${BREW_FILE_DIRECTORY%/}/${0##*/}"
 HOMEBREW_PREFIX="${HOMEBREW_BREW_FILE%/*/*}"
+HOMEBREW_TEMP="${HOMEBREW_TEMP:-/private/tmp}"


Will this upset Linux? Worth setting in brew.sh to a different default for macOS and not-macOS?

Yeah, /private/tmp is macOS only. Moving it to brew.sh sounds good.

Additionally, assign HOMEBREW_TEMP based on the host system (/tmp for Linux, /private/tmp for macOS).

MikeMcQuaid · 2018-07-01T17:24:19Z

Library/Homebrew/brew.sh

@@ -108,6 +108,8 @@ then
  then
    HOMEBREW_CACHE="$HOME/Library/Caches/Homebrew"
  fi
+
+  HOMEBREW_TEMP="${HOMEBREW_TEMP:-/private/tmp}"


👍 to defining here. I wonder if it's worth doing just the same -z as above for consistency? Alternatively; replace the -zs that have no complex logic with this form?

I'm in favor of replacing with the :- form, since it's the standard bashism for a default value and we use similar bashisms elsewhere in the code.

MikeMcQuaid · 2018-07-01T17:25:44Z

Library/Homebrew/brew.sh

@@ -294,6 +299,19 @@ EOS
 }
 check-run-command-as-root

+check-prefix-is-not-tmpdir() {
+  if [[ "${HOMEBREW_PREFIX}" = "${HOMEBREW_TEMP}"* ]]


This wants guarded to macOS only, check against the system temp rather than just HOMEBREW_TEMP (maybe a HOMEBREW_SYSTEM_TEMP var?) and use that directory in the output, too.

HOMEBREW_SYSTEM_TEMP could be potentially used wherever private/tmp is hardcoded, too.

check against the system temp rather than just HOMEBREW_TEMP

I might be misunderstanding, but: won't this leave open the case where they set both their prefix and their temp to something weird, e.g., $HOME? We want to stop all situations where Homebrew is both installing to and using the same prefix as a temporary directory, not just /private/tmp.

@woodruffw Ok so is it fine to e.g. have the prefix in /tmp if HOMEBREW_TEMP is set elsewhere?

Yep, exactly.

MikeMcQuaid · 2018-07-02T09:15:36Z

Library/Homebrew/brew.sh

-      HOMEBREW_CACHE="$HOME/.cache/Homebrew"
-    fi
-  fi
+  cache_home="${XDG_CACHE_HOME:-${HOME}/.cache}"


Use all caps for variable name here and just don't export it.

MikeMcQuaid · 2018-07-02T09:17:05Z

Library/Homebrew/brew.sh

+  if [[ "${HOMEBREW_PREFIX}" = "${HOMEBREW_TEMP}"* ]]
+  then
+    odie <<EOS
+Your HOMEBREW_PREFIX is in the system temporary directory, which Homebrew


HOMEBREW_TEMP isn't necessary the system temporary directory. To expand on my previous comment (and avoid the autohiding): is the issue if:

the prefix is in the system temporary directory

the prefix is the in the HOMEBREW_TEMP temporary directory

either

We should ensure that the wording makes that clear and the logic is as minimal as possible to the actual problems we've seen people experience.

The issue is when the prefix is in the HOMEBREW_TEMP temporary directory. I'll correct the language here 🙂

MikeMcQuaid · 2018-07-03T07:31:56Z

Thanks @woodruffw and sorry for all the back and forth! Took me a while to understand what was going on here.

ilovezfs · 2018-07-03T07:32:03Z

Library/Homebrew/config.rb

@@ -39,7 +39,7 @@
 HOMEBREW_LOGS = Pathname.new(ENV["HOMEBREW_LOGS"] || "~/Library/Logs/Homebrew/").expand_path

 # Must use /tmp instead of $TMPDIR because long paths break Unix domain sockets
-HOMEBREW_TEMP = Pathname.new(ENV.fetch("HOMEBREW_TEMP", "/tmp"))
+HOMEBREW_TEMP = Pathname.new(ENV["HOMEBREW_TEMP"]).realpath


I think this will crash when the dir doesn't exist.

Yup. Fixing 😭

woodruffw · 2018-07-03T11:40:06Z

~~No problem at all, but: I hadn't applied the brew update-test changes yet, so this still breaks the CI. Maybe worth a revert (or I could just fill it in with another PR)?~~

~~(It's 100% my fault -- I forgot to add the "do not merge" label).~~

Ignore that -- I see #4415 takes care of the brew update-test change too. Thanks @MikeMcQuaid!

brew.sh: Don't allow system tmp dirs as prefixes

35138dd

woodruffw requested review from mistydemeo and ilovezfs June 30, 2018 13:47

ghost assigned woodruffw Jun 30, 2018

ghost added the in progress Maintainers are working on this label Jun 30, 2018

woodruffw added 2 commits June 30, 2018 09:47

brew.sh: Use realpath to calculate tmpdir

949c0cc

brew.sh: Typo

42bd544

woodruffw mentioned this pull request Jun 30, 2018

openssl fails to build in non-standard prefix Homebrew/homebrew-core#28877

Closed

6 tasks

ilovezfs reviewed Jun 30, 2018

View reviewed changes

brew.sh: Tweak language

b6a0b04

MikeMcQuaid reviewed Jun 30, 2018

View reviewed changes

brew.sh: Remove trailing / from prefix in message

c552e65

woodruffw added 3 commits June 30, 2018 12:20

bin/brew: Set HOMEBREW_TEMP, allow export

696a1d1

config: Ensure HOMEBREW_TEMP is absolute

d7cdc9b

brew.sh: Test HOMEBREW_TEMP, not hardcoded tmpdir

8e4aab9

MikeMcQuaid reviewed Jul 1, 2018

View reviewed changes

woodruffw added 2 commits July 1, 2018 12:33

config: Remove /tmp fallback

72bc6b1

We provide a /private/tmp fallback in bin/brew, so this is no longer necessary.

bin/brew: Revert allowing export of TEMP

a8bcb5d

MikeMcQuaid reviewed Jul 1, 2018

View reviewed changes

brew.sh: Move HOMEBREW_TEMP declaration

039a4ee

Additionally, assign HOMEBREW_TEMP based on the host system (/tmp for Linux, /private/tmp for macOS).

MikeMcQuaid reviewed Jul 1, 2018

View reviewed changes

woodruffw added 2 commits July 1, 2018 19:19

brew.sh: Use bashisms for default values

bde7c6b

brew.sh: Only do tmpdir check on macOS

68d3dc1

MikeMcQuaid reviewed Jul 2, 2018

View reviewed changes

brew.sh: Upcase var, tweak error language

fd01415

MikeMcQuaid approved these changes Jul 3, 2018

View reviewed changes

MikeMcQuaid merged commit 09ee556 into Homebrew:master Jul 3, 2018

ghost removed the in progress Maintainers are working on this label Jul 3, 2018

ilovezfs reviewed Jul 3, 2018

View reviewed changes

woodruffw deleted the forbid-temp-prefix branch July 3, 2018 15:34

lock bot added the outdated PR was locked due to age label Aug 2, 2018

lock bot locked as resolved and limited conversation to collaborators Aug 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

brew.sh: Don't allow system tmp dirs as prefixes #4397

brew.sh: Don't allow system tmp dirs as prefixes #4397

woodruffw commented Jun 30, 2018

ilovezfs Jun 30, 2018

MikeMcQuaid Jun 30, 2018

woodruffw Jun 30, 2018

MikeMcQuaid Jun 30, 2018

ilovezfs commented Jun 30, 2018

woodruffw commented Jun 30, 2018 •

edited

Loading

MikeMcQuaid Jul 1, 2018

woodruffw Jul 1, 2018

MikeMcQuaid Jul 1, 2018

woodruffw commented Jul 1, 2018

MikeMcQuaid commented Jul 1, 2018

MikeMcQuaid Jul 1, 2018

woodruffw Jul 1, 2018

MikeMcQuaid Jul 1, 2018

woodruffw Jul 1, 2018

MikeMcQuaid Jul 1, 2018

MikeMcQuaid Jul 1, 2018

woodruffw Jul 1, 2018

MikeMcQuaid Jul 2, 2018

woodruffw Jul 2, 2018

MikeMcQuaid Jul 2, 2018

MikeMcQuaid Jul 2, 2018

woodruffw Jul 2, 2018

MikeMcQuaid commented Jul 3, 2018

ilovezfs Jul 3, 2018

MikeMcQuaid Jul 3, 2018

MikeMcQuaid Jul 3, 2018

ilovezfs Jul 3, 2018

woodruffw commented Jul 3, 2018 •

edited

Loading

brew.sh: Don't allow system tmp dirs as prefixes #4397

brew.sh: Don't allow system tmp dirs as prefixes #4397

Conversation

woodruffw commented Jun 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilovezfs commented Jun 30, 2018

woodruffw commented Jun 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw commented Jul 1, 2018

MikeMcQuaid commented Jul 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw commented Jul 3, 2018 • edited Loading

woodruffw commented Jun 30, 2018 •

edited

Loading

woodruffw commented Jul 3, 2018 •

edited

Loading