Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't allocate std::random_device statically #1743

Merged
merged 1 commit into from Dec 1, 2015

Conversation

ksmadsen
Copy link

It appears that on Linux with gcc (tested with gcc 4.6.3 on Ubuntu 12.04 and
gcc 5.2.1 on Ubuntu 15.10) std::random_device will open /dev/urandom, and keep
the file open for the lifetime of the std::random_device object.

Since libsass allocates the std::random_device globally, it will not be
destroyed if the process execs. Futhermore std::random_device does not set
close-on-exec on the fd, so the fd will be leaked to the child process.

The std::random_device object is only used by the GetSeed function, and as the
GetSeed function is only called once to seed the rand variable, there is no
need to keep the std::random_device object.

So this patch moves the std::random_device object into the GetSeed function,
thereby eliminating the fd-leak.

The following program illustrates the problem:

// To compile: g++ -o fdleak_example fdleak_example.cpp -Llib -lsass -Iinclude -ldl
#include <stdlib.h>
#include <sys/types.h>
#include <unistd.h>
#include <stdio.h>
#include "sass.h"

int main() {
  pid_t pid = getpid();
  char command[250];

  struct Sass_Options *options = sass_make_options();
  free(options);

  sprintf(command, "pfd=\"/proc/%d/fd\";"
      "cfd=\"/proc/$$/fd\";"
      "echo \"Parent: $pfd\";"
      "ls -l $pfd;"
      "echo \"Child: $cfd\";"
      "ls -l $cfd;"
      "ps uf --pid %d --ppid %d", pid, pid, pid);
  system(command);
  return 0;
}

Output from program before this patch:

Parent: /proc/6494/fd
total 0
lrwx------ 1 ksm ksm 64 nov 18 11:20 0 -> /dev/pts/3
lrwx------ 1 ksm ksm 64 nov 18 11:20 1 -> /dev/pts/3
lrwx------ 1 ksm ksm 64 nov 18 11:20 2 -> /dev/pts/3
lr-x------ 1 ksm ksm 64 nov 18 11:20 3 -> /dev/urandom
Child: /proc/6495/fd
total 0
lrwx------ 1 ksm ksm 64 nov 18 11:20 0 -> /dev/pts/3
lrwx------ 1 ksm ksm 64 nov 18 11:20 1 -> /dev/pts/3
lrwx------ 1 ksm ksm 64 nov 18 11:20 2 -> /dev/pts/3
lr-x------ 1 ksm ksm 64 nov 18 11:20 3 -> /dev/urandom
  PID TTY      STAT   TIME COMMAND
 6494 pts/3    S+     0:00 ./fdleak_example
 6495 pts/3    S+     0:00  \_ sh -c pfd="/proc/6494/fd";cfd="/proc/$$/fd";echo "Parent: $pfd";ls -l $pfd;echo "Child: $cfd";ls -l $cfd;ps f --pid 6494 --ppid 6494

Output from program after this patch:

Parent: /proc/6568/fd
total 0
lrwx------ 1 ksm ksm 64 nov 18 11:21 0 -> /dev/pts/3
lrwx------ 1 ksm ksm 64 nov 18 11:21 1 -> /dev/pts/3
lrwx------ 1 ksm ksm 64 nov 18 11:21 2 -> /dev/pts/3
Child: /proc/6569/fd
total 0
lrwx------ 1 ksm ksm 64 nov 18 11:21 0 -> /dev/pts/3
lrwx------ 1 ksm ksm 64 nov 18 11:21 1 -> /dev/pts/3
lrwx------ 1 ksm ksm 64 nov 18 11:21 2 -> /dev/pts/3
  PID TTY      STAT   TIME COMMAND
 6568 pts/3    S+     0:00 ./fdleak_example
 6569 pts/3    S+     0:00  \_ sh -c pfd="/proc/6568/fd";cfd="/proc/$$/fd";echo "Parent: $pfd";ls -l $pfd;echo "Child: $cfd";ls -l $cfd;ps f --pid 6568 --ppid 6568

It appears that on Linux with gcc (tested with gcc 4.6.3 on Ubuntu 12.04 and
gcc 5.2.1 on Ubuntu 15.10) std::random_device will open /dev/urandom, and keep
the file open for the lifetime of the std::random_device object.

Since libsass allocates the std::random_device globally, it will not be
destroyed if the processes execs. Futhermore std::random_device does not set
close-on-exec on the fd, so the fd will be leaked to the child process.

The std::random_device object is only used by the GetSeed function, and as the
GetSeed function is only called once to seed the rand variable, there is no
need to keep the std::random_device object.

So this patch moves the std::random_device object into the GetSeed function,
thereby eliminating the fd-leak.
@saper
Copy link
Member

saper commented Nov 18, 2015

Thanks, I have changed your shellcode to sprintf(command, "procstat -f %d; procstat -f $$", pid); to reproduce this on FreeBSD (with clang); I think your change makes sense.

@xzyfer
Copy link
Contributor

xzyfer commented Nov 18, 2015

This make sense to me.

@mgreter
Copy link
Contributor

mgreter commented Nov 25, 2015

LGTM

xzyfer added a commit that referenced this pull request Dec 1, 2015
Don't allocate std::random_device statically
@xzyfer xzyfer merged commit d8342a2 into sass:master Dec 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants