Re: [bug-gnu-libiconv] libiconv: patch to pledge(2) iconv binary for OpenBSD

[Bruno Haible]

[CCing the mailing list. Please don't write to me privately.]

Hiltjo Posthuma wrote:
> Hi Bruno,
> 
> Here is a patch to "pledge" the iconv command-line binary for OpenBSD.
> 
> pledge is an OpenBSD-specific system-call to restrict system operations. It is
> basically a sortof sandbox mechanism as a security mitigation.
> 
> Here is the pledge man page describing its workings: 
> https://man.openbsd.org/pledge
> 
> Would it be acceptable to have this upstreamed or maybe not because it is too
> specific for OpenBSD? If it's not accepted I totally understand.
> 
> Thanks for any reply,

If, so far, you have modified 500 programs to use this 'pledge()'
system call and don't want to eternally maintain this patches, I
fully understand this and won't object to upstreaming, unless the
patch has problems (see below).

Personally I think this sandboxing could be more useful if the
information was stored in a way that the user can look it up reliably.
That is, either store it in a separate file (this is what is done
on .NET, Android, and iOS), or embed it into the executable in a
way that 'readelf' or 'objdump' can extract it. Currently,
it is stored as machine code in the executable. So, the person
who sends me an executable to use on my machine may lie to me
about what it does. He may tell me "this executable uses only
stdio and rpath" and in fact it uses cycles from my graphics card
and makes network accesses to report crypto currency signatures.

> Patch:
> 
> 
> --- src/iconv.c.orig
> +++ src/iconv.c
> @@ -19,6 +19,12 @@
>  # define ICONV_CONST
>  #endif
>  
> +#ifdef __OpenBSD__
> +#include <unistd.h>
> +#else
> +#define pledge(p1,p2) 0
> +#endif

Please conditionalize according to the OpenBSD version as well.
If someone compiles my program on an older OpenBSD version, such
as OpenBSD 3.8, I don't want that they get compilation errors.
(I know that the OpenBSD project considers these versions unsupported.
But I aim to support OS release at least 5-10 years backwards.)

>  #include <limits.h>
>  #include <stddef.h>
>  #include <stdio.h>
> @@ -853,6 +859,8 @@ int main (int argc, char* argv[])
>    int i;
>    int status;
>  
> +  if (pledge("stdio rpath", NULL) == -1)
> +    error(EXIT_FAILURE, errno, "pledge");

This is not good. If the pledge system call fails — for whatever
reasons — I don't want to have the 'iconv' program always fail.
A program that always fails is useless.

>    set_program_name (argv[0]);
>  #if HAVE_SETLOCALE
>    /* Needed for the locale dependent encodings, "char" and "wchar_t",
> @@ -1008,6 +1016,8 @@ int main (int argc, char* argv[])
>      }
>      break;
>    }
> +  if ((do_list || i == argc) && pledge("stdio", NULL) == -1)
> +    error(EXIT_FAILURE, errno, "pledge");

This is overkill, IMO. One pledge() call per program should be enough.
What you do here is more "secure", but comes at a maintenance cost:
Each time new command-line options get introduced, this piece of code
would have to be reviewed.

>    if (do_list) {
>      if (i != 2 || i != argc)
>        usage(1);
> 
> 

Bruno