unveil bdftopcf

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

unveil bdftopcf

Ricardo Mestre-2
Hi,

If input_name is provided we can unveil it with read permissions, if
output_name is provided we need to unveil this one with rwc. Additionally
depending on the different combinations of if these files are passed via args
or from stdin/to stdout we can also pledge accordingly to the code path. This
has been tested succefully with bdf fonts we have bundled in xenocara.

Since I have several other X apps unveiled and/or pledged could you please
comment not only with the unveil/pledge part, but also err vs fprintf/exit, the
placement of the #includes and also tabs vs spaces?

Index: bdftopcf.c
===================================================================
RCS file: /cvs/xenocara/app/bdftopcf/bdftopcf.c,v
retrieving revision 1.5
diff -u -p -u -r1.5 bdftopcf.c
--- bdftopcf.c 29 Mar 2018 20:34:30 -0000 1.5
+++ bdftopcf.c 24 Oct 2018 10:15:41 -0000
@@ -38,7 +38,9 @@ from The Open Group.
 #include "fntfil.h"
 #include "bdfint.h"
 #include "pcf.h"
+#include <err.h>
 #include <stdio.h>
+#include <unistd.h>
 #include <X11/Xos.h>
 
 int
@@ -158,6 +160,26 @@ main(int argc, char *argv[])
         }
         argv++;
     }
+
+ if (input_name) {
+ if (unveil(input_name, "r") == -1)
+ err(1, "unveil");
+ }
+ if (output_name) {
+ if (unveil(output_name, "rwc") == -1)
+ err(1, "unveil");
+ if (pledge("stdio rpath wpath cpath", NULL) == -1)
+ err(1, "pledge");
+ }
+ if (input_name && !output_name) {
+ if (pledge("stdio rpath", NULL) == -1)
+ err(1, "pledge");
+ }
+ if (!input_name && !output_name) {
+ if (pledge("stdio", NULL) == -1)
+ err(1, "pledge");
+ }
+
     if (input_name) {
         input = FontFileOpen(input_name);
         if (!input) {

Reply | Threaded
Open this post in threaded view
|

Re: unveil bdftopcf

Theo de Raadt-2
bdftopcf is intended to be portable code.  I don't think it is
right to start using <err.h> functions in here.  They are within
an unveil-block which we'll carry as a diff, but still.. it doesn't
feel right.  I think you should use fprintf to stderr and exit as
the existing code does.

> If input_name is provided we can unveil it with read permissions, if
> output_name is provided we need to unveil this one with rwc. Additionally
> depending on the different combinations of if these files are passed via args
> or from stdin/to stdout we can also pledge accordingly to the code path. This
> has been tested succefully with bdf fonts we have bundled in xenocara.
>
> Since I have several other X apps unveiled and/or pledged could you please
> comment not only with the unveil/pledge part, but also err vs fprintf/exit, the
> placement of the #includes and also tabs vs spaces?
>
> Index: bdftopcf.c
> ===================================================================
> RCS file: /cvs/xenocara/app/bdftopcf/bdftopcf.c,v
> retrieving revision 1.5
> diff -u -p -u -r1.5 bdftopcf.c
> --- bdftopcf.c 29 Mar 2018 20:34:30 -0000 1.5
> +++ bdftopcf.c 24 Oct 2018 10:15:41 -0000
> @@ -38,7 +38,9 @@ from The Open Group.
>  #include "fntfil.h"
>  #include "bdfint.h"
>  #include "pcf.h"
> +#include <err.h>
>  #include <stdio.h>
> +#include <unistd.h>
>  #include <X11/Xos.h>
>  
>  int
> @@ -158,6 +160,26 @@ main(int argc, char *argv[])
>          }
>          argv++;
>      }
> +
> + if (input_name) {
> + if (unveil(input_name, "r") == -1)
> + err(1, "unveil");
> + }
> + if (output_name) {
> + if (unveil(output_name, "rwc") == -1)
> + err(1, "unveil");
> + if (pledge("stdio rpath wpath cpath", NULL) == -1)
> + err(1, "pledge");
> + }
> + if (input_name && !output_name) {
> + if (pledge("stdio rpath", NULL) == -1)
> + err(1, "pledge");
> + }
> + if (!input_name && !output_name) {
> + if (pledge("stdio", NULL) == -1)
> + err(1, "pledge");
> + }
> +
>      if (input_name) {
>          input = FontFileOpen(input_name);
>          if (!input) {

Reply | Threaded
Open this post in threaded view
|

Re: unveil bdftopcf

Matthieu Herrb-7
In reply to this post by Ricardo Mestre-2
On Wed, Oct 24, 2018 at 11:24:59AM +0100, Ricardo Mestre wrote:
> Hi,
>
> If input_name is provided we can unveil it with read permissions, if
> output_name is provided we need to unveil this one with rwc. Additionally
> depending on the different combinations of if these files are passed via args
> or from stdin/to stdout we can also pledge accordingly to the code path. This
> has been tested succefully with bdf fonts we have bundled in
> xenocara.

This one looks ok, but it lacks autoconf support to allow the
application to build on other systems.
>
> Since I have several other X apps unveiled and/or pledged could you please
> comment not only with the unveil/pledge part, but also err vs fprintf/exit, the
> placement of the #includes and also tabs vs spaces?


Generally, I'm not too found of pledging/unveiling random X client
programs. There are a lot of "hidden" features in X libraries that
will probably break with too strict pledges and/or unveils.

Also since this is OpenBSD-specific, it will be difficult to get it
upstreams, especially if you don't provide the autoconf goo to make
the code still build/work on Linux. And when not upstreaming it
creates more burden to merge new versions of the applications.


>
> Index: bdftopcf.c
> ===================================================================
> RCS file: /cvs/xenocara/app/bdftopcf/bdftopcf.c,v
> retrieving revision 1.5
> diff -u -p -u -r1.5 bdftopcf.c
> --- bdftopcf.c 29 Mar 2018 20:34:30 -0000 1.5
> +++ bdftopcf.c 24 Oct 2018 10:15:41 -0000
> @@ -38,7 +38,9 @@ from The Open Group.
>  #include "fntfil.h"
>  #include "bdfint.h"
>  #include "pcf.h"
> +#include <err.h>
>  #include <stdio.h>
> +#include <unistd.h>
>  #include <X11/Xos.h>
>  
>  int
> @@ -158,6 +160,26 @@ main(int argc, char *argv[])
>          }
>          argv++;
>      }
> +
> + if (input_name) {
> + if (unveil(input_name, "r") == -1)
> + err(1, "unveil");
> + }
> + if (output_name) {
> + if (unveil(output_name, "rwc") == -1)
> + err(1, "unveil");
> + if (pledge("stdio rpath wpath cpath", NULL) == -1)
> + err(1, "pledge");
> + }
> + if (input_name && !output_name) {
> + if (pledge("stdio rpath", NULL) == -1)
> + err(1, "pledge");
> + }
> + if (!input_name && !output_name) {
> + if (pledge("stdio", NULL) == -1)
> + err(1, "pledge");
> + }
> +
>      if (input_name) {
>          input = FontFileOpen(input_name);
>          if (!input) {
--
Matthieu Herrb

signature.asc (817 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: unveil bdftopcf

Theo de Raadt-2
Matthieu Herrb <[hidden email]> wrote:

> Generally, I'm not too found of pledging/unveiling random X client
> programs. There are a lot of "hidden" features in X libraries that
> will probably break with too strict pledges and/or unveils.

Well eventually we want to see if something can be done about xterm.
Especially if the lessons learned (I suspect some hoisting will occur)
can be pushed back upstream, and maybe allow others to apply their
own system call limiter mechanism.  Perhaps not possible...

> Also since this is OpenBSD-specific, it will be difficult to get it
> upstreams, especially if you don't provide the autoconf goo to make
> the code still build/work on Linux. And when not upstreaming it
> creates more burden to merge new versions of the applications.

Well, I doubt it will create too much burden, generally these unveil
or pledge chunks are a small set of + lines, without changing other
logic.

Anyways, bdftopcf is not running near a security boundary.

Reply | Threaded
Open this post in threaded view
|

Re: unveil bdftopcf

Ricardo Mestre-2
Something like this then?

If it's too much burden to keep these local patches I can drop it, no problem.

Index: bdftopcf.c
===================================================================
RCS file: /cvs/xenocara/app/bdftopcf/bdftopcf.c,v
retrieving revision 1.5
diff -u -p -u -r1.5 bdftopcf.c
--- bdftopcf.c 29 Mar 2018 20:34:30 -0000 1.5
+++ bdftopcf.c 25 Oct 2018 07:00:50 -0000
@@ -39,6 +39,7 @@ from The Open Group.
 #include "bdfint.h"
 #include "pcf.h"
 #include <stdio.h>
+#include <unistd.h>
 #include <X11/Xos.h>
 
 int
@@ -158,6 +159,38 @@ main(int argc, char *argv[])
         }
         argv++;
     }
+
+    if (input_name) {
+        if (unveil(input_name, "r") == -1) {
+            fprintf(stderr, "%s: could not unveil %s\n",
+                    program_name, input_name);
+            exit(1);
+ }
+    }
+    if (output_name) {
+        if (unveil(output_name, "rwc") == -1) {
+            fprintf(stderr, "%s: could not unveil %s\n",
+                    program_name, output_name);
+            exit(1);
+        }
+        if (pledge("stdio rpath wpath cpath", NULL) == -1) {
+            fprintf(stderr, "%s: could not pledge", program_name);
+            exit(1);
+        }
+    }
+    if (input_name && !output_name) {
+        if (pledge("stdio rpath", NULL) == -1) {
+            fprintf(stderr, "%s: could not pledge", program_name);
+            exit(1);
+        }
+    }
+    if (!input_name && !output_name) {
+        if (pledge("stdio", NULL) == -1) {
+            fprintf(stderr, "%s: could not pledge", program_name);
+            exit(1);
+        }
+    }
+
     if (input_name) {
         input = FontFileOpen(input_name);
         if (!input) {

On 10:41 Wed 24 Oct     , Theo de Raadt wrote:

> Matthieu Herrb <[hidden email]> wrote:
>
> > Generally, I'm not too found of pledging/unveiling random X client
> > programs. There are a lot of "hidden" features in X libraries that
> > will probably break with too strict pledges and/or unveils.
>
> Well eventually we want to see if something can be done about xterm.
> Especially if the lessons learned (I suspect some hoisting will occur)
> can be pushed back upstream, and maybe allow others to apply their
> own system call limiter mechanism.  Perhaps not possible...
>
> > Also since this is OpenBSD-specific, it will be difficult to get it
> > upstreams, especially if you don't provide the autoconf goo to make
> > the code still build/work on Linux. And when not upstreaming it
> > creates more burden to merge new versions of the applications.
>
> Well, I doubt it will create too much burden, generally these unveil
> or pledge chunks are a small set of + lines, without changing other
> logic.
>
> Anyways, bdftopcf is not running near a security boundary.
>