Re: lintian: shlib-read-write-env

[Simon McVittie]

On Wed, 01 Feb 2017 at 11:02:08 +0100, Vincent Danjean wrote:
> In an MT context, such a program should probably use setenv between
> the fork and the exec (ie not in MT context)

Calling non-async-signal safe functions after fork but before exec, in
a process that uses threads, is undefined behaviour according to POSIX.
Try not to do that, particularly in portable software.

(It sort-of-mostly-works on glibc in practice.)

The async-signal-safe functions are the same ones it is safe to call
in a POSIX signal handler - basically, those that are syscalls or
very thin wrappers around syscalls.

> or, probably better,
> use exec variants allowing to specify the new environment.

This. Functions that copy the environment and modify the copy,
like GLib's g_get_environ() and g_environ_setenv(), make this a lot
easier.

S

Re: lintian: shlib-read-write-env

[Vincent Danjean]

Le 31/01/2017 à 16:56, Christian Seiler a écrit :
> (Any program that calls setenv() will call getenv() as well at
> some point, otherwise you could simply drop the setenv() completely;

Not necessarily. Instead of calling getenv, it can call fork+exec
(that will run an other program, MT or not, in the new environment).
The most classical example is the shell that can set lots of
environment variables from its startup files without necessarily
reading them.

> so any program that does that in an MT context is broken anyway,
> regardless of whether it uses a library that does an additional
> getenv().)

In an MT context, such a program should probably use setenv between
the fork and the exec (ie not in MT context) or, probably better,
use exec variants allowing to specify the new environment.

  Regards,
Vincent

-- 
Vincent Danjean   GPG key ID 0xD17897FA vdanj...@debian.org
GPG key fingerprint: 621E 3509 654D D77C 43F5  CA4A F6AE F2AF D178 97FA
Unofficial pkgs: http://moais.imag.fr/membres/vincent.danjean/deb.html
APT repo:  deb http://people.debian.org/~vdanjean/debian unstable main

Re: lintian: shlib-read-write-env

[Mathieu Malaterre]

On Tue, Jan 31, 2017 at 4:56 PM, Christian Seiler  wrote:
> On 01/31/2017 04:49 PM, Ben Hutchings wrote:
>> On Tue, 2017-01-31 at 14:23 +0100, Christian Seiler wrote:
>>> On 01/31/2017 11:15 AM, Mathieu Malaterre wrote:
 I'd like to discuss addition of a new lintian checks for
 getenv/setenv/putenv used in shared libraries.
>>>
>>> Why getenv() though? It just reads the environment.
 From what you link yourself:
 The getenv and secure_getenv functions can be safely used in
 multi-threaded programs.
>> [...]
>>
>> But it returns a pointer to the value, which might be freed by another
>> thread before it is used.  If there were a reader function that copied
>> the value to a caller-provided buffer, it could be properly thread-
>> safe.
>
> But that's only a problem if you call setenv() or similar in a
> different thread, which you shouldn't do.
>
> getenv() is only unsafe if the environment is modified, a library
> using getenv() in a program that follows libc's guidelines to not
> call setenv() in an MT-context is perfectly fine.

That was precisely my point. Usage of `getenv` even from a
multithreaded program (see demo code I sent) can only lead to crash in
case another thread (same process) is calling `setenv` (or
equivalent). I had security concern, but this discussion proved it is
impossible to exploit.

-M

Re: lintian: shlib-read-write-env

[Christian Seiler]

On 01/31/2017 04:49 PM, Ben Hutchings wrote:
> On Tue, 2017-01-31 at 14:23 +0100, Christian Seiler wrote:
>> On 01/31/2017 11:15 AM, Mathieu Malaterre wrote:
>>> I'd like to discuss addition of a new lintian checks for
>>> getenv/setenv/putenv used in shared libraries.
>>
>> Why getenv() though? It just reads the environment.
>>> From what you link yourself:
>>> The getenv and secure_getenv functions can be safely used in
>>> multi-threaded programs.
> [...]
> 
> But it returns a pointer to the value, which might be freed by another
> thread before it is used.  If there were a reader function that copied
> the value to a caller-provided buffer, it could be properly thread-
> safe.

But that's only a problem if you call setenv() or similar in a
different thread, which you shouldn't do.

getenv() is only unsafe if the environment is modified, a library
using getenv() in a program that follows libc's guidelines to not
call setenv() in an MT-context is perfectly fine.

(Any program that calls setenv() will call getenv() as well at
some point, otherwise you could simply drop the setenv() completely;
so any program that does that in an MT context is broken anyway,
regardless of whether it uses a library that does an additional
getenv().)

So regardless of whether a check for setenv() etc. in libraries
is introduced into lintian: getenv() shouldn't be checked for
IMHO.

That said: I do agree that the way the entire API around
environment variables is defined is quite horrible.

Regards,
Christian

Re: lintian: shlib-read-write-env

[Ben Hutchings]

On Tue, 2017-01-31 at 14:23 +0100, Christian Seiler wrote:
> On 01/31/2017 11:15 AM, Mathieu Malaterre wrote:
> > I'd like to discuss addition of a new lintian checks for
> > getenv/setenv/putenv used in shared libraries.
> 
> Why getenv() though? It just reads the environment.
> > From what you link yourself:
> > The getenv and secure_getenv functions can be safely used in
> > multi-threaded programs.
[...]

But it returns a pointer to the value, which might be freed by another
thread before it is used.  If there were a reader function that copied
the value to a caller-provided buffer, it could be properly thread-
safe.

(The C library could also make getenv() thread-safe by maintaining a
per-thread cache of the environment and returning a pointer into that. 
But portable software still couldn't assume this.)

Ben.

-- 
Ben Hutchings
It is easier to write an incorrect program than to understand a correct
one.



signature.asc
Description: This is a digitally signed message part

Re: lintian: shlib-read-write-env

[Christian Seiler]

On 01/31/2017 11:15 AM, Mathieu Malaterre wrote:
> I'd like to discuss addition of a new lintian checks for
> getenv/setenv/putenv used in shared libraries.

Why getenv() though? It just reads the environment.
>From what you link yourself:

| The getenv and secure_getenv functions can be safely used in
| multi-threaded programs.

Furthermore:

> Modifications of environment variables are not allowed in
> multi-threaded programs.

Yes. However, just because a library imports a symbol doesn't mean
it uses it in a multithreaded context. (What if a specific routine
in the library uses setenv(), but that routine is clearly marked
as not to be used in an MT-context in the library's docs? And the
rest of the library is safe?)

Case in point: [1]

readelf -sW /usr/lib/x86_64-linux-gnu/libpython3.5m.so | grep putenv
   205:  0 FUNCGLOBAL DEFAULT  UND putenv@GLIBC_2.2.5 
(2)

So basically you have the problem that you can't really be certain
that this is bad just based on the symbol being imported, but if
the library does do it improperly, it's really, really bad. So
from the severity you'd at least want a lintian warning, but
OTOH I don't think it makes sense to have tons of maintainers of
perfectly fine libraries override this tag.

I think a check like this would be great, same as e.g. a check
that libraries properly use O_CLOEXEC for fds, but that's also
very, very difficult to get right as a check. [2]

Regards,
Christian

[1] Yeah, I know, Python is not great with threads due to the GIL,
but it _is_ thread-safe.
[2] Especially since code could have runtime fallbacks for older
kernels that didn't yet have dup3, accept4 or similar. And
you'd basically need to do static analysis. And the library
could provide a function where the caller explicitly requests
an fd without CLOEXEC, especially if the library wraps open()
in some way.

Re: lintian: shlib-read-write-env

[Mathieu Malaterre]

On Tue, Jan 31, 2017 at 12:14 PM, Simon McVittie  wrote:
> On Tue, 31 Jan 2017 at 11:15:32 +0100, Mathieu Malaterre wrote:
>> I'd like to discuss addition of a new lintian checks for
>> getenv/setenv/putenv used in shared libraries.
>
> A massive number of libraries call getenv(). This is not something that
> you can just ban. In many cases (any D-Bus implementation, anything
> that uses XDG_whatever_DIRS, anything that uses PATH...) it is also
> "ABI" that would lead to broken systems if removed.
>
> A massive number of libraries also call gettext(), which has similar
> issues with setlocale() as the setenv-equivalent. This is not something
> that you can ban either.
>
> The policy that the GLib/GNOME stack has chosen (and documented!) is to
> say that libraries in that stack may call getenv() and gettext() freely,
> but applications using those libraries are only allowed to call setenv()
> or setlocale() near the beginning of main(), before a second thread
> is created. This is by no means ideal, but given the constraints I
> can't see anything better[1].
>
> Functions in GLib that indirectly call setenv() or setlocale() are
> documented as having the same constraints as setenv() itself.
> Again, this is not ideal but is about as good as we're going to get.

Ok, I see your point. I'll reformulate my original bug report as a
documentation enhancement then.

Thanks for clarification, I knew this was difficult to reproduce a
crash 'in the wild'.

-M

Re: lintian: shlib-read-write-env

[Mathieu Malaterre]

On Tue, Jan 31, 2017 at 11:26 AM, Andrey Rahmatullin  wrote:
> On Tue, Jan 31, 2017 at 11:15:32AM +0100, Mathieu Malaterre wrote:
>> I'd like to discuss addition of a new lintian checks for
>> getenv/setenv/putenv used in shared libraries.
> Do you know any packages that would fail that?
> Did you mean *jpeg* ones would?

I've used the demo program mixed with djpeg.c (see demo.c attached),
and got a segfault running (run for ~30s)[*]:

$ gcc -o demo demo.c -lpthread -ljpeg && ./demo

So yes, I am trying to raise severity on bugs I reported before 778909
& 778910. I suspect other libs may use getenv, hence asking for an
automated lintian check, but you are right maybe there are no others
out there since I did not check.

[*] You'll get a killed job since this quick & dirty demo does not
deallocate memory.
#include 
#include 
#include 
#include 
 
static void* worker(void* arg) {
  for (;;) {
int i;
char var[256], *p = var;
 
for (i = 0; i < 8; ++i) {
  *p++ = 65 + (random() % 26);
}
 
*p++ = '\0';
 
setenv(var, "test", 1);
  }
 
  return NULL;
}
 
int
main (int argc, char **argv)
{
  struct jpeg_decompress_struct cinfo;
  struct jpeg_error_mgr jerr;
#ifdef PROGRESS_REPORT
  struct cdjpeg_progress_mgr progress;
#endif
  int file_index;
  FILE *input_file;
  FILE *output_file;
  unsigned char *inbuffer = NULL;
  unsigned long insize = 0;
  JDIMENSION num_scanlines;
  pthread_t t;

  /* On Mac, fetch a command line. */
#ifdef USE_CCOMMAND
  argc = ccommand();
#endif

  char * progname = argv[0];
  if (progname == NULL || progname[0] == 0)
progname = "djpeg"; /* in case C library doesn't provide it */

  /* Initialize the JPEG decompression object with default error handling. */
  cinfo.err = jpeg_std_error();


  setenv("JPEGMEM", "1", 0);
  pthread_create(, NULL, worker, 0);
 
  for (;;) 
  jpeg_create_decompress();

  return 0;
}

Re: lintian: shlib-read-write-env

[Simon McVittie]

On Tue, 31 Jan 2017 at 11:15:32 +0100, Mathieu Malaterre wrote:
> I'd like to discuss addition of a new lintian checks for
> getenv/setenv/putenv used in shared libraries.

A massive number of libraries call getenv(). This is not something that
you can just ban. In many cases (any D-Bus implementation, anything
that uses XDG_whatever_DIRS, anything that uses PATH...) it is also
"ABI" that would lead to broken systems if removed.

A massive number of libraries also call gettext(), which has similar
issues with setlocale() as the setenv-equivalent. This is not something
that you can ban either.

The policy that the GLib/GNOME stack has chosen (and documented!) is to
say that libraries in that stack may call getenv() and gettext() freely,
but applications using those libraries are only allowed to call setenv()
or setlocale() near the beginning of main(), before a second thread
is created. This is by no means ideal, but given the constraints I
can't see anything better[1].

Functions in GLib that indirectly call setenv() or setlocale() are
documented as having the same constraints as setenv() itself.
Again, this is not ideal but is about as good as we're going to get.

S

[1] For projects that don't care about POSIX but only target GNU platforms,
having glibc put mutexes around *env(), setlocale() and gettext(), and
declare direct access to environ to be non-thread-safe, would perhaps be
enough? That solution doesn't work for GLib/GNOME in general, which
aim to be portable; but it would work for some projects.

Re: lintian: shlib-read-write-env

[Andrey Rahmatullin]

On Tue, Jan 31, 2017 at 11:15:32AM +0100, Mathieu Malaterre wrote:
> I'd like to discuss addition of a new lintian checks for
> getenv/setenv/putenv used in shared libraries.
Do you know any packages that would fail that?
Did you mean *jpeg* ones would?

-- 
WBR, wRAR


signature.asc
Description: PGP signature

lintian: shlib-read-write-env

[Mathieu Malaterre]

Hi there,

I'd like to discuss addition of a new lintian checks for
getenv/setenv/putenv used in shared libraries.

Per glibc manual:

Modifications of environment variables are not allowed in
multi-threaded programs.

-- the glibc manual
[https://www.gnu.org/software/libc/manual/html_node/Environment-Access.html#Environment-Access]

I am targetting in particular the libjpeg / turbojpeg family which I
know are used in multithreaded programs.

A good demo program:
https://rachelbythebay.com/w/2017/01/30/env/


Comments anyone ?

Thanks
-M