Michael Mikonos <m...@ii.net> wrote:
> On Mon, Sep 24, 2018 at 10:53:47PM -0600, Theo de Raadt wrote:
> > Ugh. A diff which doens't check error returns. Averting my gaze
> > is similar to "no way". Hope you have another quarter, because you
> > need to try again
>
> Oops... new coin inserted. I decided to create a fatal_perror()
> function which behaves like perror(). yacc's error.c didn't seem
> to have anything like that. Now if either pledge() or unveil()
> fails we see the appropriate error string instead of always seeing
> "invalid arguments" for pledge().
I don't understand parts of the diff.
> Index: defs.h
> ===================================================================
> RCS file: /cvs/src/usr.bin/yacc/defs.h,v
> retrieving revision 1.18
> diff -u -p -u -r1.18 defs.h
> --- defs.h 2 Dec 2014 15:56:22 -0000 1.18
> +++ defs.h 25 Sep 2018 05:34:27 -0000
> @@ -307,6 +307,7 @@ extern void closure(short *, int);
> extern void finalize_closure(void);
>
> extern __dead void fatal(char *);
> +extern __dead void fatal_perror(char *);
>
> extern void reflexive_transitive_closure(unsigned *, int);
> extern __dead void done(int);
> Index: error.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/yacc/error.c,v
> retrieving revision 1.14
> diff -u -p -u -r1.14 error.c
> --- error.c 8 Mar 2014 01:05:39 -0000 1.14
> +++ error.c 25 Sep 2018 05:34:27 -0000
> @@ -35,6 +35,8 @@
>
> /* routines for printing error messages */
>
> +#include <errno.h>
> +
> #include "defs.h"
>
>
> @@ -45,6 +47,12 @@ fatal(char *msg)
> done(2);
> }
>
> +void
> +fatal_perror(char *msg)
> +{
> + fprintf(stderr, "%s: %s\n", msg, strerror(errno));
> + done(2);
> +}
>
> void
> no_space(void)
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/yacc/main.c,v
> retrieving revision 1.29
> diff -u -p -u -r1.29 main.c
> --- main.c 25 May 2017 20:11:03 -0000 1.29
> +++ main.c 25 Sep 2018 05:34:27 -0000
> @@ -305,10 +305,14 @@ open_files(void)
> create_file_names();
>
> if (input_file == 0) {
> + if (unveil(input_file_name, "r") == -1)
> + fatal_perror("unveil");
> input_file = fopen(input_file_name, "r");
At this point, all files are allowed.
So you restrict it to one.
Then you open it.
You won't open it again.
Why does it remain on the permitted open list?
Why not just open it, and not have it on the permitted open list?
Meanwhile, unveil hasn't been blocked. So if someone finds a bug
at this point which gives them control, they can continue calling
unveil, and get full filesystem access back.
> if (input_file == 0)
> open_error(input_file_name);
> }
> + if (unveil("/tmp", "crw") == -1)
> + fatal_perror("unveil");
> fd = mkstemp(action_file_name);
Same concern.
> if (fd == -1 || (action_file = fdopen(fd, "w")) == NULL)
> open_error(action_file_name);
> @@ -318,11 +322,15 @@ open_files(void)
> open_error(text_file_name);
>
> if (vflag) {
> + if (unveil(verbose_file_name, "cw") == -1)
> + fatal_perror("unveil");
> verbose_file = fopen(verbose_file_name, "w");
Same concern.
> if (verbose_file == 0)
> open_error(verbose_file_name);
> }
> if (dflag) {
> + if (unveil(defines_file_name, "cw") == -1)
> + fatal_perror("unveil");
> defines_file = fopen(defines_file_name, "w");
Same concern.
> if (defines_file == NULL)
> open_write_error(defines_file_name);
> @@ -330,24 +338,30 @@ open_files(void)
> if (fd == -1 || (union_file = fdopen(fd, "w")) == NULL)
> open_error(union_file_name);
> }
> + if (unveil(output_file_name, "cw") == -1)
> + fatal_perror("unveil");
> output_file = fopen(output_file_name, "w");
Same concern.
> if (output_file == 0)
> open_error(output_file_name);
>
> if (rflag) {
> + if (unveil(code_file_name, "cw") == -1)
> + fatal_perror("unveil");
> code_file = fopen(code_file_name, "w");
Same concern.
The best approach for using unveil should be more similar to pledge -
calculate the valid security ruleset early on. Hoist initialization
earlier, and hoist security initialization code even earlier if possible.
That means, try to seperate the generation of the paths way up front,
unveil them, then pledge without unveil or unveil(NULL,NULL), and then
let the post-initialization part of the program continue.
> if (code_file == 0)
> open_error(code_file_name);
> } else
> code_file = output_file;
> + if (unveil(NULL, NULL) == -1)
> + fatal_perror("unveil");
> }
>
>
> int
> main(int argc, char *argv[])
> {
> - if (pledge("stdio rpath wpath cpath", NULL) == -1)
> - fatal("pledge: invalid arguments");
> + if (pledge("stdio rpath wpath cpath unveil", NULL) == -1)
> + fatal_perror("pledge");
>
> set_signals();
> getargs(argc, argv);
Use of persistant "unveil" without another pledge later taking it away,
is also a red flag.
