Ted Unangst <[email protected]> wrote: > Jake Champlin wrote: > > - readscores(1); > > penalty = loot = 0; > > initscr(); > > + if (unveil(scorepath, "rwc") == -1) > > + err(1, "unveil"); > > +#ifdef LOGGING > > + if (unveil(logpath, "rwc") == -1) > > + err(1, "unveil"); > > + logfile = fopen(logpath, "a"); > > +#endif > > + readscores(1); > > > > if (pledge("stdio tty", NULL) == -1) > > err(1, "pledge"); > > We're unveiling the same file we're going to immediately open. And then we > never open it later, as proved by pledge without rpath. There doesn't seem to > be any benefit in this case. > > If we needed to reopen these files at some later point, this would be the > right thing.
I've seen this poor pattern a few times, so I let me make a few comments. unveil(path, "r"); open(path, O_RDONLY) This is more expensive than just doing open(). After the open() is finished, the kernel still has to remember the vnode. Secondly, it has a subtle race. Both of those system calls are traversing the path. That's a form of TOCTOU. Always try to traverse a path once. Third, installing unveil's without unveil(NULL,NULL) or dropping pledge "unveil" has on security benefit. In the sample above, a bug in the readscores() code can read anywhere in the filesystem. Please think carefully about what I've mentioned. I think a common pattern is for people to read the unveil() manual page and determine they know how to use unveil correctly. It is better to read many uses of unveil() throughout the source tree, since the usage pattern is highly idiomatic and beyond the scope the manual page can describe. This idiomatic pattern just isn't ingrained in everyone's minds yet, so do read other code...
