Re: add pledge to devel/cvsweb

[Ingo Schwarze]

Hi Solene,

Solene Rapenne wrote on Tue, Oct 01, 2019 at 02:15:02PM +0200:

> I don't really want to work more on cvsweb,

Fair enough, you get to choose what you want to work on.  :-)

> I did this because it's a cgi scripts run on official
> cvsweb.openbsd.org website and I wanted to help there.

Sure, thank you, so i'll integrate your work into 2.0.7 and 4.0.x
when i find the time.

Whether to commit it to the port until then - i leave that decision to
other porters...

> Altough it may be a good project for a port hackathon if
> someone wants to read / debug some perl.

Good idea.  I might do that in Bukarest if people there consider it
a useful idea.

Yours,
  Ingo

Re: add pledge to devel/cvsweb

[Solene Rapenne]

On Sun, Sep 29, 2019 at 07:37:26PM +0200, Ingo Schwarze wrote:
> Hi Solene,
> 
> Solene Rapenne wrote on Thu, Sep 26, 2019 at 05:27:08PM +0200:
> 
> > Hi, now that we have OpenBSD::pledge I thought it would be nice to use
> > it in devel/cvsweb
> 
> I think this is a thoroughly bad idea.
> 
> Pledge is useful for well-understood high-quality code.
> 
> But CVSweb, at this point, is very old, barely maintained, low-quality
> code.  With such code, adding pledge is mostly calling for trouble in
> the form of crashes that result from unexpected, but required behaviour.
> 
> Besides, the CVSweb we have in ports is an old version: 2.0.6 (September
> 2002) plus patches.  The latest upstream version is 3.0.6 (September
> 2005), but upstream is long dead.  Doing new development in the form
> of patches to the port looks like a very bad idea.
> 
> Some time ago, i set up a new upstream:
> 
>   http://mandoc.bsd.lv/cvsweb/
> 
> I admit i got distracted, but i hope to return to it.
> 
> If we want to improve CVSweb (which i do think makes sense), then the
> proper course of action, i think, is to
> 
>  1. Commit such improvements as can be easily done and are quite
> useful to the FreeBSD-cvsweb-2_0-branch.  Possibly, pledge can
> be part of that if we are really sure we understand what all the
> code does.  Otherwise, i think step 6 below is the proper time
> for adding pledge.
> 
>  2. Release 2.0.7 from that branch.  This can be a short-term goal,
> the only reason it didn't happen yet is that i got distracted.
> 
>  3. Update the port to 2.0.7.
> 
>  4. Update the bsd.lv and possibly cvsweb.openbsd.org servers to 2.0.7.
> 
>  5. Merge 2.0.7 into the MAIN branch (4.x revision numbers).
> 
>  6. Develop and release cvsweb-4.0.0 as a medium-term project.
> 
>  7. Update the port to 4.0.0.
> 
>  8. Update the two servers.
> 
> If you want to help with that, i'm happy to give you commit access
> to the upstream repository on mandoc.bsd.lv.
>
> If other porters think that solene's work should be committed directly
> to the port, i don't veto that, but i do not consider it useful either.
> I doubt that any running server will update to a new version of the
> port before the release of 2.0.7.
> 
> Yours,
>   Ingo

All of what you said make sense, I did not consider looking for a newer
version before trying to pledge cvsweb.

I don't really want to work more on cvsweb, I did this because it's a
cgi scripts run on official cvsweb.openbsd.org website and I wanted to
help there.. Altough it may be a good project for a port hackathon if
someone wants to read / debug some perl.

Re: add pledge to devel/cvsweb

[Ingo Schwarze]

Hi Solene,

Solene Rapenne wrote on Thu, Sep 26, 2019 at 05:27:08PM +0200:

> Hi, now that we have OpenBSD::pledge I thought it would be nice to use
> it in devel/cvsweb

I think this is a thoroughly bad idea.

Pledge is useful for well-understood high-quality code.

But CVSweb, at this point, is very old, barely maintained, low-quality
code.  With such code, adding pledge is mostly calling for trouble in
the form of crashes that result from unexpected, but required behaviour.

Besides, the CVSweb we have in ports is an old version: 2.0.6 (September
2002) plus patches.  The latest upstream version is 3.0.6 (September
2005), but upstream is long dead.  Doing new development in the form
of patches to the port looks like a very bad idea.

Some time ago, i set up a new upstream:

  http://mandoc.bsd.lv/cvsweb/

I admit i got distracted, but i hope to return to it.

If we want to improve CVSweb (which i do think makes sense), then the
proper course of action, i think, is to

 1. Commit such improvements as can be easily done and are quite
useful to the FreeBSD-cvsweb-2_0-branch.  Possibly, pledge can
be part of that if we are really sure we understand what all the
code does.  Otherwise, i think step 6 below is the proper time
for adding pledge.

 2. Release 2.0.7 from that branch.  This can be a short-term goal,
the only reason it didn't happen yet is that i got distracted.

 3. Update the port to 2.0.7.

 4. Update the bsd.lv and possibly cvsweb.openbsd.org servers to 2.0.7.

 5. Merge 2.0.7 into the MAIN branch (4.x revision numbers).

 6. Develop and release cvsweb-4.0.0 as a medium-term project.

 7. Update the port to 4.0.0.

 8. Update the two servers.

If you want to help with that, i'm happy to give you commit access
to the upstream repository on mandoc.bsd.lv.

If other porters think that solene's work should be committed directly
to the port, i don't veto that, but i do not consider it useful either.
I doubt that any running server will update to a new version of the
port before the release of 2.0.7.

Yours,
  Ingo

Re: add pledge to devel/cvsweb

[Solene Rapenne]

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.

Re: add pledge to devel/cvsweb

[Andrew Hewus Fresh]

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


Index: Makefile
===
RCS file: /cvs/ports/devel/cvsweb/Makefile,v
retrieving revision 1.62
diff -u -p -r1.62 Makefile
--- Makefile12 Jul 2019 20:44:07 -  1.62
+++ Makefile28 Sep 2019 01:47:23 -
@@ -3,7 +3,7 @@
 COMMENT=   CGI script to browse CVS repository trees
 
 DISTNAME=  cvsweb-2.0.6
-REVISION=  27
+REVISION=  28
 CATEGORIES=devel www
 HOMEPAGE=  http://www.freebsd.org/projects/cvsweb.html
 
Index: patches/patch-cvsweb_cgi
===
RCS file: /cvs/ports/devel/cvsweb/patches/patch-cvsweb_cgi,v
retrieving revision 1.13
diff -u -p -r1.13 patch-cvsweb_cgi
--- patches/patch-cvsweb_cgi7 Apr 2013 20:07:24 -   1.13
+++ patches/patch-cvsweb_cgi28 Sep 2019 01:47:23 -
@@ -1,6 +1,7 @@
 $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/04/07 20:07:24 naddy Exp $
 cvsweb.cgi.origThu Sep 26 22:56:05 2002
-+++ cvsweb.cgi Sun Apr  7 14:15:55 2013
+Index: cvsweb.cgi
+--- cvsweb.cgi.orig
 cvsweb.cgi
 @@ -1,4 +1,4 @@
 -#!/usr/bin/perl -wT
 +#!/usr/bin/perl -w
@@ -37,7 +38,86 @@ $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/0
  );
  
  @LOGSORTKEYS = qw(cvs date rev);
-@@ -2014,20 +2009,6 @@ sub doDiff($$) {
+@@ -247,14 +242,44 @@ EOM
+ 
+ # End of configuration variables #
+ 
++use Cwd qw< abs_path >;
+ use Time::Local ();
+ use IPC::Open2 qw(open2);
++use OpenBSD::Pledge;
++use OpenBSD::Unveil;
+ 
+ # Check if the zlib C library interface is installed, and if yes
+ # we can avoid using the extra gzip process.
+ eval { require Compress::Zlib; };
+ $has_zlib = !$@;
+ 
++if (-f $config) {
++  do "$config" or fatal("500 Internal Error",
++'Error in loading configuration file: 
%s%s',
++$config, $@);
++} else {
++  fatal("500 Internal Error",
++'Configuration not found.  Set the variable $config 
in cvsweb.cgi to your cvsweb.conf configuration file first.'
++   );
++}
++
++open my $DEVNULL, '>', '/dev/null' or die "Unable to open /dev/null: $!";
++
++# CVS Repositories
++for ( my $i = 1 ; $i < @CVSrepositories ; $i += 2 ) {
++unveil( $CVSrepositories[$i][1], "r" ) || die "Unable to unveil: 

Re: add pledge to devel/cvsweb

[Andrew Hewus Fresh]

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.

You could pre-open a /dev/null file handle and then dup(2) it instead of
opening a new one.  I haven't had time to really look at the rest of the
patch though.

https://perldoc.perl.org/functions/open.html

You may also, in the Bourne shell tradition, specify an EXPR
beginning with >& , in which case the rest of the string is
interpreted as the name of a filehandle (or file descriptor, if
numeric) to be duped (as in dup(2)) and opened. 


#!/usr/bin/perl
use strict;
use warnings;

use OpenBSD::Pledge;
use OpenBSD::Unveil;

open my $DEVNULL, '>', '/dev/null' or die "Unable to open /dev/null: $!";

pledge( qw( rpath proc exec prot_exec unveil ) ) || die "Can't pledge: $!";

unveil( "/usr/libdata/perl5/", "r" ) || die "Unable to unveil: $!";
unveil( "/bin/sh", "rx" ) || die "Unable to unveil: $!";
unveil() || die "Unable to unveil: $!";

my $pid = open my $child, "-|" // die "Unable to fork: $!";
unless ($pid) {
open STDERR, '>&', $DEVNULL or die "Unable to dup DEVNULL: $!";
exec 'sh', '-c', 'echo stderr >&2; echo stdout';
}

print "got: $_" for readline $child;


-- 
andrew - http://afresh1.com

($do || !$do) && undef($try) ;  # Master of Perl, Yoda is.  H?

Re: add pledge to devel/cvsweb

[Florian Obser]

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.

That's not exactly how this works. Slowcgi transports stderr, but it
does not mix stdout and stderr.  There is a record type for stderr in
the FastCGI spec. Slowcgi transmits stderr inside of that. It's the
webserver that does something with stderr, either drop it on the floor
or log it to the error log.

In practice it does not matter, stderr does not show up on the
delivered webpage. But if someone is looking for the error output they
at least have a chance of finding it.

-- 
I'm not entirely sure you are real.

Re: add pledge to devel/cvsweb

[Sebastien Marie]

On Fri, Sep 27, 2019 at 10:10:29AM +0200, Solene Rapenne wrote:
> On Fri, Sep 27, 2019 at 10:02:35AM +0200, Sebastien Marie wrote:
> > 
> > > Index: patches/patch-cvsweb_cgi
> > > ===
> > > RCS file: /data/cvs/ports/devel/cvsweb/patches/patch-cvsweb_cgi,v
> > > retrieving revision 1.13
> > > diff -u -p -r1.13 patch-cvsweb_cgi
> > > --- patches/patch-cvsweb_cgi  7 Apr 2013 20:07:24 -   1.13
> > > +++ patches/patch-cvsweb_cgi  27 Sep 2019 07:20:20 -
> > > @@ -1,6 +1,7 @@
> > >  $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/04/07 20:07:24 naddy Exp $
> > >  cvsweb.cgi.orig  Thu Sep 26 22:56:05 2002
> > > -+++ cvsweb.cgi   Sun Apr  7 14:15:55 2013
> > > +Index: cvsweb.cgi
> > > +--- cvsweb.cgi.orig
> > >  cvsweb.cgi
> > >  @@ -1,4 +1,4 @@
> > >  -#!/usr/bin/perl -wT
> > >  +#!/usr/bin/perl -w
> > 
> > any reason to remove the taint flag on perl shebang ?
>  
> no idea, it was already there
> 

ah, my bad. I missed it is a diff, and not the diff of the diff :)

-- 
Sebastien Marie

Re: add pledge to devel/cvsweb

[Solene Rapenne]

On Fri, Sep 27, 2019 at 10:02:35AM +0200, Sebastien Marie wrote:
> Just two comments.
> 
> > Index: patches/patch-cvsweb_cgi
> > ===
> > RCS file: /data/cvs/ports/devel/cvsweb/patches/patch-cvsweb_cgi,v
> > retrieving revision 1.13
> > diff -u -p -r1.13 patch-cvsweb_cgi
> > --- patches/patch-cvsweb_cgi7 Apr 2013 20:07:24 -   1.13
> > +++ patches/patch-cvsweb_cgi27 Sep 2019 07:20:20 -
> > @@ -1,6 +1,7 @@
> >  $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/04/07 20:07:24 naddy Exp $
> >  cvsweb.cgi.origThu Sep 26 22:56:05 2002
> > -+++ cvsweb.cgi Sun Apr  7 14:15:55 2013
> > +Index: cvsweb.cgi
> > +--- cvsweb.cgi.orig
> >  cvsweb.cgi
> >  @@ -1,4 +1,4 @@
> >  -#!/usr/bin/perl -wT
> >  +#!/usr/bin/perl -w
> 
> any reason to remove the taint flag on perl shebang ?
 
no idea, it was already there

> > @@ -37,7 +38,43 @@ $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/0
> >   );
> >   
> >   @LOGSORTKEYS = qw(cvs date rev);
> > -@@ -2014,20 +2009,6 @@ sub doDiff($$) {
> > +@@ -249,7 +244,26 @@ EOM
> > + 
> > + use Time::Local ();
> > + use IPC::Open2 qw(open2);
> > ++use OpenBSD::Pledge;
> > ++use OpenBSD::Unveil;
> > + 
> > ++pledge( qw( rpath proc exec prot_exec unveil ) ) || die "Can't pledge: 
> > $!";
> > ++
> > ++# directories
> > ++unveil( "/usr/libdata/perl5/", "r" ) || die "Unable to unveil: $!";
> > ++unveil( "/cvs/", "r" ) || die "Unable to unveil: $!";
> > ++unveil( "/conf/", "r" ) || die "Unable to unveil: $!";
> > ++
> > ++# files
> > ++unveil( "/dev/null", "r" ) || die "Unable to unveil: $!";
> > ++unveil( "/usr/bin/rcsdiff", "rx" ) || die "Unable to unveil: $!";
> > ++unveil( "/usr/bin/rlog", "rx" ) || die "Unable to unveil: $!";
> > ++unveil( "/usr/bin/cvs", "rx" ) || die "Unable to unveil: $!";
> > ++unveil( "/usr/bin/uname", "rx" ) || die "Unable to unveil: $!";
> > ++
> > ++unveil() || die "Unvable to unveil: $!";
> > ++
> > ++
> 
> the proper idiom is :
> - unveil() calls first
> - pledge() call without "unveil"
> 
> you will have almost(*) the same result that what you have, but it is more 
> obvious
> that unveil(2) is not permitted after pledge(2) call.
> 
> (*) almost: with your diff, calling unveil(2) later result in an error 
> (EPERM) ;
> whereas without "unveil" in pledge(2) is would result a program 
> terminaison
> (pledge violation).
 
indeed, it makes more sense this way
> 
> 
> And if you want to avoid patching /dev/null usage, you could add "wpath" in
> pledge(2) and unveil("/dev/null", "rw"). The capability to write will be
> restricted to only /dev/null with pledge+unveil. so no other write will be
> allowed on filesystem.
> 

good idea

Re: add pledge to devel/cvsweb

[Sebastien Marie]

Just two comments.

> Index: patches/patch-cvsweb_cgi
> ===
> RCS file: /data/cvs/ports/devel/cvsweb/patches/patch-cvsweb_cgi,v
> retrieving revision 1.13
> diff -u -p -r1.13 patch-cvsweb_cgi
> --- patches/patch-cvsweb_cgi  7 Apr 2013 20:07:24 -   1.13
> +++ patches/patch-cvsweb_cgi  27 Sep 2019 07:20:20 -
> @@ -1,6 +1,7 @@
>  $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/04/07 20:07:24 naddy Exp $
>  cvsweb.cgi.orig  Thu Sep 26 22:56:05 2002
> -+++ cvsweb.cgi   Sun Apr  7 14:15:55 2013
> +Index: cvsweb.cgi
> +--- cvsweb.cgi.orig
>  cvsweb.cgi
>  @@ -1,4 +1,4 @@
>  -#!/usr/bin/perl -wT
>  +#!/usr/bin/perl -w

any reason to remove the taint flag on perl shebang ?

> @@ -37,7 +38,43 @@ $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/0
>   );
>   
>   @LOGSORTKEYS = qw(cvs date rev);
> -@@ -2014,20 +2009,6 @@ sub doDiff($$) {
> +@@ -249,7 +244,26 @@ EOM
> + 
> + use Time::Local ();
> + use IPC::Open2 qw(open2);
> ++use OpenBSD::Pledge;
> ++use OpenBSD::Unveil;
> + 
> ++pledge( qw( rpath proc exec prot_exec unveil ) ) || die "Can't pledge: $!";
> ++
> ++# directories
> ++unveil( "/usr/libdata/perl5/", "r" ) || die "Unable to unveil: $!";
> ++unveil( "/cvs/", "r" ) || die "Unable to unveil: $!";
> ++unveil( "/conf/", "r" ) || die "Unable to unveil: $!";
> ++
> ++# files
> ++unveil( "/dev/null", "r" ) || die "Unable to unveil: $!";
> ++unveil( "/usr/bin/rcsdiff", "rx" ) || die "Unable to unveil: $!";
> ++unveil( "/usr/bin/rlog", "rx" ) || die "Unable to unveil: $!";
> ++unveil( "/usr/bin/cvs", "rx" ) || die "Unable to unveil: $!";
> ++unveil( "/usr/bin/uname", "rx" ) || die "Unable to unveil: $!";
> ++
> ++unveil() || die "Unvable to unveil: $!";
> ++
> ++

the proper idiom is :
- unveil() calls first
- pledge() call without "unveil"

you will have almost(*) the same result that what you have, but it is more 
obvious
that unveil(2) is not permitted after pledge(2) call.

(*) almost: with your diff, calling unveil(2) later result in an error (EPERM) ;
whereas without "unveil" in pledge(2) is would result a program terminaison
(pledge violation).



And if you want to avoid patching /dev/null usage, you could add "wpath" in
pledge(2) and unveil("/dev/null", "rw"). The capability to write will be
restricted to only /dev/null with pledge+unveil. so no other write will be
allowed on filesystem.



-- 
Sebastien Marie

Re: add pledge to devel/cvsweb

[Solene Rapenne]

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.

latest patch adding unveil


Index: Makefile
===
RCS file: /data/cvs/ports/devel/cvsweb/Makefile,v
retrieving revision 1.62
diff -u -p -r1.62 Makefile
--- Makefile12 Jul 2019 20:44:07 -  1.62
+++ Makefile27 Sep 2019 07:19:23 -
@@ -3,7 +3,7 @@
 COMMENT=   CGI script to browse CVS repository trees
 
 DISTNAME=  cvsweb-2.0.6
-REVISION=  27
+REVISION=  28
 CATEGORIES=devel www
 HOMEPAGE=  http://www.freebsd.org/projects/cvsweb.html
 
Index: patches/patch-cvsweb_cgi
===
RCS file: /data/cvs/ports/devel/cvsweb/patches/patch-cvsweb_cgi,v
retrieving revision 1.13
diff -u -p -r1.13 patch-cvsweb_cgi
--- patches/patch-cvsweb_cgi7 Apr 2013 20:07:24 -   1.13
+++ patches/patch-cvsweb_cgi27 Sep 2019 07:20:20 -
@@ -1,6 +1,7 @@
 $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/04/07 20:07:24 naddy Exp $
 cvsweb.cgi.origThu Sep 26 22:56:05 2002
-+++ cvsweb.cgi Sun Apr  7 14:15:55 2013
+Index: cvsweb.cgi
+--- cvsweb.cgi.orig
 cvsweb.cgi
 @@ -1,4 +1,4 @@
 -#!/usr/bin/perl -wT
 +#!/usr/bin/perl -w
@@ -37,7 +38,43 @@ $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/0
  );
  
  @LOGSORTKEYS = qw(cvs date rev);
-@@ -2014,20 +2009,6 @@ sub doDiff($$) {
+@@ -249,7 +244,26 @@ EOM
+ 
+ use Time::Local ();
+ use IPC::Open2 qw(open2);
++use OpenBSD::Pledge;
++use OpenBSD::Unveil;
+ 
++pledge( qw( rpath proc exec prot_exec unveil ) ) || die "Can't pledge: $!";
++
++# directories
++unveil( "/usr/libdata/perl5/", "r" ) || die "Unable to unveil: $!";
++unveil( "/cvs/", "r" ) || die "Unable to unveil: $!";
++unveil( "/conf/", "r" ) || die "Unable to unveil: $!";
++
++# files
++unveil( "/dev/null", "r" ) || die "Unable to unveil: $!";
++unveil( "/usr/bin/rcsdiff", "rx" ) || die "Unable to unveil: $!";
++unveil( "/usr/bin/rlog", "rx" ) || die "Unable to unveil: $!";
++unveil( "/usr/bin/cvs", "rx" ) || die "Unable to unveil: $!";
++unveil( "/usr/bin/uname", "rx" ) || die "Unable to unveil: $!";
++
++unveil() || die "Unvable to unveil: $!";
++
++
+ # Check if the zlib C library interface is installed, and if yes
+ # we can avoid using the extra gzip process.
+ eval { require Compress::Zlib; };
+@@ -1578,7 +1592,7 @@ sub openOutputFilter() {
+   open(STDOUT, "|-") and return;
+ 
+   # child of child
+-  open(STDERR, '>/dev/null');
++  #open(STDERR, '>/dev/null');
+   exec($output_filter) or exit -1;
+ }
+ 
+@@ -2014,20 +2028,6 @@ sub doDiff($$) {
my @difftype   = @{$difftype->{'opts'}};
my $human_readable = $difftype->{'colored'};
  
@@ -58,7 +95,24 @@ $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/0
if ($human_readable) {
if ($hr_ignwhite) {
push @difftype, '-w';
-@@ -2658,7 +2639,7 @@ sub printLog($;$) {
+@@ -2128,14 +2128,14 @@ sub getDirLogs($$@) {
+ 
+   #can't use -r as - is allowed in tagnames, but 
misinterpreated by rlog..
+   if (!open($fh, "-|")) {# child
+-  open(STDERR, '>/dev/null'); # rlog may complain; ignore.
++  #open(STDERR, '>/dev/null'); # rlog may complain; 
ignore.
+   openOutputFilter();
+   exec($CMD{rlog}, @files) or exit -1;
+   }
+   } else {
+ 
+   if (!open($fh, "-|")) {# child
+-  open(STDERR, '>/dev/null'); # rlog may complain; ignore.
++  #open(STDERR, '>/dev/null'); # rlog may complain; 
ignore.
+   openOutputFilter();
+   exec($CMD{rlog}, '-r', @files) or exit -1;
+   }
+@@ -2658,7 +2658,7 @@ sub printLog($;$) {
if (/^1\.1\.1\.\d+$/) {
print " (vendor branch)";
}
Index: pkg/README

Re: add pledge to devel/cvsweb

[Otto Moerbeek]

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

> 
> 
> Index: Makefile
> ===
> RCS file: /data/cvs/ports/devel/cvsweb/Makefile,v
> retrieving revision 1.62
> diff -u -p -r1.62 Makefile
> --- Makefile  12 Jul 2019 20:44:07 -  1.62
> +++ Makefile  26 Sep 2019 14:24:53 -
> @@ -3,7 +3,7 @@
>  COMMENT= CGI script to browse CVS repository trees
>  
>  DISTNAME=cvsweb-2.0.6
> -REVISION=27
> +REVISION=28
>  CATEGORIES=  devel www
>  HOMEPAGE=http://www.freebsd.org/projects/cvsweb.html
>  
> Index: patches/patch-cvsweb_cgi
> ===
> RCS file: /data/cvs/ports/devel/cvsweb/patches/patch-cvsweb_cgi,v
> retrieving revision 1.13
> diff -u -p -r1.13 patch-cvsweb_cgi
> --- patches/patch-cvsweb_cgi  7 Apr 2013 20:07:24 -   1.13
> +++ patches/patch-cvsweb_cgi  26 Sep 2019 15:21:46 -
> @@ -1,6 +1,7 @@
>  $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/04/07 20:07:24 naddy Exp $
>  cvsweb.cgi.orig  Thu Sep 26 22:56:05 2002
> -+++ cvsweb.cgi   Sun Apr  7 14:15:55 2013
> +Index: cvsweb.cgi
> +--- cvsweb.cgi.orig
>  cvsweb.cgi
>  @@ -1,4 +1,4 @@
>  -#!/usr/bin/perl -wT
>  +#!/usr/bin/perl -w
> @@ -37,7 +38,27 @@ $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/0
>   );
>   
>   @LOGSORTKEYS = qw(cvs date rev);
> -@@ -2014,20 +2009,6 @@ sub doDiff($$) {
> +@@ -249,7 +244,10 @@ EOM
> + 
> + use Time::Local ();
> + use IPC::Open2 qw(open2);
> ++use OpenBSD::Pledge;
> + 
> ++pledge( qw( rpath proc exec prot_exec ) ) || die "Can't pledge: $!";
> ++
> + # Check if the zlib C library interface is installed, and if yes
> + # we can avoid using the extra gzip process.
> + eval { require Compress::Zlib; };
> +@@ -1578,7 +1576,7 @@ sub openOutputFilter() {
> + open(STDOUT, "|-") and return;
> + 
> + # child of child
> +-open(STDERR, '>/dev/null');
> ++#open(STDERR, '>/dev/null');
> + exec($output_filter) or exit -1;
> + }
> + 
> +@@ -2014,20 +2012,6 @@ sub doDiff($$) {
>   my @difftype   = @{$difftype->{'opts'}};
>   my $human_readable = $difftype->{'colored'};
>   
> @@ -58,7 +79,24 @@ $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/0
>   if ($human_readable) {
>   if ($hr_ignwhite) {
>   push @difftype, '-w';
> -@@ -2658,7 +2639,7 @@ sub printLog($;$) {
> +@@ -2128,14 +2112,14 @@ sub getDirLogs($$@) {
> + 
> + #can't use -r as - is allowed in tagnames, but 
> misinterpreated by rlog..
> + if (!open($fh, "-|")) {# child
> +-open(STDERR, '>/dev/null'); # rlog may complain; ignore.
> ++#open(STDERR, '>/dev/null'); # rlog may complain; 
> ignore.
> + openOutputFilter();
> + exec($CMD{rlog}, @files) or exit -1;
> + }
> + } else {
> + 
> + if (!open($fh, "-|")) {# child
> +-open(STDERR, '>/dev/null'); # rlog may complain; ignore.
> ++#open(STDERR, '>/dev/null'); # rlog may complain; 
> ignore.
> + openOutputFilter();
> + exec($CMD{rlog}, '-r', @files) or exit -1;
> + }
> +@@ -2658,7 +2642,7 @@ sub printLog($;$) {
>   if (/^1\.1\.1\.\d+$/) {
>   print " (vendor branch)";
>   }
> Index: pkg/README
> ===
> RCS file: /data/cvs/ports/devel/cvsweb/pkg/README,v
> retrieving revision 1.18
> diff -u -p -r1.18 README
> --- pkg/README2 May 2019 18:58:38 -   1.18
> +++ pkg/README26 Sep 2019 14:24:47 -
> @@ -22,7 +22,7 @@ cd /var/www/usr
>  mkdir -p bin lib libdata/perl5 libexec
>  
>  cd /var/www/usr/libdata/perl5
> -mkdir -p File IPC Time warnings `arch -s`-openbsd/auto/{Cwd,Fcntl} unicore
> +mkdir -p File IPC Time warnings `arch 
> -s`-openbsd/auto/{Cwd,Fcntl,OpenBSD/Pledge} `arch -s`-openbsd/OpenBSD unicore
>  
>  # The "annotate" function requires this empty file:
>  #
> @@ -72,6 +72,8 @@ cp -p /usr/libdata/perl5/`arch -s`-openb
>  cp -p /usr/libdata/perl5/`arch -s`-openbsd/DynaLoader.pm .
>  cp -p /usr/libdata/perl5/`arch -s`-openbsd/Fcntl.pm .
>  cp -p 

add pledge to devel/cvsweb

[Solene Rapenne]

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.


Index: Makefile
===
RCS file: /data/cvs/ports/devel/cvsweb/Makefile,v
retrieving revision 1.62
diff -u -p -r1.62 Makefile
--- Makefile12 Jul 2019 20:44:07 -  1.62
+++ Makefile26 Sep 2019 14:24:53 -
@@ -3,7 +3,7 @@
 COMMENT=   CGI script to browse CVS repository trees
 
 DISTNAME=  cvsweb-2.0.6
-REVISION=  27
+REVISION=  28
 CATEGORIES=devel www
 HOMEPAGE=  http://www.freebsd.org/projects/cvsweb.html
 
Index: patches/patch-cvsweb_cgi
===
RCS file: /data/cvs/ports/devel/cvsweb/patches/patch-cvsweb_cgi,v
retrieving revision 1.13
diff -u -p -r1.13 patch-cvsweb_cgi
--- patches/patch-cvsweb_cgi7 Apr 2013 20:07:24 -   1.13
+++ patches/patch-cvsweb_cgi26 Sep 2019 15:21:46 -
@@ -1,6 +1,7 @@
 $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/04/07 20:07:24 naddy Exp $
 cvsweb.cgi.origThu Sep 26 22:56:05 2002
-+++ cvsweb.cgi Sun Apr  7 14:15:55 2013
+Index: cvsweb.cgi
+--- cvsweb.cgi.orig
 cvsweb.cgi
 @@ -1,4 +1,4 @@
 -#!/usr/bin/perl -wT
 +#!/usr/bin/perl -w
@@ -37,7 +38,27 @@ $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/0
  );
  
  @LOGSORTKEYS = qw(cvs date rev);
-@@ -2014,20 +2009,6 @@ sub doDiff($$) {
+@@ -249,7 +244,10 @@ EOM
+ 
+ use Time::Local ();
+ use IPC::Open2 qw(open2);
++use OpenBSD::Pledge;
+ 
++pledge( qw( rpath proc exec prot_exec ) ) || die "Can't pledge: $!";
++
+ # Check if the zlib C library interface is installed, and if yes
+ # we can avoid using the extra gzip process.
+ eval { require Compress::Zlib; };
+@@ -1578,7 +1576,7 @@ sub openOutputFilter() {
+   open(STDOUT, "|-") and return;
+ 
+   # child of child
+-  open(STDERR, '>/dev/null');
++  #open(STDERR, '>/dev/null');
+   exec($output_filter) or exit -1;
+ }
+ 
+@@ -2014,20 +2012,6 @@ sub doDiff($$) {
my @difftype   = @{$difftype->{'opts'}};
my $human_readable = $difftype->{'colored'};
  
@@ -58,7 +79,24 @@ $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/0
if ($human_readable) {
if ($hr_ignwhite) {
push @difftype, '-w';
-@@ -2658,7 +2639,7 @@ sub printLog($;$) {
+@@ -2128,14 +2112,14 @@ sub getDirLogs($$@) {
+ 
+   #can't use -r as - is allowed in tagnames, but 
misinterpreated by rlog..
+   if (!open($fh, "-|")) {# child
+-  open(STDERR, '>/dev/null'); # rlog may complain; ignore.
++  #open(STDERR, '>/dev/null'); # rlog may complain; 
ignore.
+   openOutputFilter();
+   exec($CMD{rlog}, @files) or exit -1;
+   }
+   } else {
+ 
+   if (!open($fh, "-|")) {# child
+-  open(STDERR, '>/dev/null'); # rlog may complain; ignore.
++  #open(STDERR, '>/dev/null'); # rlog may complain; 
ignore.
+   openOutputFilter();
+   exec($CMD{rlog}, '-r', @files) or exit -1;
+   }
+@@ -2658,7 +2642,7 @@ sub printLog($;$) {
if (/^1\.1\.1\.\d+$/) {
print " (vendor branch)";
}
Index: pkg/README
===
RCS file: /data/cvs/ports/devel/cvsweb/pkg/README,v
retrieving revision 1.18
diff -u -p -r1.18 README
--- pkg/README  2 May 2019 18:58:38 -   1.18
+++ pkg/README  26 Sep 2019 14:24:47 -
@@ -22,7 +22,7 @@ cd /var/www/usr
 mkdir -p bin lib libdata/perl5 libexec
 
 cd /var/www/usr/libdata/perl5
-mkdir -p File IPC Time warnings `arch -s`-openbsd/auto/{Cwd,Fcntl} unicore
+mkdir -p File IPC Time warnings `arch 
-s`-openbsd/auto/{Cwd,Fcntl,OpenBSD/Pledge} `arch -s`-openbsd/OpenBSD unicore
 
 # The "annotate" function requires this empty file:
 #
@@ -72,6 +72,8 @@ cp -p /usr/libdata/perl5/`arch -s`-openb
 cp -p /usr/libdata/perl5/`arch -s`-openbsd/DynaLoader.pm .
 cp -p /usr/libdata/perl5/`arch -s`-openbsd/Fcntl.pm .
 cp -p /usr/libdata/perl5/`arch -s`-openbsd/auto/Fcntl/Fcntl.so ./auto/Fcntl/
+cp -p /usr/libdata/perl5/`arch -s`-openbsd/OpenBSD/Pledge.pm ./OpenBSD/
+cp -p /usr/libdata/perl5/`arch -s`-openbsd/auto/OpenBSD/Pledge/Pledge.so 
./auto/OpenBSD/Pledge/
 
 # You also need to enable slowcgi(8):