Adding pledge() to www/lynx

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

Adding pledge() to www/lynx

Frederic Cambus
Hi ports@,

This diff adds support for pledge() to the current Lynx version.

Needs 'cpath', 'rpath', 'wpath' for reading/saving configuration and
bookmarks, and to save downloaded files.

Needs 'proc' and 'exec' for mailing printed files and spawning shell.

The other promises are obvious.

Index: Makefile
===================================================================
RCS file: /cvs/ports/www/lynx/Makefile,v
retrieving revision 1.22
diff -u -p -u -p -r1.22 Makefile
--- Makefile 14 Jan 2016 10:45:07 -0000 1.22
+++ Makefile 28 Jan 2016 22:32:56 -0000
@@ -5,7 +5,7 @@ PL = 8
 COMMENT = text web browser
 DISTNAME = lynx${V}dev.${PL}
 PKGNAME = lynx-${V}pl${PL}
-REVISION = 1
+REVISION = 2
 EXTRACT_SUFX = .tar.bz2
 CATEGORIES = www net
 
@@ -16,6 +16,7 @@ MAINTAINER = Frederic Cambus <fred@statd
 # GPLv2 only
 PERMIT_PACKAGE_CDROM = Yes
 
+# uses pledge()
 WANTLIB += c crypto ncurses ssl z
 
 MASTER_SITES = http://invisible-mirror.net/archives/lynx/tarballs/
Index: patches/patch-src_LYMain_c
===================================================================
RCS file: patches/patch-src_LYMain_c
diff -N patches/patch-src_LYMain_c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_LYMain_c 28 Jan 2016 22:32:56 -0000
@@ -0,0 +1,14 @@
+$OpenBSD$
+--- src/LYMain.c.orig Thu Jan 28 17:57:15 2016
++++ src/LYMain.c Wed Jan 20 21:14:26 2016
+@@ -1024,6 +1024,10 @@ int main(int argc,
+     WSADATA WSAData;
+ #endif /* _WINDOWS */
+
++    if (pledge("stdio tty cpath rpath wpath dns inet proc exec", NULL) == -1) {
++ err(EXIT_FAILURE, "pledge");
++    }
++
+     /*
+      * Just in case someone has the idea to install lynx set-uid, let's try
+      * to discourage it.

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Daniel Dickman
On Thu, Jan 28, 2016 at 12:51 PM, Frederic Cambus <[hidden email]> wrote:
> Hi ports@,
>
> This diff adds support for pledge() to the current Lynx version.
>
> +
> ++    if (pledge("stdio tty cpath rpath wpath dns inet proc exec", NULL) == -1) {
> ++      err(EXIT_FAILURE, "pledge");
> ++    }
> ++

i have ioctl in my local patch. is it not needed?

is it possible to get rid of proc exec? I didn't add them on my end...

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Daniel Dickman
On Thu, Jan 28, 2016 at 10:11 PM, Daniel Dickman <[hidden email]> wrote:

> On Thu, Jan 28, 2016 at 12:51 PM, Frederic Cambus <[hidden email]> wrote:
>> Hi ports@,
>>
>> This diff adds support for pledge() to the current Lynx version.
>>
>> +
>> ++    if (pledge("stdio tty cpath rpath wpath dns inet proc exec", NULL) == -1) {
>> ++      err(EXIT_FAILURE, "pledge");
>> ++    }
>> ++
>
> i have ioctl in my local patch. is it not needed?
>
> is it possible to get rid of proc exec? I didn't add them on my end...


Also should it call "err" or "exit_immediately" on failure?

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Theo Buehler
On Thu, Jan 28, 2016 at 10:15:53PM -0500, Daniel Dickman wrote:

> On Thu, Jan 28, 2016 at 10:11 PM, Daniel Dickman <[hidden email]> wrote:
> > On Thu, Jan 28, 2016 at 12:51 PM, Frederic Cambus <[hidden email]> wrote:
> >> Hi ports@,
> >>
> >> This diff adds support for pledge() to the current Lynx version.
> >>
> >> +
> >> ++    if (pledge("stdio tty cpath rpath wpath dns inet proc exec", NULL) == -1) {
> >> ++      err(EXIT_FAILURE, "pledge");
> >> ++    }
> >> ++
> >
> > i have ioctl in my local patch. is it not needed?

probably not.  a quick grep only shows TIOCGWINSZ (which needs pledge
"tty") and FIONBIO ioctl calls.  pledge ioctl in interactive programs is
usually a hint that "tty" was forgotten in earlier attempts with too
tight pledges.

> > is it possible to get rid of proc exec? I didn't add them on my end...

there are shell escapes, so they are probably needed.  I don't really
use lynx myself, but it seems to me that it's worth investigating
tighter pledges conditionally on various "lynx -restriction=..."
options (hopefully those can't be changed at runtime).

> Also should it call "err" or "exit_immediately" on failure?

I agree that the latter looks like the right way to go.

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Frederic Cambus
On Fri, Jan 29, 2016 at 04:37:13AM +0100, Theo Buehler wrote:

> > > is it possible to get rid of proc exec? I didn't add them on my end...
>
> there are shell escapes, so they are probably needed.  I don't really
> use lynx myself, but it seems to me that it's worth investigating
> tighter pledges conditionally on various "lynx -restriction=..."
> options (hopefully those can't be changed at runtime).
>
> > Also should it call "err" or "exit_immediately" on failure?
>
> I agree that the latter looks like the right way to go.

Here is a new diff for testing, with more restrictive promises.

It builds on patches and suggestions provided off-list by tb@ and
daniel@. Thanks guys for all the feedback and ideas.

The idea is to avoid using otherwise required 'getpw', 'proc', 'exec'
promises entirely. We achieve this by disabling a couple of features,
mostly removing obsolete stuff. While we are at it, we attempt to pave
the way to be able to remove even more promises in the future, and
reduce potential attack vectors.

We disable them either at compile time :

--disable-bibp-urls
--disable-dired
--disable-finger

Or by hardcoding boolean values to disable the features just before
calling pledge and entering main program loop :

no_exec = TRUE;
no_mail = TRUE;
no_shell = TRUE;

rlogin_ok = FALSE;
telnet_ok = FALSE;

Manpage has been updated to mention those restrictions.

Also, CFLAGS="-DNOUSERS" was added in the Makefile to disable getpwnam
and getpwuid.

Comments?

Index: Makefile
===================================================================
RCS file: /cvs/ports/www/lynx/Makefile,v
retrieving revision 1.22
diff -u -p -u -p -r1.22 Makefile
--- Makefile 14 Jan 2016 10:45:07 -0000 1.22
+++ Makefile 3 Feb 2016 23:17:38 -0000
@@ -5,7 +5,7 @@ PL = 8
 COMMENT = text web browser
 DISTNAME = lynx${V}dev.${PL}
 PKGNAME = lynx-${V}pl${PL}
-REVISION = 1
+REVISION = 2
 EXTRACT_SUFX = .tar.bz2
 CATEGORIES = www net
 
@@ -16,6 +16,7 @@ MAINTAINER = Frederic Cambus <fred@statd
 # GPLv2 only
 PERMIT_PACKAGE_CDROM = Yes
 
+# uses pledge()
 WANTLIB += c crypto ncurses ssl z
 
 MASTER_SITES = http://invisible-mirror.net/archives/lynx/tarballs/
@@ -24,11 +25,16 @@ CONFIGURE_STYLE = gnu
 CONFIGURE_ARGS = --datarootdir="${PREFIX}/share/doc/lynx" \
  --disable-idna \
  --disable-nls \
+ --disable-bibp-urls \
+ --disable-dired \
+ --disable-finger \
  --enable-default-colors \
  --enable-ipv6 \
  --enable-widec \
  --with-ssl=/usr \
  --with-zlib
+
+CONFIGURE_ENV = CFLAGS="-DNOUSERS"
 
 MAKE_FILE = makefile
 
Index: patches/patch-lynx_man
===================================================================
RCS file: patches/patch-lynx_man
diff -N patches/patch-lynx_man
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-lynx_man 3 Feb 2016 23:17:38 -0000
@@ -0,0 +1,28 @@
+$OpenBSD$
+--- lynx.man.orig Thu Oct  8 02:19:45 2015
++++ lynx.man Wed Feb  3 20:21:11 2016
+@@ -591,8 +591,24 @@ flushes the cache on a proxy server
+ .TP
+ .B \-restrictions\fR=\fI[option][,option][,option]...
+ allows a list of services to be disabled selectively.
++.TP
+ Dashes and underscores in option names can be intermixed.
+ The following list is printed if no options are specified.
++.IP
++On OpenBSD the following restrictions are always enabled:
++\fBexec\fR,
++\fBmail\fR,
++and
++\fBshell\fR.
++Additionally,
++\fBbibp-urls\fR,
++\fBdired\fR,
++\fBfinger\fR,
++\fBrlogin\fR,
++and
++\fBtelnet \fR
++features have been disabled entirely.
++.IP
+ .RS
+ .TP 3
+ .B all
Index: patches/patch-src_LYMain_c
===================================================================
RCS file: patches/patch-src_LYMain_c
diff -N patches/patch-src_LYMain_c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_LYMain_c 3 Feb 2016 23:17:38 -0000
@@ -0,0 +1,25 @@
+$OpenBSD$
+--- src/LYMain.c.orig Fri Dec 18 01:34:45 2015
++++ src/LYMain.c Wed Feb  3 19:50:41 2016
+@@ -2142,6 +2142,21 @@ int main(int argc,
+     }
+
+     /*
++     * Disabling features requiring 'proc' + 'exec' and calling pledge
++     */
++    no_exec = TRUE;
++    no_mail = TRUE;
++    no_shell = TRUE;
++
++    rlogin_ok = FALSE;
++    telnet_ok = FALSE;
++
++    if (pledge("stdio rpath wpath cpath fattr dns inet tty", NULL) == -1) {
++ fprintf(stderr, "%s: pledge: %s\n", getprogname(), strerror(errno));
++ exit_immediately(EXIT_FAILURE);
++    }
++
++    /*
+      * Here's where we do all the work.
+      */
+     if (dump_output_immediately) {
Index: patches/patch-src_LYUtils_c
===================================================================
RCS file: patches/patch-src_LYUtils_c
diff -N patches/patch-src_LYUtils_c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_LYUtils_c 3 Feb 2016 23:17:38 -0000
@@ -0,0 +1,18 @@
+$OpenBSD$
+--- src/LYUtils.c.orig Sun Mar 22 16:38:23 2015
++++ src/LYUtils.c Sun Jan 31 07:49:03 2016
+@@ -5253,10 +5253,11 @@ const char *Home_Dir(void)
+    /*
+     * One could use getlogin() and getpwnam() here instead.
+     */
+-    struct passwd *pw = getpwuid(geteuid());
++    char *home;
+
+-    if (pw && pw->pw_dir) {
+- StrAllocCopy(HomeDir, pw->pw_dir);
++    home = getenv("HOME");
++    if (home && *home) {
++ StrAllocCopy(HomeDir, home);
+    } else
+ #endif
+    {

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Landry Breuil-6
On Wed, Feb 03, 2016 at 08:24:57PM +0100, Frederic Cambus wrote:

> On Fri, Jan 29, 2016 at 04:37:13AM +0100, Theo Buehler wrote:
>
> > > > is it possible to get rid of proc exec? I didn't add them on my end...
> >
> > there are shell escapes, so they are probably needed.  I don't really
> > use lynx myself, but it seems to me that it's worth investigating
> > tighter pledges conditionally on various "lynx -restriction=..."
> > options (hopefully those can't be changed at runtime).
> >
> > > Also should it call "err" or "exit_immediately" on failure?
> >
> > I agree that the latter looks like the right way to go.
>
> Here is a new diff for testing, with more restrictive promises.
>
> It builds on patches and suggestions provided off-list by tb@ and
> daniel@. Thanks guys for all the feedback and ideas.
>
> The idea is to avoid using otherwise required 'getpw', 'proc', 'exec'
> promises entirely. We achieve this by disabling a couple of features,
> mostly removing obsolete stuff. While we are at it, we attempt to pave
> the way to be able to remove even more promises in the future, and
> reduce potential attack vectors.
>
> We disable them either at compile time :
>
> --disable-bibp-urls
> --disable-dired
> --disable-finger

That looks like a wise move to me :)
If you can just add comments to patches explaining why you do the
getenv(HOME) dance to avoid getpw in pledge for the next guy that
stumbles upon it..

Landry

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Theo Buehler
> If you can just add comments to patches explaining why you do the
> getenv(HOME) dance to avoid getpw in pledge for the next guy that
> stumbles upon it..

Since that's my hack, I added a short explanation to the patch itself.
I also added a comment to the Makefile to explain the -DNOUSERS option.

ok?

Index: Makefile
===================================================================
RCS file: /cvs/ports/www/lynx/Makefile,v
retrieving revision 1.22
diff -u -p -r1.22 Makefile
--- Makefile 14 Jan 2016 10:45:07 -0000 1.22
+++ Makefile 4 Feb 2016 11:52:29 -0000
@@ -5,7 +5,7 @@ PL = 8
 COMMENT = text web browser
 DISTNAME = lynx${V}dev.${PL}
 PKGNAME = lynx-${V}pl${PL}
-REVISION = 1
+REVISION = 2
 EXTRACT_SUFX = .tar.bz2
 CATEGORIES = www net
 
@@ -16,6 +16,7 @@ MAINTAINER = Frederic Cambus <fred@statd
 # GPLv2 only
 PERMIT_PACKAGE_CDROM = Yes
 
+# uses pledge()
 WANTLIB += c crypto ncurses ssl z
 
 MASTER_SITES = http://invisible-mirror.net/archives/lynx/tarballs/
@@ -24,11 +25,17 @@ CONFIGURE_STYLE = gnu
 CONFIGURE_ARGS = --datarootdir="${PREFIX}/share/doc/lynx" \
  --disable-idna \
  --disable-nls \
+ --disable-bibp-urls \
+ --disable-dired \
+ --disable-finger \
  --enable-default-colors \
  --enable-ipv6 \
  --enable-widec \
  --with-ssl=/usr \
  --with-zlib
+
+# This disables most calls to getpw*(3) so we can avoid pledge "getpw".
+CONFIGURE_ENV = CFLAGS="-DNOUSERS"
 
 MAKE_FILE = makefile
 
Index: patches/patch-lynx_man
===================================================================
RCS file: patches/patch-lynx_man
diff -N patches/patch-lynx_man
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-lynx_man 4 Feb 2016 11:52:29 -0000
@@ -0,0 +1,25 @@
+$OpenBSD$
+--- lynx.man.orig Thu Oct  8 02:19:45 2015
++++ lynx.man Thu Feb  4 12:37:28 2016
+@@ -593,6 +593,21 @@ flushes the cache on a proxy server
+ allows a list of services to be disabled selectively.
+ Dashes and underscores in option names can be intermixed.
+ The following list is printed if no options are specified.
++.IP
++On OpenBSD the following restrictions are always enabled:
++\fBexec\fR,
++\fBmail\fR,
++and
++\fBshell\fR.
++Additionally,
++\fBbibp-urls\fR,
++\fBdired\fR,
++\fBfinger\fR,
++\fBrlogin\fR,
++and
++\fBtelnet \fR
++features have been disabled entirely.
++.IP
+ .RS
+ .TP 3
+ .B all
Index: patches/patch-src_LYMain_c
===================================================================
RCS file: patches/patch-src_LYMain_c
diff -N patches/patch-src_LYMain_c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_LYMain_c 4 Feb 2016 11:52:29 -0000
@@ -0,0 +1,25 @@
+$OpenBSD$
+--- src/LYMain.c.orig Fri Dec 18 01:34:45 2015
++++ src/LYMain.c Wed Feb  3 19:50:41 2016
+@@ -2142,6 +2142,21 @@ int main(int argc,
+     }
+
+     /*
++     * Disabling features requiring 'proc' + 'exec' and calling pledge
++     */
++    no_exec = TRUE;
++    no_mail = TRUE;
++    no_shell = TRUE;
++
++    rlogin_ok = FALSE;
++    telnet_ok = FALSE;
++
++    if (pledge("stdio rpath wpath cpath fattr dns inet tty", NULL) == -1) {
++ fprintf(stderr, "%s: pledge: %s\n", getprogname(), strerror(errno));
++ exit_immediately(EXIT_FAILURE);
++    }
++
++    /*
+      * Here's where we do all the work.
+      */
+     if (dump_output_immediately) {
Index: patches/patch-src_LYUtils_c
===================================================================
RCS file: patches/patch-src_LYUtils_c
diff -N patches/patch-src_LYUtils_c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_LYUtils_c 4 Feb 2016 11:52:29 -0000
@@ -0,0 +1,24 @@
+$OpenBSD$
+
+Use getenv("HOME") to determine the home directory instead of using getpwuid in
+order to avoid a "getpw" promise.  This is the only location not covered by the
+'-DNOUSERS' option in the Makefile.  If HOME is unset, the fallback is /tmp, so
+no breakage is to be expected from this.
+
+--- src/LYUtils.c.orig Sun Mar 22 16:38:23 2015
++++ src/LYUtils.c Sun Jan 31 07:49:03 2016
+@@ -5253,10 +5253,11 @@ const char *Home_Dir(void)
+    /*
+     * One could use getlogin() and getpwnam() here instead.
+     */
+-    struct passwd *pw = getpwuid(geteuid());
++    char *home;
+
+-    if (pw && pw->pw_dir) {
+- StrAllocCopy(HomeDir, pw->pw_dir);
++    home = getenv("HOME");
++    if (home && *home) {
++ StrAllocCopy(HomeDir, home);
+    } else
+ #endif
+    {

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Stuart Henderson-6
On 2016/02/04 12:53, Theo Buehler wrote:
> > If you can just add comments to patches explaining why you do the
> > getenv(HOME) dance to avoid getpw in pledge for the next guy that
> > stumbles upon it..
>
> Since that's my hack, I added a short explanation to the patch itself.
> I also added a comment to the Makefile to explain the -DNOUSERS option.
>
> ok?

I think we need to take it easy on adding new pledges now until we're
done with 5.9, we aren't going to get wide enough testing in to identify
use cases that we've missed before it's too late to fix them..

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Theo Buehler
In reply to this post by Theo Buehler
On Thu, Feb 04, 2016 at 12:53:56PM +0100, Theo Buehler wrote:
> > If you can just add comments to patches explaining why you do the
> > getenv(HOME) dance to avoid getpw in pledge for the next guy that
> > stumbles upon it..
>
> Since that's my hack, I added a short explanation to the patch itself.
> I also added a comment to the Makefile to explain the -DNOUSERS option.
>
> ok?
>

Here's a new version of the patch. The only change is in the Makefile
due to the recent update of MASTER_SITES.

ok?

Index: Makefile
===================================================================
RCS file: /var/cvs/ports/www/lynx/Makefile,v
retrieving revision 1.23
diff -u -p -r1.23 Makefile
--- Makefile 27 Feb 2016 22:46:10 -0000 1.23
+++ Makefile 1 Mar 2016 07:08:54 -0000
@@ -5,7 +5,7 @@ PL = 8
 COMMENT = text web browser
 DISTNAME = lynx${V}dev.${PL}
 PKGNAME = lynx-${V}pl${PL}
-REVISION = 1
+REVISION = 2
 EXTRACT_SUFX = .tar.bz2
 CATEGORIES = www net
 
@@ -16,6 +16,7 @@ MAINTAINER = Frederic Cambus <fred@statd
 # GPLv2 only
 PERMIT_PACKAGE_CDROM = Yes
 
+# uses pledge()
 WANTLIB += c crypto ncurses ssl z
 
 MASTER_SITES = http://lynx.invisible-island.net/current/ \
@@ -26,11 +27,17 @@ CONFIGURE_STYLE = gnu
 CONFIGURE_ARGS = --datarootdir="${PREFIX}/share/doc/lynx" \
  --disable-idna \
  --disable-nls \
+ --disable-bibp-urls \
+ --disable-dired \
+ --disable-finger \
  --enable-default-colors \
  --enable-ipv6 \
  --enable-widec \
  --with-ssl=/usr \
  --with-zlib
+
+# This disables most calls to getpw*(3) so we can avoid pledge "getpw".
+CONFIGURE_ENV = CFLAGS="-DNOUSERS"
 
 MAKE_FILE = makefile
 
Index: patches/patch-lynx_man
===================================================================
RCS file: patches/patch-lynx_man
diff -N patches/patch-lynx_man
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-lynx_man 4 Feb 2016 11:37:34 -0000
@@ -0,0 +1,25 @@
+$OpenBSD$
+--- lynx.man.orig Thu Oct  8 02:19:45 2015
++++ lynx.man Thu Feb  4 12:37:28 2016
+@@ -593,6 +593,21 @@ flushes the cache on a proxy server
+ allows a list of services to be disabled selectively.
+ Dashes and underscores in option names can be intermixed.
+ The following list is printed if no options are specified.
++.IP
++On OpenBSD the following restrictions are always enabled:
++\fBexec\fR,
++\fBmail\fR,
++and
++\fBshell\fR.
++Additionally,
++\fBbibp-urls\fR,
++\fBdired\fR,
++\fBfinger\fR,
++\fBrlogin\fR,
++and
++\fBtelnet \fR
++features have been disabled entirely.
++.IP
+ .RS
+ .TP 3
+ .B all
Index: patches/patch-src_LYMain_c
===================================================================
RCS file: patches/patch-src_LYMain_c
diff -N patches/patch-src_LYMain_c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_LYMain_c 4 Feb 2016 11:35:38 -0000
@@ -0,0 +1,25 @@
+$OpenBSD$
+--- src/LYMain.c.orig Fri Dec 18 01:34:45 2015
++++ src/LYMain.c Wed Feb  3 19:50:41 2016
+@@ -2142,6 +2142,21 @@ int main(int argc,
+     }
+
+     /*
++     * Disabling features requiring 'proc' + 'exec' and calling pledge
++     */
++    no_exec = TRUE;
++    no_mail = TRUE;
++    no_shell = TRUE;
++
++    rlogin_ok = FALSE;
++    telnet_ok = FALSE;
++
++    if (pledge("stdio rpath wpath cpath fattr dns inet tty", NULL) == -1) {
++ fprintf(stderr, "%s: pledge: %s\n", getprogname(), strerror(errno));
++ exit_immediately(EXIT_FAILURE);
++    }
++
++    /*
+      * Here's where we do all the work.
+      */
+     if (dump_output_immediately) {
Index: patches/patch-src_LYUtils_c
===================================================================
RCS file: patches/patch-src_LYUtils_c
diff -N patches/patch-src_LYUtils_c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_LYUtils_c 4 Feb 2016 11:52:22 -0000
@@ -0,0 +1,24 @@
+$OpenBSD$
+
+Use getenv("HOME") to determine the home directory instead of using getpwuid in
+order to avoid a "getpw" promise.  This is the only location not covered by the
+'-DNOUSERS' option in the Makefile.  If HOME is unset, the fallback is /tmp, so
+no breakage is to be expected from this.
+
+--- src/LYUtils.c.orig Sun Mar 22 16:38:23 2015
++++ src/LYUtils.c Sun Jan 31 07:49:03 2016
+@@ -5253,10 +5253,11 @@ const char *Home_Dir(void)
+    /*
+     * One could use getlogin() and getpwnam() here instead.
+     */
+-    struct passwd *pw = getpwuid(geteuid());
++    char *home;
+
+-    if (pw && pw->pw_dir) {
+- StrAllocCopy(HomeDir, pw->pw_dir);
++    home = getenv("HOME");
++    if (home && *home) {
++ StrAllocCopy(HomeDir, home);
+    } else
+ #endif
+    {

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Theo Buehler
On Tue, Mar 01, 2016 at 08:40:10AM +0100, Theo Buehler wrote:

> On Thu, Feb 04, 2016 at 12:53:56PM +0100, Theo Buehler wrote:
> > > If you can just add comments to patches explaining why you do the
> > > getenv(HOME) dance to avoid getpw in pledge for the next guy that
> > > stumbles upon it..
> >
> > Since that's my hack, I added a short explanation to the patch itself.
> > I also added a comment to the Makefile to explain the -DNOUSERS option.
> >
> > ok?
> >
>
> Here's a new version of the patch. The only change is in the Makefile
> due to the recent update of MASTER_SITES.
>
> ok?

ping?

>
> Index: Makefile
> ===================================================================
> RCS file: /var/cvs/ports/www/lynx/Makefile,v
> retrieving revision 1.23
> diff -u -p -r1.23 Makefile
> --- Makefile 27 Feb 2016 22:46:10 -0000 1.23
> +++ Makefile 1 Mar 2016 07:08:54 -0000
> @@ -5,7 +5,7 @@ PL = 8
>  COMMENT = text web browser
>  DISTNAME = lynx${V}dev.${PL}
>  PKGNAME = lynx-${V}pl${PL}
> -REVISION = 1
> +REVISION = 2
>  EXTRACT_SUFX = .tar.bz2
>  CATEGORIES = www net
>  
> @@ -16,6 +16,7 @@ MAINTAINER = Frederic Cambus <fred@statd
>  # GPLv2 only
>  PERMIT_PACKAGE_CDROM = Yes
>  
> +# uses pledge()
>  WANTLIB += c crypto ncurses ssl z
>  
>  MASTER_SITES = http://lynx.invisible-island.net/current/ \
> @@ -26,11 +27,17 @@ CONFIGURE_STYLE = gnu
>  CONFIGURE_ARGS = --datarootdir="${PREFIX}/share/doc/lynx" \
>   --disable-idna \
>   --disable-nls \
> + --disable-bibp-urls \
> + --disable-dired \
> + --disable-finger \
>   --enable-default-colors \
>   --enable-ipv6 \
>   --enable-widec \
>   --with-ssl=/usr \
>   --with-zlib
> +
> +# This disables most calls to getpw*(3) so we can avoid pledge "getpw".
> +CONFIGURE_ENV = CFLAGS="-DNOUSERS"
>  
>  MAKE_FILE = makefile
>  
> Index: patches/patch-lynx_man
> ===================================================================
> RCS file: patches/patch-lynx_man
> diff -N patches/patch-lynx_man
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ patches/patch-lynx_man 4 Feb 2016 11:37:34 -0000
> @@ -0,0 +1,25 @@
> +$OpenBSD$
> +--- lynx.man.orig Thu Oct  8 02:19:45 2015
> ++++ lynx.man Thu Feb  4 12:37:28 2016
> +@@ -593,6 +593,21 @@ flushes the cache on a proxy server
> + allows a list of services to be disabled selectively.
> + Dashes and underscores in option names can be intermixed.
> + The following list is printed if no options are specified.
> ++.IP
> ++On OpenBSD the following restrictions are always enabled:
> ++\fBexec\fR,
> ++\fBmail\fR,
> ++and
> ++\fBshell\fR.
> ++Additionally,
> ++\fBbibp-urls\fR,
> ++\fBdired\fR,
> ++\fBfinger\fR,
> ++\fBrlogin\fR,
> ++and
> ++\fBtelnet \fR
> ++features have been disabled entirely.
> ++.IP
> + .RS
> + .TP 3
> + .B all
> Index: patches/patch-src_LYMain_c
> ===================================================================
> RCS file: patches/patch-src_LYMain_c
> diff -N patches/patch-src_LYMain_c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ patches/patch-src_LYMain_c 4 Feb 2016 11:35:38 -0000
> @@ -0,0 +1,25 @@
> +$OpenBSD$
> +--- src/LYMain.c.orig Fri Dec 18 01:34:45 2015
> ++++ src/LYMain.c Wed Feb  3 19:50:41 2016
> +@@ -2142,6 +2142,21 @@ int main(int argc,
> +     }
> +
> +     /*
> ++     * Disabling features requiring 'proc' + 'exec' and calling pledge
> ++     */
> ++    no_exec = TRUE;
> ++    no_mail = TRUE;
> ++    no_shell = TRUE;
> ++
> ++    rlogin_ok = FALSE;
> ++    telnet_ok = FALSE;
> ++
> ++    if (pledge("stdio rpath wpath cpath fattr dns inet tty", NULL) == -1) {
> ++ fprintf(stderr, "%s: pledge: %s\n", getprogname(), strerror(errno));
> ++ exit_immediately(EXIT_FAILURE);
> ++    }
> ++
> ++    /*
> +      * Here's where we do all the work.
> +      */
> +     if (dump_output_immediately) {
> Index: patches/patch-src_LYUtils_c
> ===================================================================
> RCS file: patches/patch-src_LYUtils_c
> diff -N patches/patch-src_LYUtils_c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ patches/patch-src_LYUtils_c 4 Feb 2016 11:52:22 -0000
> @@ -0,0 +1,24 @@
> +$OpenBSD$
> +
> +Use getenv("HOME") to determine the home directory instead of using getpwuid in
> +order to avoid a "getpw" promise.  This is the only location not covered by the
> +'-DNOUSERS' option in the Makefile.  If HOME is unset, the fallback is /tmp, so
> +no breakage is to be expected from this.
> +
> +--- src/LYUtils.c.orig Sun Mar 22 16:38:23 2015
> ++++ src/LYUtils.c Sun Jan 31 07:49:03 2016
> +@@ -5253,10 +5253,11 @@ const char *Home_Dir(void)
> +    /*
> +     * One could use getlogin() and getpwnam() here instead.
> +     */
> +-    struct passwd *pw = getpwuid(geteuid());
> ++    char *home;
> +
> +-    if (pw && pw->pw_dir) {
> +- StrAllocCopy(HomeDir, pw->pw_dir);
> ++    home = getenv("HOME");
> ++    if (home && *home) {
> ++ StrAllocCopy(HomeDir, home);
> +    } else
> + #endif
> +    {
>

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Stuart Henderson
I've run into a slight problem with the lynx pledging. If there's a
~/.mailcap entry for a mimetype, lynx uses it to try and display the
file, for example I have

application/pdf;        mutt_bgrun mupdf '%s'; nametemplate=%s.pdf

(mutt_bgrun is the old script from http://www.spocom.com/users/gjohnson/mutt/mutt_bgrun)
so with this, following a link that ends up in a pdf results in lynx
being killed by pledge.

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Theo de Raadt
> I've run into a slight problem with the lynx pledging. If there's a
> ~/.mailcap entry for a mimetype, lynx uses it to try and display the
> file, for example I have
>
> application/pdf;        mutt_bgrun mupdf '%s'; nametemplate=%s.pdf
>
> (mutt_bgrun is the old script from http://www.spocom.com/users/gjohnson/mutt/mutt_bgrun)
> so with this, following a link that ends up in a pdf results in lynx
> being killed by pledge.

Same overreach we see elsewhere -- where every program can do
anything, everyone forgets it can do so, and the features become
indistinguishable from attack surface...

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Frederic Cambus
In reply to this post by Stuart Henderson
On Tue, Mar 29, 2016 at 04:32:40PM +0100, Stuart Henderson wrote:

> I've run into a slight problem with the lynx pledging. If there's a
> ~/.mailcap entry for a mimetype, lynx uses it to try and display the
> file, for example I have
>
> application/pdf;        mutt_bgrun mupdf '%s'; nametemplate=%s.pdf
>
> (mutt_bgrun is the old script from http://www.spocom.com/users/gjohnson/mutt/mutt_bgrun)
> so with this, following a link that ends up in a pdf results in lynx
> being killed by pledge.
>

Indeed, I was able to reproduce the issue locally, and that's something
I initially missed. It doesn't seem to be possible to disable this
behavior using an hardcoded boolean like in the previous patches.

What we could do is return early in the LYSystem function so that no
external command is ever called. It's not perfect as in the case you
mentioned, there is no error message reported and lynx doesn't offer
to save the file on disk. But that would also prevent future unpleasant
discoveries like this one, as while investigating, I found another
possible case of 'proc' + 'exec' use : it's possible to define an
external editor which will be executed if you press the 'e' key when
invoking lynx with a local file as argument, for example:
lynx index.html

Here is a patch hardcoding 'no_editor = TRUE' before calling pledge
so calling the editor is disabled with a proper error message, and
returning early in LYSystem. It might be better to return an error
code different than zero, in order to possibly trigger error
messages display?

Index: Makefile
===================================================================
RCS file: /cvs/ports/www/lynx/Makefile,v
retrieving revision 1.24
diff -u -p -u -p -r1.24 Makefile
--- Makefile 12 Mar 2016 14:29:13 -0000 1.24
+++ Makefile 13 Apr 2016 10:40:36 -0000
@@ -5,7 +5,7 @@ PL = 8
 COMMENT = text web browser
 DISTNAME = lynx${V}dev.${PL}
 PKGNAME = lynx-${V}pl${PL}
-REVISION = 2
+REVISION = 3
 EXTRACT_SUFX = .tar.bz2
 CATEGORIES = www net
 
Index: patches/patch-lynx_man
===================================================================
RCS file: /cvs/ports/www/lynx/patches/patch-lynx_man,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 patch-lynx_man
--- patches/patch-lynx_man 12 Mar 2016 14:29:13 -0000 1.1
+++ patches/patch-lynx_man 13 Apr 2016 10:40:36 -0000
@@ -1,12 +1,13 @@
 $OpenBSD: patch-lynx_man,v 1.1 2016/03/12 14:29:13 tb Exp $
 --- lynx.man.orig Thu Oct  8 02:19:45 2015
-+++ lynx.man Thu Feb  4 12:37:28 2016
-@@ -593,6 +593,21 @@ flushes the cache on a proxy server
++++ lynx.man Wed Apr 13 13:03:58 2016
+@@ -593,6 +593,22 @@ flushes the cache on a proxy server
  allows a list of services to be disabled selectively.
  Dashes and underscores in option names can be intermixed.
  The following list is printed if no options are specified.
 +.IP
 +On OpenBSD the following restrictions are always enabled:
++\fBeditor\fR,
 +\fBexec\fR,
 +\fBmail\fR,
 +and
Index: patches/patch-src_LYMain_c
===================================================================
RCS file: /cvs/ports/www/lynx/patches/patch-src_LYMain_c,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 patch-src_LYMain_c
--- patches/patch-src_LYMain_c 12 Mar 2016 14:29:13 -0000 1.1
+++ patches/patch-src_LYMain_c 13 Apr 2016 10:40:36 -0000
@@ -1,12 +1,13 @@
 $OpenBSD: patch-src_LYMain_c,v 1.1 2016/03/12 14:29:13 tb Exp $
 --- src/LYMain.c.orig Fri Dec 18 01:34:45 2015
-+++ src/LYMain.c Wed Feb  3 19:50:41 2016
-@@ -2142,6 +2142,21 @@ int main(int argc,
++++ src/LYMain.c Mon Apr 11 01:55:21 2016
+@@ -2142,6 +2142,22 @@ int main(int argc,
      }
 
      /*
 +     * Disabling features requiring 'proc' + 'exec' and calling pledge
 +     */
++    no_editor = TRUE;
 +    no_exec = TRUE;
 +    no_mail = TRUE;
 +    no_shell = TRUE;
Index: patches/patch-src_LYUtils_c
===================================================================
RCS file: /cvs/ports/www/lynx/patches/patch-src_LYUtils_c,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 patch-src_LYUtils_c
--- patches/patch-src_LYUtils_c 12 Mar 2016 14:29:13 -0000 1.1
+++ patches/patch-src_LYUtils_c 13 Apr 2016 10:40:36 -0000
@@ -6,7 +6,7 @@ order to avoid a "getpw" promise.  This
 no breakage is to be expected from this.
 
 --- src/LYUtils.c.orig Sun Mar 22 16:38:23 2015
-+++ src/LYUtils.c Sun Jan 31 07:49:03 2016
++++ src/LYUtils.c Mon Apr 11 01:39:12 2016
 @@ -5253,10 +5253,11 @@ const char *Home_Dir(void)
     /*
      * One could use getlogin() and getpwnam() here instead.
@@ -22,3 +22,11 @@ no breakage is to be expected from this.
     } else
  #endif
     {
+@@ -7185,6 +7186,7 @@ static char *escape_backslashes(char *source)
+  */
+ int LYSystem(char *command)
+ {
++    return 0;
+     int code;
+     int do_free = 0;
+

 

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Frederic Cambus
On Thu, Apr 14, 2016 at 12:28:24AM +0200, Theo Buehler wrote:
>
> Here's a cleaner version that makes no unnecessary change to lynx.cfg.
>
> I'm going to commit this version tomorrow.

That makes sense to me, and I like tb@ approach it's much cleaner
indeed. However, it's still possible to override the hardcoded default
by setting GLOBAL_MAILCAP and PERSONAL_MAILCAP in /etc/lynx.cfg.

This is an updated patch which removes mailcap configuration parsing.

As per deraadt@ suggestion, I will contact upstream about those issues.

Index: Makefile
===================================================================
RCS file: /cvs/ports/www/lynx/Makefile,v
retrieving revision 1.24
diff -u -p -u -p -r1.24 Makefile
--- Makefile 12 Mar 2016 14:29:13 -0000 1.24
+++ Makefile 14 Apr 2016 20:35:41 -0000
@@ -5,7 +5,7 @@ PL = 8
 COMMENT = text web browser
 DISTNAME = lynx${V}dev.${PL}
 PKGNAME = lynx-${V}pl${PL}
-REVISION = 2
+REVISION = 3
 EXTRACT_SUFX = .tar.bz2
 CATEGORIES = www net
 
Index: patches/patch-lynx_cfg
===================================================================
RCS file: /cvs/ports/www/lynx/patches/patch-lynx_cfg,v
retrieving revision 1.2
diff -u -p -u -p -r1.2 patch-lynx_cfg
--- patches/patch-lynx_cfg 12 Jan 2016 16:05:54 -0000 1.2
+++ patches/patch-lynx_cfg 14 Apr 2016 20:35:41 -0000
@@ -1,6 +1,6 @@
 $OpenBSD: patch-lynx_cfg,v 1.2 2016/01/12 16:05:54 dcoppa Exp $
---- lynx.cfg.orig Tue Jan 12 00:09:37 2016
-+++ lynx.cfg Tue Jan 12 00:09:57 2016
+--- lynx.cfg.orig Tue Dec 22 02:45:35 2015
++++ lynx.cfg Wed Apr 13 23:26:29 2016
 @@ -90,7 +90,7 @@
  #
  # Normally we expect you will connect to a remote site, e.g., the Lynx starting
Index: patches/patch-lynx_man
===================================================================
RCS file: /cvs/ports/www/lynx/patches/patch-lynx_man,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 patch-lynx_man
--- patches/patch-lynx_man 12 Mar 2016 14:29:13 -0000 1.1
+++ patches/patch-lynx_man 14 Apr 2016 20:35:41 -0000
@@ -1,12 +1,13 @@
 $OpenBSD: patch-lynx_man,v 1.1 2016/03/12 14:29:13 tb Exp $
 --- lynx.man.orig Thu Oct  8 02:19:45 2015
-+++ lynx.man Thu Feb  4 12:37:28 2016
-@@ -593,6 +593,21 @@ flushes the cache on a proxy server
++++ lynx.man Wed Apr 13 13:03:58 2016
+@@ -593,6 +593,22 @@ flushes the cache on a proxy server
  allows a list of services to be disabled selectively.
  Dashes and underscores in option names can be intermixed.
  The following list is printed if no options are specified.
 +.IP
 +On OpenBSD the following restrictions are always enabled:
++\fBeditor\fR,
 +\fBexec\fR,
 +\fBmail\fR,
 +and
Index: patches/patch-src_LYMain_c
===================================================================
RCS file: /cvs/ports/www/lynx/patches/patch-src_LYMain_c,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 patch-src_LYMain_c
--- patches/patch-src_LYMain_c 12 Mar 2016 14:29:13 -0000 1.1
+++ patches/patch-src_LYMain_c 14 Apr 2016 20:35:41 -0000
@@ -1,12 +1,13 @@
 $OpenBSD: patch-src_LYMain_c,v 1.1 2016/03/12 14:29:13 tb Exp $
 --- src/LYMain.c.orig Fri Dec 18 01:34:45 2015
-+++ src/LYMain.c Wed Feb  3 19:50:41 2016
-@@ -2142,6 +2142,21 @@ int main(int argc,
++++ src/LYMain.c Mon Apr 11 01:55:21 2016
+@@ -2142,6 +2142,22 @@ int main(int argc,
      }
 
      /*
 +     * Disabling features requiring 'proc' + 'exec' and calling pledge
 +     */
++    no_editor = TRUE;
 +    no_exec = TRUE;
 +    no_mail = TRUE;
 +    no_shell = TRUE;
Index: patches/patch-src_LYReadCFG_c
===================================================================
RCS file: patches/patch-src_LYReadCFG_c
diff -N patches/patch-src_LYReadCFG_c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_LYReadCFG_c 14 Apr 2016 20:35:41 -0000
@@ -0,0 +1,19 @@
+$OpenBSD$
+--- src/LYReadCFG.c.orig Wed Apr 13 16:48:57 2016
++++ src/LYReadCFG.c Wed Apr 13 16:49:39 2016
+@@ -1544,7 +1544,6 @@ static Config_Type Config_Table [] =
+ #endif
+      PARSE_Env(RC_FTP_PROXY,            0),
+      PARSE_STR(RC_GLOBAL_EXTENSION_MAP, global_extension_map),
+-     PARSE_STR(RC_GLOBAL_MAILCAP,       global_type_map),
+      PARSE_Env(RC_GOPHER_PROXY,         0),
+      PARSE_SET(RC_GOTOBUFFER,           goto_buffer),
+      PARSE_PRG(RC_GZIP_PATH,            ppGZIP),
+@@ -1662,7 +1661,6 @@ static Config_Type Config_Table [] =
+      PARSE_SET(RC_PERSISTENT_COOKIES,   persistent_cookies),
+ #endif /* USE_PERSISTENT_COOKIES */
+      PARSE_STR(RC_PERSONAL_EXTENSION_MAP, personal_extension_map),
+-     PARSE_STR(RC_PERSONAL_MAILCAP,     personal_type_map),
+      PARSE_LST(RC_POSITIONABLE_EDITOR,  positionable_editor),
+      PARSE_STR(RC_PREFERRED_CHARSET,    pref_charset),
+      PARSE_ENU(RC_PREFERRED_ENCODING,   LYAcceptEncoding, tbl_preferred_encoding),
Index: patches/patch-userdefs_h
===================================================================
RCS file: patches/patch-userdefs_h
diff -N patches/patch-userdefs_h
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-userdefs_h 14 Apr 2016 20:35:41 -0000
@@ -0,0 +1,25 @@
+$OpenBSD$
+--- userdefs.h.orig Tue Dec 22 02:45:35 2015
++++ userdefs.h Thu Apr 14 00:11:57 2016
+@@ -129,8 +129,8 @@
+  * Mappings in these global and personal files override any VIEWER
+  * definitions in lynx.cfg and built-in defaults from src/HTInit.c.
+  */
+-#define GLOBAL_MAILCAP "Lynx_Dir:mailcap"
+-#define PERSONAL_MAILCAP ".mailcap"
++#define GLOBAL_MAILCAP "/dev/null"
++#define PERSONAL_MAILCAP "/dev/null"
+
+ /**************************
+  * XLOADIMAGE_COMMAND will be used as a default in src/HTInit.c
+@@ -327,8 +327,8 @@
+  * Mappings in these global and personal files override any VIEWER
+  * definitions in lynx.cfg and built-in defaults from src/HTInit.c.
+  */
+-#define GLOBAL_MAILCAP MIME_LIBDIR "mailcap"
+-#define PERSONAL_MAILCAP "~/.mailcap"
++#define GLOBAL_MAILCAP "/dev/null"
++#define PERSONAL_MAILCAP "/dev/null"
+
+ /**************************
+  * XLOADIMAGE_COMMAND will be used as a default in src/HTInit.c for

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Stuart Henderson
In reply to this post by Frederic Cambus
On 2016/04/13 13:50, Frederic Cambus wrote:

> On Tue, Mar 29, 2016 at 04:32:40PM +0100, Stuart Henderson wrote:
> > I've run into a slight problem with the lynx pledging. If there's a
> > ~/.mailcap entry for a mimetype, lynx uses it to try and display the
> > file, for example I have
> >
> > application/pdf;        mutt_bgrun mupdf '%s'; nametemplate=%s.pdf
> >
> > (mutt_bgrun is the old script from http://www.spocom.com/users/gjohnson/mutt/mutt_bgrun)
> > so with this, following a link that ends up in a pdf results in lynx
> > being killed by pledge.
> >
>
> Indeed, I was able to reproduce the issue locally, and that's something
> I initially missed. It doesn't seem to be possible to disable this
> behavior using an hardcoded boolean like in the previous patches.
>
> What we could do is return early in the LYSystem function so that no
> external command is ever called. It's not perfect as in the case you
> mentioned, there is no error message reported and lynx doesn't offer
> to save the file on disk.

I'd actually prefer the crash over this, because it shows clearly that
there is something we need to handle, and points to the area of code
that's needed.

I didn't look at the code but might it be an option to just avoid
reading mailcap (pretend that the file doesn't exist) ..?

> Here is a patch hardcoding 'no_editor = TRUE' before calling pledge
> so calling the editor is disabled with a proper error message

That makes sense.

In decreasing order of preference:

- fallback behaviour (e.g. download for unhandled mime-types)
- nice error
- hard crash
- silently don't do what is supposed to happen

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Theo de Raadt-2
In reply to this post by Frederic Cambus
>On 2016/04/13 13:50, Frederic Cambus wrote:
>> On Tue, Mar 29, 2016 at 04:32:40PM +0100, Stuart Henderson wrote:
>> > I've run into a slight problem with the lynx pledging. If there's a
>> > ~/.mailcap entry for a mimetype, lynx uses it to try and display the
>> > file, for example I have
>> >
>> > application/pdf;        mutt_bgrun mupdf '%s'; nametemplate=%s.pdf
>> >
>> > (mutt_bgrun is the old script from http://www.spocom.com/users/gjohnson/mutt/mutt_bgrun)
>> > so with this, following a link that ends up in a pdf results in lynx
>> > being killed by pledge.
>> >
>>
>> Indeed, I was able to reproduce the issue locally, and that's something
>> I initially missed. It doesn't seem to be possible to disable this
>> behavior using an hardcoded boolean like in the previous patches.
>>
>> What we could do is return early in the LYSystem function so that no
>> external command is ever called. It's not perfect as in the case you
>> mentioned, there is no error message reported and lynx doesn't offer
>> to save the file on disk.
>
>I'd actually prefer the crash over this, because it shows clearly that
>there is something we need to handle, and points to the area of code
>that's needed.
>
>I didn't look at the code but might it be an option to just avoid
>reading mailcap (pretend that the file doesn't exist) ..?
>
>> Here is a patch hardcoding 'no_editor = TRUE' before calling pledge
>> so calling the editor is disabled with a proper error message
>
>That makes sense.
>
>In decreasing order of preference:
>
>- fallback behaviour (e.g. download for unhandled mime-types)
>- nice error
>- hard crash
>- silently don't do what is supposed to happen

Let's start from the beginning.

How do the lynx authors justify the risk of a coding error
in the surrounding code?

Keeping it simple, I bet it never even entered their thought
process.

How about a patch to REMOVE the behavior.  Then submit it upstream,
and simultaneously challenge the greater community to find a vuln
related to this bizzare feature someone added eons ago?

Eventually we must push back against software that does too-much,
because too-much is very much related to unconsidered risk.

A year ago when we were discussing removal of lynx from base we
received comments from folk who considered it safe.  Did they know
about these mimetypes?  Sorry, this "OpenBSD makes crap software
safe" meme is so tiring.

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Theo Buehler
In reply to this post by Frederic Cambus
On Wed, Apr 13, 2016 at 05:55:51PM +0200, Frederic Cambus wrote:
> On Thu, Apr 14, 2016 at 12:28:24AM +0200, Theo Buehler wrote:
> >
> > Here's a cleaner version that makes no unnecessary change to lynx.cfg.
> >
> > I'm going to commit this version tomorrow.
>
> That makes sense to me, and I like tb@ approach it's much cleaner
> indeed. However, it's still possible to override the hardcoded default
> by setting GLOBAL_MAILCAP and PERSONAL_MAILCAP in /etc/lynx.cfg.

good catch

> This is an updated patch which removes mailcap configuration parsing.

Since this patch is obviously better than the previous version, I
committed this one.

> As per deraadt@ suggestion, I will contact upstream about those issues.

Excellent, thanks!

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Frederic Cambus
On Fri, Apr 15, 2016 at 05:27:28AM +0200, Theo Buehler wrote:

> > That makes sense to me, and I like tb@ approach it's much cleaner
> > indeed. However, it's still possible to override the hardcoded default
> > by setting GLOBAL_MAILCAP and PERSONAL_MAILCAP in /etc/lynx.cfg.
>
> good catch
>
> > This is an updated patch which removes mailcap configuration parsing.
>
> Since this patch is obviously better than the previous version, I
> committed this one.

I received a bug report mentioning that invoking "lynx drudgereport"
crashed the program, and found out that Lynx sends an 'Accept-Encoding'
header saying it can handle bzip2 compressed HTML, and attempts to
uncompress the content by calling the bzip2 binary. It gets killed
by pledge when attempting that.

This can be mitigated by linking against bzlib, so the library will be
used instead of calling the bzip2 binary directly. I attached a patch
doing this.

Index: Makefile
===================================================================
RCS file: /cvs/ports/www/lynx/Makefile,v
retrieving revision 1.25
diff -u -p -u -p -r1.25 Makefile
--- Makefile 15 Apr 2016 03:21:51 -0000 1.25
+++ Makefile 22 Apr 2016 14:23:34 -0000
@@ -5,7 +5,7 @@ PL = 8
 COMMENT = text web browser
 DISTNAME = lynx${V}dev.${PL}
 PKGNAME = lynx-${V}pl${PL}
-REVISION = 3
+REVISION = 4
 EXTRACT_SUFX = .tar.bz2
 CATEGORIES = www net
 
@@ -17,12 +17,14 @@ MAINTAINER = Frederic Cambus <fred@statd
 PERMIT_PACKAGE_CDROM = Yes
 
 # uses pledge()
-WANTLIB += c crypto ncurses ssl z
+WANTLIB += bz2 c crypto ncurses ssl z
 
 MASTER_SITES = http://lynx.invisible-island.net/current/ \
  http://invisible-mirror.net/archives/lynx/tarballs/ \
  http://invisible-mirror.net/archives/lynx/patches/
 
+LIB_DEPENDS = archivers/bzip2
+
 CONFIGURE_STYLE = gnu
 CONFIGURE_ARGS = --datarootdir="${PREFIX}/share/doc/lynx" \
  --disable-idna \
@@ -34,7 +36,8 @@ CONFIGURE_ARGS = --datarootdir="${PREFIX
  --enable-ipv6 \
  --enable-widec \
  --with-ssl=/usr \
- --with-zlib
+ --with-zlib \
+ --with-bzlib
 
 # This disables most calls to getpw*(3) so we can avoid pledge "getpw".
 CONFIGURE_ENV = CFLAGS="-DNOUSERS"

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Dmitrij D. Czarkoff-2
Frederic Cambus said:
> I received a bug report mentioning that invoking "lynx drudgereport"
> crashed the program, and found out that Lynx sends an 'Accept-Encoding'
> header saying it can handle bzip2 compressed HTML, and attempts to
> uncompress the content by calling the bzip2 binary. It gets killed
> by pledge when attempting that.

You get similar results when trying to follow a link to an image in Lynx.

--
Dmitrij D. Czarkoff

Reply | Threaded
Open this post in threaded view
|

Re: Adding pledge() to www/lynx

Frederic Cambus
On Mon, Apr 25, 2016 at 01:13:09AM +0200, Dmitrij D. Czarkoff wrote:

> > I received a bug report mentioning that invoking "lynx drudgereport"
> > crashed the program, and found out that Lynx sends an 'Accept-Encoding'
> > header saying it can handle bzip2 compressed HTML, and attempts to
> > uncompress the content by calling the bzip2 binary. It gets killed
> > by pledge when attempting that.
>
> You get similar results when trying to follow a link to an image in Lynx.

I cannot reproduce this locally, are you using p3 or later? I believe
this is the same kind of issue happening with a .mailcap file which
was reported by sthen@ and fixed in p3.