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

11 files changed, 34 insertions, 11 deletions

diff --git a/Changes b/Changes
index ad1541da09..7d45323f6a 100644
--- a/Changes
+++ b/Changes
@@ -292,6 +292,10 @@ Next major version (4.05.0):
 
 ### Runtime system:
 
+- MPR#7557, GPR#1213: More security for getenv
+  (Damien Doligez, reports by Seth Arnold and Eric Milliken, review by
+  Xavier Leroy, David Allsopp, Stephen Dolan, Hannes Mehnert)
+
 - MPR#385, GPR#953: Add caml_startup_exn
   (Mark Shinwell)
 
diff --git a/asmrun/spacetime.c b/asmrun/spacetime.c
index 6e4dd4017b..e95cf68771 100644
--- a/asmrun/spacetime.c
+++ b/asmrun/spacetime.c
@@ -199,7 +199,7 @@ void caml_spacetime_initialize(void)
 
   caml_spacetime_static_shape_tables = &caml_spacetime_shapes;
 
-  ap_interval = getenv ("OCAML_SPACETIME_INTERVAL");
+  ap_interval = caml_secure_getenv ("OCAML_SPACETIME_INTERVAL");
   if (ap_interval != NULL) {
     unsigned int interval = 0;
     sscanf(ap_interval, "%u", &interval);
diff --git a/byterun/afl.c b/byterun/afl.c
index ef405bc66a..bd87ce8d8a 100644
--- a/byterun/afl.c
+++ b/byterun/afl.c
@@ -38,8 +38,11 @@ CAMLprim value caml_reset_afl_instrumentation(value unused)
 #include <sys/wait.h>
 #include <stdio.h>
 #include <string.h>
+
+#define CAML_INTERNALS
 #include "caml/misc.h"
 #include "caml/mlvalues.h"
+#include "caml/osdeps.h"
 
 static int afl_initialised = 0;
 
@@ -75,7 +78,7 @@ CAMLprim value caml_setup_afl(value unit)
   if (afl_initialised) return Val_unit;
   afl_initialised = 1;
 
-  char* shm_id_str = getenv("__AFL_SHM_ID");
+  char* shm_id_str = caml_secure_getenv("__AFL_SHM_ID");
   if (shm_id_str == NULL) {
     /* Not running under afl-fuzz, continue as normal */
     return Val_unit;
diff --git a/byterun/caml/osdeps.h b/byterun/caml/osdeps.h
index 18a3b5b107..bf9a48172f 100644
--- a/byterun/caml/osdeps.h
+++ b/byterun/caml/osdeps.h
@@ -87,7 +87,7 @@ extern int caml_read_directory(char * dirname, struct ext_table * contents);
 extern char * caml_executable_name(void);
 
 /* Secure version of [getenv]: returns NULL if the process has special
-   privileges (setuid bit or capabilities).
+   privileges (setuid bit, setgid bit, capabilities).
 */
 extern char *caml_secure_getenv(char const *var);
 
diff --git a/byterun/misc.c b/byterun/misc.c
index eb2ef02921..9d33ac118e 100644
--- a/byterun/misc.c
+++ b/byterun/misc.c
@@ -229,10 +229,10 @@ void CAML_INSTR_INIT (void)
   char *s;
 
   CAML_INSTR_STARTTIME = 0;
-  s = getenv ("OCAML_INSTR_START");
+  s = caml_secure_getenv ("OCAML_INSTR_START");
   if (s != NULL) CAML_INSTR_STARTTIME = atol (s);
   CAML_INSTR_STOPTIME = LONG_MAX;
-  s = getenv ("OCAML_INSTR_STOP");
+  s = caml_secure_getenv ("OCAML_INSTR_STOP");
   if (s != NULL) CAML_INSTR_STOPTIME = atol (s);
 }
 
diff --git a/byterun/unix.c b/byterun/unix.c
index 6853cfe57b..a5c5ed4576 100644
--- a/byterun/unix.c
+++ b/byterun/unix.c
@@ -412,6 +412,8 @@ char *caml_secure_getenv (char const *var)
 {
 #ifdef HAS_SECURE_GETENV
   return secure_getenv (var);
+#elif defined (HAS___SECURE_GETENV)
+  return __secure_getenv (var);
 #elif defined(HAS_ISSETUGID)
   if (!issetugid ())
     return CAML_SYS_GETENV (var);
diff --git a/configure b/configure
index 26e4dbfe7c..ec5e73c5eb 100755
--- a/configure
+++ b/configure
@@ -1144,6 +1144,9 @@ fi
 if sh ./hasgot2 -D_GNU_SOURCE -i stdlib.h secure_getenv; then
   inf "secure_getenv() found."
   echo "#define HAS_SECURE_GETENV" >> s.h
+elif sh ./hasgot2 -D_GNU_SOURCE -i stdlib.h __secure_getenv; then
+  inf "__secure_getenv() found."
+  echo "#define HAS___SECURE_GETENV" >> s.h
 fi
 
 if sh ./hasgot -i unistd.h issetugid; then
diff --git a/man/ocamlc.m b/man/ocamlc.m
index 596b225c8e..4d76da9db9 100644
--- a/man/ocamlc.m
+++ b/man/ocamlc.m
@@ -315,6 +315,11 @@ Never use the
 command on executables produced by
 .BR ocamlc\ \-custom ,
 this would remove the bytecode part of the executable.
+
+Security warning: never set the "setuid" or "setgid" bits on
+executables produced by
+.BR ocamlc\ \-custom ,
+this would make them vulnerable to attacks.
 .TP
 .BI \-dllib\ \-l libname
 Arrange for the C shared library
diff --git a/manual/manual/cmds/unified-options.etex b/manual/manual/cmds/unified-options.etex
index 1af12e20c9..943b29590b 100644
--- a/manual/manual/cmds/unified-options.etex
+++ b/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
+produced by "ocamlc -custom", this would make them vulnerable to attacks.
+\end{unix}
 }%comp
 
 \comp{
diff --git a/otherlibs/unix/unix.mli b/otherlibs/unix/unix.mli
index 75121cf726..e414be00b3 100644
--- a/otherlibs/unix/unix.mli
+++ b/otherlibs/unix/unix.mli
@@ -138,9 +138,10 @@ val unsafe_getenv : string -> string
 
    Unlike {!getenv}, this function returns the value even if the
    process has special privileges. It is considered unsafe because the
-   programmer of a setuid program must be careful to prevent execution
-   of arbitrary commands through manipulation of the environment
-   variables.
+   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.
 
    @raise Not_found if the variable is unbound.  *)
 *)
diff --git a/otherlibs/unix/unixLabels.mli b/otherlibs/unix/unixLabels.mli
index c6289cab80..f1e68061f7 100644
--- a/otherlibs/unix/unixLabels.mli
+++ b/otherlibs/unix/unixLabels.mli
@@ -133,9 +133,10 @@ val unsafe_getenv : string -> string
 
    Unlike {!getenv}, this function returns the value even if the
    process has special privileges. It is considered unsafe because the
-   programmer of a setuid program must be careful to prevent execution
-   of arbitrary commands through manipulation of the environment
-   variables.
+   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.
 
    @raise Not_found if the variable is unbound.  *)
 *)