proposal: os/exec: return ErrDot from LookPath when argument is relative path

[rolandshoemaker]

The package documentation for os/exec and the function documentation for LookPath detail security properties of LookPath which attempt to reduce issues around discovery of binaries in the current directory, and say that LookPath will not resolve a program "using an implicit or explicit path entry relative to the current directory" without also returning ErrDot. This is currently not entirely true.

If you are in a directory with contains a child directory, bin, which itself contains an executable, prog, and you call exec.LookPath("bin/prog") it will return "bin/prog", nil. On the face this seems reasonable, you asked for a path relative to the current directory, and it gave it to you. The problem is this is not immediately obvious given the described security properties of LookPath, and as such makes it somewhat dangerous to make assumptions about its properties which do not hold.

Based on an informal survey of a few developers, none correctly guessed the result of the example described above, with the majority assuming the result would be "bin/prog", ErrDot, which I think is the reasonable assumption.

I propose we either make LookPath always return ErrDot if the path it is returning is not absolute (which I believe is in line with the current documentation), or update the documentation to be explicit that it will not return ErrDot if you explicitly pass a relative path (I have a somewhat strong preference for the former, but could be convinced otherwise).

[rittneje]

If I execute a command containing a slash, then by definition the PATH environment variable is not consulted. Consequently, there isn't any "path lookup" that occurs. Hence returning exec.ErrDot would violate its documentation.

ErrDot indicates that a path lookup resolved to an executable in the current directory due to ‘.’ being in the path, either implicitly or explicitly.

Since this would technically be a breaking change, I think we need a high bar for making the proposed change. Is there a concrete security issue today?

All of that said, is there a real use case for passing a name containing a slash to exec.LookPath? Seems kind of pointless.

[rolandshoemaker]

If I execute a command containing a slash, then by definition the PATH environment variable is not consulted. Consequently, there isn't any "path lookup" that occurs. Hence returning exec.ErrDot would violate its documentation.

Yes, the LookPath documentation makes this point:

If file contains a slash, it is tried directly and the PATH is not consulted.

but then contradicts itself in the second paragraph:

In older versions of Go, LookPath could return a path relative to the current directory. As of Go 1.19, LookPath will instead return that path along with an error satisfying errors.Is(err, ErrDot). See the package documentation for more details.

the package documentation then makes this more confusing with:

[...] as of Go 1.19, this package will not resolve a program using an implicit or explicit path entry relative to the current directory.

Which makes the operation of the function extremely confusing. If you pass a relative path, it will be tried directly, but will it be returned with ErrDot, or without? I think we can just fix the documentation here, but I personally find the operation of the API to be divergent from its (seeming) intended function.

In practice it seems like most people assume LookPath is either only consulting PATH, or trying absolute paths directly, but not consulting paths relative to the current directory. We've seen this multiple times where the input to LookPath is not properly sanitized as the developer expects it will not return things relative to the current directory, resulting in security issues where a tool maybe execute an arbitrary program based on the directory structure it is executed in (this was at least partially responsible for #62198).

[rittneje]

@rolandshoemaker The package documentation is correct as-is. It is not "using an implicit or explicit path entry relative to the current directory" because it is not using a path entry (i.e., the PATH environment variable) at all. It is strictly the documentation of exec.LookPath that makes inconsistent claims about what exec.ErrDot is supposed to mean.

I will also point out that even if you make this change, developers still have to know to use exec.LookPath at all. If you directly use exec.Command you will not get any error, and changing that is definitely a much bigger breaking change. And diverging exec.LookPath and exec.Command on this point seems like it could lead to greater confusion.

With regards to the linked issue, I question whether even allowing an absolute path is really intended. To that end, it seems like the true problem isn't so much a need for exec.ErrDot but rather "you asked for a path lookup but the input does not need one." So maybe it would be clearer to add a new error case for that specifically?

[rolandshoemaker]

@rolandshoemaker The package documentation is correct as-is. It is not "using an implicit or explicit path entry relative to the current directory" because it is not using a path entry (i.e., the PATH environment variable) at all.

I think this interpretation is putting a huge amount of weight on the reader properly understanding that in "this package will not resolve a program" the word resolve refers to the usage of the PATH lookup algorithm, and not simply that it will not return a relative path at all.

I will also point out that even if you make this change, developers still have to know to use exec.LookPath at all. If you directly use exec.Command you will not get any error, and changing that is definitely a much bigger breaking change. And diverging exec.LookPath and exec.Command on this point seems like it could lead to greater confusion.

How would this cause divergence? Command only uses LookPath if the "name contains no path separators", i.e only if it is passed a bare program name.

I think the strongest argument against my proposal is that this diverges from the semantics of most Linux shell implementations and the Windows command prompt.

With regards to the linked issue, I question whether even allowing an absolute path is really intended. To that end, it seems like the true problem isn't so much a need for exec.ErrDot but rather "you asked for a path lookup but the input does not need one." So maybe it would be clearer to add a new error case for that specifically?

This seems like a significantly more serious breaking change, no?

[rittneje]

I think this interpretation is putting a huge amount of weight on the reader properly understanding that in "this package will not resolve a program" the word resolve refers to the usage of the PATH lookup algorithm, and not simply that it will not return a relative path at all.

The next two sentences explain further.

That is, if you run exec.LookPath("go"), it will not successfully return ./go on Unix nor .\go.exe on Windows, no matter how the path is configured. Instead, if the usual path algorithms would result in that answer, these functions return an error err satisfying errors.Is(err, ErrDot).

And later it explicitly mentions that a relative path is not an error.

Code that always wants to run a program from the current directory can be rewritten to say "./prog" instead of "prog".

So I feel the existing docs are quite clear. But it should probably have said "$PATH entry" instead of "path entry" to reinforce what it means.

---

How would this cause divergence? Command only uses LookPath if the "name contains no path separators", i.e only if it is passed a bare program name.

Because if I pass a command name like "./go" to exec.LookPath now I get exec.ErrDot but if I pass the same name to exec.Command I don't. That's very confusing, and it makes the meaning of exec.ErrDot inconsistent.

This seems like a significantly more serious breaking change, no?

You are already going to break everyone that passes a relative path to exec.LookPath. Plus now if I get exec.ErrDot it might mean that my PATH contains a dot, or it might mean the executable name is a relative path. The only way to tell them apart is to look at the executable name, at which point there was no use in returning exec.ErrDot in particular.