Theo de Raadt <[email protected]> wrote: > Michael Mikonos <[email protected]> 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.
As another way of explaining, the logic you created goes like this allow only this file open it do some more calculation oh wait, allow this file also! open it do some more calculation OH WAIT, here is another file to open...
