On Fri, Sep 27, 2019 at 06:53:56PM -0700, Andrew Hewus Fresh wrote:
> On Fri, Sep 27, 2019 at 09:28:39AM +0200, Solene Rapenne wrote:
> > On Thu, Sep 26, 2019 at 05:40:38PM +0200, Otto Moerbeek wrote:
> > > On Thu, Sep 26, 2019 at 05:27:08PM +0200, Solene Rapenne wrote:
> > >
> > > > Hi, now that we have OpenBSD::pledge I thought it would be nice to use
> > > > it in devel/cvsweb
> > > >
> > > > I've been able to tight it to "rpath proc exec prot_exec", removing
> > > > wpath and cpath was possible by commenting lines piping STDERROR to
> > > > /dev/null, that doesn't mean creating dev/null is not required anymore,
> > > > it's still required for cvsweb to work correctly (due to rlog I think).
> > > >
> > > > I updated pkg/README because this requires OpenBSD/Pledge.pm and a so
> > > > file to be copied into the chroot too.
> > > >
> > > > I had some testing on www repository by lot of people and it worked
> > > > perfectly.
> > >
> > > Be careful that error messages do not show up on the web pages
> > > generated by not redirecting stderr...
> > >
> > > -Otto
> >
> > at least slowcgi discard stderr output, not sure for others cgi.
> > if you have a better way (not writing to something) to discard the
> > stderr output that would be better than making slowcgi ignoring it.
>
> Oh, slowcgi doesn't actually discard stderr, instead it passes it
> through to httpd which, in my case, logs it to /var/www/log/error.log,
> but not certain we want to fill that will errors from commands cvsweb
> execs.
>
> > latest patch adding unveil
>
>
> I looked at this a bit more, and with a bit of work, some feedback on
> better use of pledge (and following the recommendation to unveil before
> pledge), we can reduce the promises and unveil'd paths that are needed
> and make them more accurate.
>
> First, by moving `require Compress::Zlib;` and `use Cwd` above the
> pledge, we no longer need to prot_exec in order to load the XS modules,
> which I've been told is a terribly dangerous promise and using it should
> only be done with large amounts of caution.
>
> Then by reading the config file above, hopefully you trust the contents,
> we no longer need to unveil conf/ and can instead read the list of
> CVSrepositories that need to be unveiled. We can also unveil the actual
> CMDs that are configured, although it's probably a good practice to
> hardcode those, rather than using the default search that's in the
> sample config file.
>
> I also think we only need to unveil those CMDs "x" and not "rx", but I
> may have missed something.
>
> I did use a dup(2) of a /dev/null file handle to avoid having to open
> that after pledge and unveil, although with a proper unveil, a wpath
> promise is probably just as safe.
>
> This is running on my site now, so if you want to try to find some 500
> errors, I wouldn't complain.
> http://cvs.afresh1.com/cgi-bin/cvsweb
>
Your diff works fine for me and is much better than my first attempt.
I'm ok for committing it.
No comments:
Post a Comment