Re: [Nano-devel] segmentation fault when trying to browse to an inaccessible directory

[Mike Frysinger]

On 02 Jun 2016 17:31, Rishabh Dave wrote:
> I have figured 2 ways to solve this bug. I have tried them, patches
> are attached. Which method/approach should I use on to fix the bug?

afaict, neither of these methods actually solve the problem.
they both suffer from TOCTOU races -- if your chdir/access passes,
then something else changes the perms, then nano keeps going, you
still hit the same scenario.  the only real method is to handle
the perm errors when they show up.

> #1 chdir-to-check-path.patch - we change directory to see if we can
> access it, print an error if we can't and return to original
> directory. But then, as always, we shouldn't be sure that the path
> still exists. But we use this method elsewhere.

i strongly dislike progs that attempt to chdir and then chdir back.
you're assuming the cwd has not been moved/modified in the fs since
nano was started, but there's no guarantee of that at all.  so the
active state of the process might be corrupted when you try to chdir
back and that path doesn't exist or is actually a diff dir.

the closest you could get would be to do an open(".", O_DIRECTORY)
to get a fd, and then fchdir back to that.

> #2 access-to-check-path.patch - access() from unistd.h can check
> permissions but manual describes exactly our scenario -  "Using
> access() to check if a user is authorized to, for example, open a file
> before actually doing so using open(2) creates a security  hole,
> because  the  user  might  exploit the short time interval between
> checking and opening the file to manipulate it.  For this  reason,
> the  use  of this system call should be avoided.  (In the example just
> described, a safer alternative would be to temporarily switch  the
> process's effective user ID to the real ID and then call open(2).)".
> 
> 
> After patch #2, the code  i.e. use of stat(), ISDIR() and access()
> sequentially is strikingly similar to the code in has_valid_path(),
> files.c. Some modifications - which will make it difficult to read but
> will reuse the block of code - can be made to do the same task.

the diff is that those other helper funcs are designed to do some
sanity checks up front and show nicer user messages rather than be
used to catch errors.
-mike

signature.asc
Description: Digital signature