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

Some tweaks for MPR#7557 #1213

Merged
merged 5 commits into from Jun 28, 2017

Conversation

Projects
None yet
8 participants
@damiendoligez
Member

damiendoligez commented Jun 26, 2017

For the first commit, @setharnold pointed out in 38e2cd6#commitcomment-22736369 that secure_getenv is a "relatively new" addition to glibc (introduced in 2012 with glibc 2.17). This PR adds a fall-back to __secure_getenv, which dates back (at least) to 2002 with glibc 2.2.5.

references:

The second commit secures one more call to getenv in the Spacetime code. Although you'd be crazy to set the setuid bit on a program compiled with instrumentation, I think it's better to fix this potential vulnerability (reported privately by Eric Milliken).

The third commit is a change in the manual to warn users against setting the setuid bit on a custom-mode bytecode executable because this configuration is always vulnerable to privilege escalation attacks.

@damiendoligez damiendoligez added this to the 4.05.0 milestone Jun 26, 2017

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@lefessan

This comment has been minimized.

Show comment
Hide comment
@lefessan

lefessan Jun 27, 2017

Contributor
Contributor

lefessan commented Jun 27, 2017

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Jun 27, 2017

Member

@lefessan
I'm not sure it should be discussed publicly yet, and I don't think it can be fixed in a reliable way.

Member

damiendoligez commented Jun 27, 2017

@lefessan
I'm not sure it should be discussed publicly yet, and I don't think it can be fixed in a reliable way.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Jun 27, 2017

Contributor

OK

Contributor

mshinwell commented Jun 27, 2017

OK

@mshinwell mshinwell added the approved label Jun 27, 2017

Show outdated Hide outdated manual/manual/cmds/unified-options.etex
@@ -169,6 +169,10 @@ chapter~\ref{c:intf-c}.
Never use the "strip" command on executables produced by "ocamlc -custom",
this would remove the bytecode part of the executable.
\end{unix}
\begin{unix}
Security warning: never set the "setuid" or "setgid" bits on executables

This comment has been minimized.

@xavierleroy

xavierleroy Jun 27, 2017

Contributor

The double quotes around "setuid" causes this word to be typeset in \tt font. This is probably not what you want. Consider ``setuid'' instead. Likewise for setgid.

@xavierleroy

xavierleroy Jun 27, 2017

Contributor

The double quotes around "setuid" causes this word to be typeset in \tt font. This is probably not what you want. Consider ``setuid'' instead. Likewise for setgid.

This comment has been minimized.

@damiendoligez

damiendoligez Jun 27, 2017

Member

Actually that's what I wanted but you're right, it's probably not a good idea.

@damiendoligez

damiendoligez Jun 27, 2017

Member

Actually that's what I wanted but you're right, it's probably not a good idea.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Jun 27, 2017

Contributor

Looks good to me.

Contributor

xavierleroy commented Jun 27, 2017

Looks good to me.

@avsm

This comment has been minimized.

Show comment
Hide comment
@avsm

avsm Jun 27, 2017

Member

I've tested this on a CentOS 6 container and confirmed that __secure_getenv is found and used.

# docker run -it ocaml/opam:centos-6_ocaml-4.04.2
$ ldd --version
ldd (GNU libc) 2.12
$ git clone git://github.com/damiendoligez/ocaml -b more-secure-getenv
$ cd ocaml
$ ./configure
Configuring OCaml version 4.05.0+dev10-2017-06-23
Configuring for host x86_64-unknown-linux-gnu ...
Configuring for target x86_64-unknown-linux-gnu ...
Using compiler gcc.
Compiler family and version: gcc-4-4.
The C compiler is ISO C99 compliant.
Checking the sizes of integers and pointers...
Wow! A 64 bit architecture!
This is a little-endian architecture.
Doubles can be word-aligned.
64-bit integers can be word-aligned.
ranlib found
#! appears to work in shell scripts.
POSIX signal handling found.
expm1(), log1p(), hypot(), copysign() found.
getrusage() found.
times() found.
__secure_getenv() found.
termcap functions found (with libraries '-lcurses')
You have BSD sockets.
Member

avsm commented Jun 27, 2017

I've tested this on a CentOS 6 container and confirmed that __secure_getenv is found and used.

# docker run -it ocaml/opam:centos-6_ocaml-4.04.2
$ ldd --version
ldd (GNU libc) 2.12
$ git clone git://github.com/damiendoligez/ocaml -b more-secure-getenv
$ cd ocaml
$ ./configure
Configuring OCaml version 4.05.0+dev10-2017-06-23
Configuring for host x86_64-unknown-linux-gnu ...
Configuring for target x86_64-unknown-linux-gnu ...
Using compiler gcc.
Compiler family and version: gcc-4-4.
The C compiler is ISO C99 compliant.
Checking the sizes of integers and pointers...
Wow! A 64 bit architecture!
This is a little-endian architecture.
Doubles can be word-aligned.
64-bit integers can be word-aligned.
ranlib found
#! appears to work in shell scripts.
POSIX signal handling found.
expm1(), log1p(), hypot(), copysign() found.
getrusage() found.
times() found.
__secure_getenv() found.
termcap functions found (with libraries '-lcurses')
You have BSD sockets.
@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Jun 27, 2017

Contributor

FWIW, I'd define HAS_SECURE_GETENV a la HAS_NANOSECOND_STAT (so either #define HAS_SECURE_GETENV secure_getenv or #define HAS_SECURE_GETENV __secure_getenv as appropriate) just to avoid a triple underscore. I appreciate that having tried to add a PR with some vast number of backslashes in a row, I may be guilty of being a pot here...

However, it all looks good to me, even with a triple underscore!

Contributor

dra27 commented Jun 27, 2017

FWIW, I'd define HAS_SECURE_GETENV a la HAS_NANOSECOND_STAT (so either #define HAS_SECURE_GETENV secure_getenv or #define HAS_SECURE_GETENV __secure_getenv as appropriate) just to avoid a triple underscore. I appreciate that having tried to add a PR with some vast number of backslashes in a row, I may be guilty of being a pot here...

However, it all looks good to me, even with a triple underscore!

@stedolan

This comment has been minimized.

Show comment
Hide comment
@stedolan

stedolan Jun 27, 2017

Contributor

The second commit secures one more call to getenv in the Spacetime code. Although you'd be crazy to set the setuid bit on a program compiled with instrumentation, I think it's better to fix this potential vulnerability (reported privately by Eric Milliken).

It's probably worth doing the same in byterun/afl.c (Same logic applies as Spacetime: you'd be crazy to setuid an instrumented binary, and it's hard to get useful data out of afl-fuzz traces, but it's still worth fixing)

Contributor

stedolan commented Jun 27, 2017

The second commit secures one more call to getenv in the Spacetime code. Although you'd be crazy to set the setuid bit on a program compiled with instrumentation, I think it's better to fix this potential vulnerability (reported privately by Eric Milliken).

It's probably worth doing the same in byterun/afl.c (Same logic applies as Spacetime: you'd be crazy to setuid an instrumented binary, and it's hard to get useful data out of afl-fuzz traces, but it's still worth fixing)

Show outdated Hide outdated otherlibs/unix/unix.mli
programmer of a setuid program must be careful to prevent execution
of arbitrary commands through manipulation of the environment
programmer of a setuid or setgid program must be careful to prevent
execution of arbitrary commands through manipulation of the environment

This comment has been minimized.

@stedolan

stedolan Jun 27, 2017

Contributor

It is considered unsafe because the programmer of a setuid or setgid program must be careful to prevent execution of arbitrary commands through manipulation of the environment variables.

I think this message is not scary enough! Preventing direct execution of arbitrary commands is not enough: the usual attacks are more subtle things like attacker-controlled filenames or temporary directories. I'd suggest:

It is considered unsafe because the programmer of a setuid or setgid program must be careful to avoid using maliciously crafted environment variables in the search path for executables, the locations for temporary files or logs, and the like.

@stedolan

stedolan Jun 27, 2017

Contributor

It is considered unsafe because the programmer of a setuid or setgid program must be careful to prevent execution of arbitrary commands through manipulation of the environment variables.

I think this message is not scary enough! Preventing direct execution of arbitrary commands is not enough: the usual attacks are more subtle things like attacker-controlled filenames or temporary directories. I'd suggest:

It is considered unsafe because the programmer of a setuid or setgid program must be careful to avoid using maliciously crafted environment variables in the search path for executables, the locations for temporary files or logs, and the like.

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Jun 28, 2017

Member

@dra27 I don't really like the idea of a HAS_ macro being anything else than a boolean...

Member

damiendoligez commented Jun 28, 2017

@dra27 I don't really like the idea of a HAS_ macro being anything else than a boolean...

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Jun 28, 2017

Contributor

@damiendoligez - otherlibs/unix/nanosecond_stat.h? While talking about types in the C pre-processor is, um, interesting, surely the boolean bit is that it is defined and the value it is defined as (if it has one) in an optional extra?

Contributor

dra27 commented Jun 28, 2017

@damiendoligez - otherlibs/unix/nanosecond_stat.h? While talking about types in the C pre-processor is, um, interesting, surely the boolean bit is that it is defined and the value it is defined as (if it has one) in an optional extra?

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Jun 28, 2017

Member

@dra27 Just because it exists in the system doesn't mean I have to like it. In any case, it's too small of a difference to justify the (small) amount of work to change it. (And I still think it's better as it is now.)

Member

damiendoligez commented Jun 28, 2017

@dra27 Just because it exists in the system doesn't mean I have to like it. In any case, it's too small of a difference to justify the (small) amount of work to change it. (And I still think it's better as it is now.)

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Jun 28, 2017

Member

@stedolan Done for AFL and also for GC instrumentation, just in case.

Member

damiendoligez commented Jun 28, 2017

@stedolan Done for AFL and also for GC instrumentation, just in case.

@hannesm

This comment has been minimized.

Show comment
Hide comment
@hannesm

hannesm Jun 28, 2017

Member

this patch looks fine to me. Will the default runtime in 4.05 still have this cplugin "feature" enabled? I read through MPR 7557 and found some discussion about enabling/disabling it, but couldn't find any decision.

Member

hannesm commented Jun 28, 2017

this patch looks fine to me. Will the default runtime in 4.05 still have this cplugin "feature" enabled? I read through MPR 7557 and found some discussion about enabling/disabling it, but couldn't find any decision.

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Jun 28, 2017

Member

@hannesm It will because I'm trying to release today. Let's have a relaxed discussion (in a new PR!) about disabling it for 4.06, but I don't want to delay 4.05.0 yet again.

Member

damiendoligez commented Jun 28, 2017

@hannesm It will because I'm trying to release today. Let's have a relaxed discussion (in a new PR!) about disabling it for 4.06, but I don't want to delay 4.05.0 yet again.

@damiendoligez damiendoligez merged commit cf100ba into ocaml:4.05 Jun 28, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@damiendoligez damiendoligez deleted the damiendoligez:more-secure-getenv branch Jun 28, 2017

damiendoligez added a commit that referenced this pull request Jun 28, 2017

Some tweaks for MPR#7557 (#1213)
* fall back to __secure_getenv when secure_getenv is not available

* use secure_getenv for instrumented runtimes

* documentation: warn against setting the setuid or setgid bits on custom bytecode executables
@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Jun 28, 2017

Member

merged to 4.05 and cherry-picked to trunk (fee0110)

Member

damiendoligez commented Jun 28, 2017

merged to 4.05 and cherry-picked to trunk (fee0110)

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Some tweaks for MPR#7557 (#1213)
* fall back to __secure_getenv when secure_getenv is not available

* use secure_getenv for instrumented runtimes

* documentation: warn against setting the setuid or setgid bits on custom bytecode executables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment