Tighten nl(1) pledge(2) a bit

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

Tighten nl(1) pledge(2) a bit

Rafael Neves-2
Hi tech@,

The Patch 1 below tighten pledge(2) promises to stdio, after the
freopen(3) call, and replaces an exit(3) call to return, so the
stack protector could be used.

I verify that after pledge stdio, there are only calls to:
getline(3), memcmp(3), regexec(3), printf(3), fwrite(3), fputs(3),
ferror(3), free(3), and err(3). All this functions do not need
the rpath promise.

The Patch 2, includes Patch 1 and uses unveil(2) when the user supply
a file. A prior version of this patch used unveil("/dev/null", "r")
after the first pledge call, to forbid any filesystem access, except
the file supplied. But it seemed like a hack.


Comments?

Regards,
Rafael Neves




Patch 1 (just pledge):

Index: usr.bin/nl/nl.c
===================================================================
RCS file: /cvs/src/usr.bin/nl/nl.c,v
retrieving revision 1.6
diff -u -p -r1.6 nl.c
--- usr.bin/nl/nl.c 9 Oct 2015 01:37:08 -0000 1.6
+++ usr.bin/nl/nl.c 20 Apr 2019 21:27:39 -0000
@@ -218,6 +218,9 @@ main(int argc, char *argv[])
  /* NOTREACHED */
  }
 
+ if (pledge("stdio", NULL) == -1)
+ err(1, "pledge");
+
  /* Generate the delimiter sequence */
  memcpy(delim, delim1, delim1len);
  memcpy(delim + delim1len, delim2, delim2len);
@@ -226,7 +229,7 @@ main(int argc, char *argv[])
  /* Do the work. */
  filter();
 
- exit(EXIT_SUCCESS);
+ return(EXIT_SUCCESS);
 }
 
 void



Pledge 2 (pledge and unveil):

Index: usr.bin/nl/nl.c
===================================================================
RCS file: /cvs/src/usr.bin/nl/nl.c,v
retrieving revision 1.6
diff -u -p -r1.6 nl.c
--- usr.bin/nl/nl.c 9 Oct 2015 01:37:08 -0000 1.6
+++ usr.bin/nl/nl.c 20 Apr 2019 21:28:44 -0000
@@ -118,7 +118,7 @@ main(int argc, char *argv[])
 
  (void)setlocale(LC_ALL, "");
 
- if (pledge("stdio rpath", NULL) == -1)
+ if (pledge("stdio unveil rpath", NULL) == -1)
  err(1, "pledge");
 
  while ((c = getopt(argc, argv, "pb:d:f:h:i:l:n:s:v:w:")) != -1) {
@@ -209,8 +209,14 @@ main(int argc, char *argv[])
  case 0:
  break;
  case 1:
- if (strcmp(argv[0], "-") != 0 &&
-    freopen(argv[0], "r", stdin) == NULL)
+ if (strcmp(argv[0], "-") == 0)
+ break;
+
+ /* Let freopen(3) check deal with permission problems. */
+ if (unveil(argv[0], "r") == -1 && errno != ENOENT)
+ err(1, "unveil");
+
+ if (freopen(argv[0], "r", stdin) == NULL)
  err(EXIT_FAILURE, "%s", argv[0]);
  break;
  default:
@@ -218,6 +224,9 @@ main(int argc, char *argv[])
  /* NOTREACHED */
  }
 
+ if (pledge("stdio", NULL) == -1)
+ err(1, "pledge");
+
  /* Generate the delimiter sequence */
  memcpy(delim, delim1, delim1len);
  memcpy(delim + delim1len, delim2, delim2len);
@@ -226,7 +235,7 @@ main(int argc, char *argv[])
  /* Do the work. */
  filter();
 
- exit(EXIT_SUCCESS);
+ return(EXIT_SUCCESS);
 }
 
 void

Reply | Threaded
Open this post in threaded view
|

Re: Tighten nl(1) pledge(2) a bit

Theo de Raadt-2
Rafael Neves <[hidden email]> wrote:

> Hi tech@,
>
> The Patch 1 below tighten pledge(2) promises to stdio, after the
> freopen(3) call,

I've commited this.

> and replaces an exit(3) call to return, so the
> stack protector could be used.

I'm still not a huge fan of those.  I don't recall ever seeing
an overflow in such a circumstance.

> I verify that after pledge stdio, there are only calls to:
> getline(3), memcmp(3), regexec(3), printf(3), fwrite(3), fputs(3),
> ferror(3), free(3), and err(3). All this functions do not need
> the rpath promise.
>
> The Patch 2, includes Patch 1 and uses unveil(2) when the user supply
> a file. A prior version of this patch used unveil("/dev/null", "r")
> after the first pledge call, to forbid any filesystem access, except
> the file supplied. But it seemed like a hack.

Doing 1 unveil before 1 open, is pointless.  Instead, drop all path
pledges immediately afterwards.  Which is what "stdio" is about.

Reply | Threaded
Open this post in threaded view
|

Re: Tighten nl(1) pledge(2) a bit

Rafael Neves-2
On Sat, Apr 20, 2019 at 07:10:21PM -0600, Theo de Raadt wrote:

> Rafael Neves <[hidden email]> wrote:
>
> > Hi tech@,
> >
> > The Patch 1 below tighten pledge(2) promises to stdio, after the
> > freopen(3) call,
>
> I've commited this.
>
> > and replaces an exit(3) call to return, so the
> > stack protector could be used.
>
> I'm still not a huge fan of those.  I don't recall ever seeing
> an overflow in such a circumstance.
>

I am no security expert, so I can't comment on this. Actually,
I saw the exit() instead of return while analysing the code path
after freopen() call. I remembered there were commits with that
kind of change, then I combined both. Anyway, it was not the
focus of the change.

> > I verify that after pledge stdio, there are only calls to:
> > getline(3), memcmp(3), regexec(3), printf(3), fwrite(3), fputs(3),
> > ferror(3), free(3), and err(3). All this functions do not need
> > the rpath promise.
> >
> > The Patch 2, includes Patch 1 and uses unveil(2) when the user supply
> > a file. A prior version of this patch used unveil("/dev/null", "r")
> > after the first pledge call, to forbid any filesystem access, except
> > the file supplied. But it seemed like a hack.
>
> Doing 1 unveil before 1 open, is pointless.  Instead, drop all path
> pledges immediately afterwards.  Which is what "stdio" is about.

Indeed, totally right! Thanks to draw attention to that.

Regards,
Rafael Neves