unveil(2): new corner case: failure on using a directory if not already exists

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

unveil(2): new corner case: failure on using a directory if not already exists

Sebastien Marie-3
Hi,

solene@ reported on ports an error with unveil(2) on creating
subdirectories on previously not existent directory, for a port she
tried to unveiled.

        https://marc.info/?l=openbsd-ports&m=155992715132013&w=2

so I tried to reproduced it with simple C program.

        $ cat test.c
        #include <sys/stat.h>

        #include <err.h>
        #include <errno.h>
        #include <stdio.h>
        #include <stdlib.h>
        #include <unistd.h>

        int
        main(int argc, char *argv[])
        {
                struct stat sb;
                int error;

                /* check if "test" already exists */
                error = stat("test", &sb);
                if (error == 0)
                        errx(EXIT_FAILURE, "test already exists, please remove it for test");
                else if ((error == -1) && (errno != ENOENT))
                        err(EXIT_FAILURE, "stat: test");

                /* unveil the name in current directory */
                if (unveil("test", "rwc") == -1)
                        err(EXIT_FAILURE, "unveil: test");

                /* create a directory with that name */
                if (mkdir("test", 0700) == -1)
                        err(EXIT_FAILURE, "mkdir: test");

                /* create a subdirectory in it */
                if (mkdir("test/foo", 0700) == -1)
                        err(EXIT_FAILURE, "mkdir: test/foo");

                return EXIT_SUCCESS;
        }

        $ cc -Wall test.c && ./a.out
        a.out: mkdir: test/foo: No such file or directory


As the "test" vnode doesn't exists in "." at unveil(2) time, it will
record a "non-directory path" name attached to "." vnode (as documented
in man page).

Later, when mkdir(2) will try to use it, namei() will use
unveil_check_compoment() during path traversal, and the function which
only look at "directory path" (using unveil_lookup()), and will not
found it.

The problem is "test" vnode isn't unveiled as a directory, but is
unveiled as a name in "." directory.

Please note the problem exists for any use of "test" as a directory: all
traversal using it will return ENOENT.


Now I am unsure what to do with that.

We could just add a new CAVEATS in unveil(2) as we already have for
another directory subtility:

        a directory that is removed and recreated after a call to
        unveil() will appear to not exist.

Or we could try to duplicate the logical from unveil_check_final() to
unveil_check_compoment(), but I fear of corner-cases it could introduce.

Thanks.
--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: unveil(2): new corner case: failure on using a directory if not already exists

Theo de Raadt-2
> solene@ reported on ports an error with unveil(2) on creating
> subdirectories on previously not existent directory, for a port she
> tried to unveiled.

Step back for a moment.

Let's say you are allowed to create such directories.

Is that a safe thing to provide?

Non-existant was only supposed to be about files creation, not
directories or hard links or symbolic links or device nodes etc
etc etc, because some of those give you potential for filesystem
aliasing situations.





Reply | Threaded
Open this post in threaded view
|

Re: unveil(2): new corner case: failure on using a directory if not already exists

Bob Beck-3
We should clarify the man page.  trying to think about wording.

On Sat, Jun 8, 2019 at 01:10 Theo de Raadt <[hidden email]> wrote:

> > solene@ reported on ports an error with unveil(2) on creating
> > subdirectories on previously not existent directory, for a port she
> > tried to unveiled.
>
> Step back for a moment.
>
> Let's say you are allowed to create such directories.
>
> Is that a safe thing to provide?
>
> Non-existant was only supposed to be about files creation, not
> directories or hard links or symbolic links or device nodes etc
> etc etc, because some of those give you potential for filesystem
> aliasing situations.
>
>
>
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: unveil(2): new corner case: failure on using a directory if not already exists

Theo de Raadt-2
In reply to this post by Sebastien Marie-3
The man page says:

     Non-directory paths are remembered by name within their containing
     directory, and so may be created, removed, or re-created after a call to
     unveil() and still appear to exist.

This piece of documentation covers a number of cases, including this one.
It says that non-directory (ie. files) work.

But the manual page makes no claim about directories.  Either an
assumption is being made here that directories work the same as files
(the sentence says otherwise), or there is an assumption that because
the behaviour of new directory creation isn't mentioned, that directories
can therefore be created.

Well, the story is that directories cannot be created.  That opens the
door to filesystem loops.

You cannot create subdirectories, you can only create files.

I think the unveil and pledge work in irrsi is insane.  It is breaking
behaviours people need.  Therefore the attempt to side-step that by adding
a -u argument to disable the security measures when the program crashes.
I think that is insane.  Great, let's secure software.  But don't add
a flag for a low-level security mitigation which people turn on if the
program crashes.  A user will be 20 minutes in some critical conversation,
trigger a feature which causes a crash, or not be able to access some object?
That is insane.

unveil and pledge cannot be added abstractly to applications not designed
for the model, and that's precisely why you don't see unveil or pledge
enabling flags throughout the source tree.

I believe Code mustf either be correctly adapted to the unveil/pledge
universe, or it should be left untouched.

Sebastien Marie <[hidden email]> wrote:

> Hi,
>
> solene@ reported on ports an error with unveil(2) on creating
> subdirectories on previously not existent directory, for a port she
> tried to unveiled.
>
> https://marc.info/?l=openbsd-ports&m=155992715132013&w=2
>
> so I tried to reproduced it with simple C program.
>
> $ cat test.c
> #include <sys/stat.h>
>
> #include <err.h>
> #include <errno.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
>
> int
> main(int argc, char *argv[])
> {
> struct stat sb;
> int error;
>
> /* check if "test" already exists */
> error = stat("test", &sb);
> if (error == 0)
> errx(EXIT_FAILURE, "test already exists, please remove it for test");
> else if ((error == -1) && (errno != ENOENT))
> err(EXIT_FAILURE, "stat: test");
>
> /* unveil the name in current directory */
> if (unveil("test", "rwc") == -1)
> err(EXIT_FAILURE, "unveil: test");
>
> /* create a directory with that name */
> if (mkdir("test", 0700) == -1)
> err(EXIT_FAILURE, "mkdir: test");
>
> /* create a subdirectory in it */
> if (mkdir("test/foo", 0700) == -1)
> err(EXIT_FAILURE, "mkdir: test/foo");
>
> return EXIT_SUCCESS;
> }
>
> $ cc -Wall test.c && ./a.out
> a.out: mkdir: test/foo: No such file or directory
>
>
> As the "test" vnode doesn't exists in "." at unveil(2) time, it will
> record a "non-directory path" name attached to "." vnode (as documented
> in man page).
>
> Later, when mkdir(2) will try to use it, namei() will use
> unveil_check_compoment() during path traversal, and the function which
> only look at "directory path" (using unveil_lookup()), and will not
> found it.
>
> The problem is "test" vnode isn't unveiled as a directory, but is
> unveiled as a name in "." directory.
>
> Please note the problem exists for any use of "test" as a directory: all
> traversal using it will return ENOENT.
>
>
> Now I am unsure what to do with that.
>
> We could just add a new CAVEATS in unveil(2) as we already have for
> another directory subtility:
>
> a directory that is removed and recreated after a call to
> unveil() will appear to not exist.
>
> Or we could try to duplicate the logical from unveil_check_final() to
> unveil_check_compoment(), but I fear of corner-cases it could introduce.
>
> Thanks.
> --
> Sebastien Marie
>