bug#45198: 28.0.50; Sandbox mode

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

Package: Emacs
Version: 28.0.50


Currently we cannot enable `flymake-mode` by default in `elisp-mode`
for simple reasons of security: having this mode enabled means that
whenever we happen to open a file containing ELisp code, our
Emacs will happily try to compile this file, including evaluating
arbitrary code contained in its macros.

Similarly, elpa.gnu.org can't automatically take a documentation file in
Org format and convert it to Texinfo (and then Info) when generating
a package's tarball, because the Org -> Texinfo conversion can run
arbitrary code.

To try and address these problems I suggest the function `sandbox-enter`
in the patch below, which should let us run untrusted ELisp code safely
(in an Emacs subprocess).

The patch for `elisp-flymake-byte-compile` is just there to give an idea
of how I expect it to work: currently the compiler will still try to write
the result of its compilation, which will fail because we're now
sandboxed, so it needs more work before it behaves like it should.

One thing I'm particularly eager to hear your opinion about is whether
there might be more holes to plug (i.e. more places where we need to
call `ensure_no_sandbox`).  Clearly, from a security perspective, this is
the main drawback of this approach: it's based on a black list rather
than on a whitelist.  Still, I have the impression that it should
be manageable.


        Stefan


diff --git a/etc/NEWS b/etc/NEWS
index 514209516d..9a569b4bd2 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -85,6 +85,16 @@ useful on systems such as FreeBSD which ships only with "etc/termcap".
 \f
 * Changes in Emacs 28.1
 
+** Sandbox mode to run untrusted code.
+The new function 'sandbox-enter' puts Emacs mode into a state in which
+it can (hopefully) run untrusted code safely.  This mode is restricted such
+that Emacs cannot exit the mode, nor can it write to files or launch
+new processes.  It can still read arbitrary files and communicate over
+pre-existing communication links.  This can only be used in batch mode.
+The expected use is to launch a new batch Emacs session, put it
+into sandbox mode, then run the untrusted code, send back the
+result via 'message', and then exit.
+
 ** Minibuffer scrolling is now conservative by default.
 This is controlled by the new variable 'scroll-minibuffer-conservatively'.
 
diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index fa360a8f3f..189ee896b6 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -1839,6 +1839,7 @@ elisp-flymake--batch-compile-for-flymake
             (push (list string position fill level)
                   collected)
             t)))
+    (sandbox-enter)     ;FIXME: this will break the subsequent delete-file!
     (unwind-protect
         (byte-compile-file file)
       (ignore-errors
diff --git a/src/emacs-module.c b/src/emacs-module.c
index 0f3ef59fd8..1bebdfeb4a 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -1091,6 +1091,7 @@ DEFUN ("module-load", Fmodule_load, Smodule_load, 1, 1, 0,
        doc: /* Load module FILE.  */)
   (Lisp_Object file)
 {
+  ensure_no_sandbox ();
   dynlib_handle_ptr handle;
   emacs_init_function module_init;
   void *gpl_sym;
@@ -1151,6 +1152,7 @@ DEFUN ("module-load", Fmodule_load, Smodule_load, 1, 1, 0,
 Lisp_Object
 funcall_module (Lisp_Object function, ptrdiff_t nargs, Lisp_Object *arglist)
 {
+  ensure_no_sandbox ();
   const struct Lisp_Module_Function *func = XMODULE_FUNCTION (function);
   eassume (0 <= func->min_arity);
   if (! (func->min_arity <= nargs
diff --git a/src/emacs.c b/src/emacs.c
index 2a32083ba1..0df70ae43e 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -139,6 +139,8 @@ #define MAIN_PROGRAM
 struct gflags gflags;
 bool initialized;
 
+bool emacs_is_sandboxed;
+
 /* If true, Emacs should not attempt to use a window-specific code,
    but instead should use the virtual terminal under which it was started.  */
 bool inhibit_window_system;
@@ -2886,6 +2888,30 @@ DEFUN ("daemon-initialized", Fdaemon_initialized, Sdaemon_initialized, 0, 0, 0,
   return Qt;
 }
 
+DEFUN ("sandbox-enter", Fsandbox_enter, Ssandbox_enter, 0, 0, 0,
+  doc: /* Put Emacs in sandboxed mode.
+After calling this function, Emacs cannot have externally visible
+side effects any more.  For example, it will not be able to write to files,
+create new processes, open new displays, or call functionality provided by
+modules, ...
+It can still send data to pre-existing processes, on the other hand.
+There is no mechanism to exit sandboxed mode.  */)
+  (void)
+{
+  if (!noninteractive)
+    /* We could allow a sandbox in interactive sessions, but it seems
+       useless, so we'll assume that it was a pilot-error.  */
+    error ("Can't enter sandbox in interactive session");
+  emacs_is_sandboxed = true;
+  return Qnil;
+}
+
+void ensure_no_sandbox (void)
+{
+  if (emacs_is_sandboxed)
+    error ("Sandboxed");
+}
+
 void
 syms_of_emacs (void)
 {
@@ -2906,6 +2932,7 @@ syms_of_emacs (void)
   defsubr (&Sinvocation_directory);
   defsubr (&Sdaemonp);
   defsubr (&Sdaemon_initialized);
+  defsubr (&Ssandbox_enter);
 
   DEFVAR_LISP ("command-line-args", Vcommand_line_args,
 	       doc: /* Args passed by shell to Emacs, as a list of strings.
diff --git a/src/fileio.c b/src/fileio.c
index 702c143828..e221048493 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -689,6 +689,7 @@ DEFUN ("make-temp-file-internal", Fmake_temp_file_internal,
   (Lisp_Object prefix, Lisp_Object dir_flag, Lisp_Object suffix,
    Lisp_Object text)
 {
+  ensure_no_sandbox ();
   CHECK_STRING (prefix);
   CHECK_STRING (suffix);
   Lisp_Object encoded_prefix = ENCODE_FILE (prefix);
@@ -2043,6 +2044,7 @@ DEFUN ("copy-file", Fcopy_file, Scopy_file, 2, 6,
    Lisp_Object keep_time, Lisp_Object preserve_uid_gid,
    Lisp_Object preserve_permissions)
 {
+  ensure_no_sandbox ();
   Lisp_Object handler;
   ptrdiff_t count = SPECPDL_INDEX ();
   Lisp_Object encoded_file, encoded_newname;
@@ -2301,6 +2303,7 @@ DEFUN ("make-directory-internal", Fmake_directory_internal,
        doc: /* Create a new directory named DIRECTORY.  */)
   (Lisp_Object directory)
 {
+  ensure_no_sandbox ();
   const char *dir;
   Lisp_Object handler;
   Lisp_Object encoded_dir;
@@ -2327,6 +2330,7 @@ DEFUN ("delete-directory-internal", Fdelete_directory_internal,
        doc: /* Delete the directory named DIRECTORY.  Does not follow symlinks.  */)
   (Lisp_Object directory)
 {
+  ensure_no_sandbox ();
   const char *dir;
   Lisp_Object encoded_dir;
 
@@ -2356,6 +2360,7 @@ DEFUN ("delete-file", Fdelete_file, Sdelete_file, 1, 2,
 With a prefix argument, TRASH is nil.  */)
   (Lisp_Object filename, Lisp_Object trash)
 {
+  ensure_no_sandbox ();
   Lisp_Object handler;
   Lisp_Object encoded_file;
 
@@ -2494,6 +2499,7 @@ DEFUN ("rename-file", Frename_file, Srename_file, 2, 3,
 This is what happens in interactive use with M-x.  */)
   (Lisp_Object file, Lisp_Object newname, Lisp_Object ok_if_already_exists)
 {
+  ensure_no_sandbox ();
   Lisp_Object handler;
   Lisp_Object encoded_file, encoded_newname;
 
@@ -2614,6 +2620,7 @@ DEFUN ("add-name-to-file", Fadd_name_to_file, Sadd_name_to_file, 2, 3,
 This is what happens in interactive use with M-x.  */)
   (Lisp_Object file, Lisp_Object newname, Lisp_Object ok_if_already_exists)
 {
+  ensure_no_sandbox ();
   Lisp_Object handler;
   Lisp_Object encoded_file, encoded_newname;
 
@@ -2667,6 +2674,7 @@ DEFUN ("make-symbolic-link", Fmake_symbolic_link, Smake_symbolic_link, 2, 3,
 This happens for interactive use with M-x.  */)
   (Lisp_Object target, Lisp_Object linkname, Lisp_Object ok_if_already_exists)
 {
+  ensure_no_sandbox ();
   Lisp_Object handler;
   Lisp_Object encoded_target, encoded_linkname;
 
@@ -3176,6 +3184,7 @@ DEFUN ("set-file-selinux-context", Fset_file_selinux_context,
 or if Emacs was not compiled with SELinux support.  */)
   (Lisp_Object filename, Lisp_Object context)
 {
+  ensure_no_sandbox ();
   Lisp_Object absname;
   Lisp_Object handler;
 #if HAVE_LIBSELINUX
@@ -3307,6 +3316,7 @@ DEFUN ("set-file-acl", Fset_file_acl, Sset_file_acl,
 support.  */)
   (Lisp_Object filename, Lisp_Object acl_string)
 {
+  ensure_no_sandbox ();
 #if USE_ACL
   Lisp_Object absname;
   Lisp_Object handler;
@@ -3392,6 +3402,7 @@ DEFUN ("set-file-modes", Fset_file_modes, Sset_file_modes, 2, 3,
 symbolic notation, like the `chmod' command from GNU Coreutils.  */)
   (Lisp_Object filename, Lisp_Object mode, Lisp_Object flag)
 {
+  ensure_no_sandbox ();
   CHECK_FIXNUM (mode);
   int nofollow = symlink_nofollow_flag (flag);
   Lisp_Object absname = Fexpand_file_name (filename,
@@ -3458,6 +3469,7 @@ DEFUN ("set-file-times", Fset_file_times, Sset_file_times, 1, 3, 0,
 TIMESTAMP is in the format of `current-time'. */)
   (Lisp_Object filename, Lisp_Object timestamp, Lisp_Object flag)
 {
+  ensure_no_sandbox ();
   int nofollow = symlink_nofollow_flag (flag);
 
   struct timespec ts[2];
@@ -5039,6 +5051,7 @@ DEFUN ("write-region", Fwrite_region, Swrite_region, 3, 7,
   (Lisp_Object start, Lisp_Object end, Lisp_Object filename, Lisp_Object append,
    Lisp_Object visit, Lisp_Object lockname, Lisp_Object mustbenew)
 {
+  ensure_no_sandbox ();
   return write_region (start, end, filename, append, visit, lockname, mustbenew,
 		       -1);
 }
@@ -5837,6 +5850,7 @@ DEFUN ("do-auto-save", Fdo_auto_save, Sdo_auto_save, 0, 2, "",
 A non-nil CURRENT-ONLY argument means save only current buffer.  */)
   (Lisp_Object no_message, Lisp_Object current_only)
 {
+  ensure_no_sandbox ();
   struct buffer *old = current_buffer, *b;
   Lisp_Object tail, buf, hook;
   bool auto_saved = 0;
diff --git a/src/lisp.h b/src/lisp.h
index e83304462f..107a2d9f03 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -603,6 +603,10 @@ #define ENUM_BF(TYPE) enum TYPE
    subsequent starts.  */
 extern bool initialized;
 
+/* Whether we've been neutralized.  */
+extern bool emacs_is_sandboxed;
+extern void ensure_no_sandbox (void);
+
 extern struct gflags
 {
   /* True means this Emacs instance was born to dump.  */
diff --git a/src/process.c b/src/process.c
index 4fe8ac7fc0..68460868c4 100644
--- a/src/process.c
+++ b/src/process.c
@@ -862,6 +862,8 @@ allocate_pty (char pty_name[PTY_NAME_SIZE])
 static struct Lisp_Process *
 allocate_process (void)
 {
+  ensure_no_sandbox ();
+
   return ALLOCATE_ZEROED_PSEUDOVECTOR (struct Lisp_Process, thread,
 				       PVEC_PROCESS);
 }
diff --git a/src/xdisp.c b/src/xdisp.c
index 96dd4fade2..72c37e8194 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -15442,6 +15442,10 @@ #define RESUME_POLLING					\
 static void
 redisplay_internal (void)
 {
+  /* Not sure if it's important to avoid redisplay inside a sandbox,
+     but it seems safer to avoid risking introducing security holes via
+     image libraries and such.  */
+  ensure_no_sandbox ();
   struct window *w = XWINDOW (selected_window);
   struct window *sw;
   struct frame *fr;
diff --git a/src/xterm.c b/src/xterm.c
index 3de0d2e73c..e8a4d2f29d 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -12698,6 +12698,7 @@ x_term_init (Lisp_Object display_name, char *xrm_option, char *resource_name)
   xcb_connection_t *xcb_conn;
 #endif
 
+  ensure_no_sandbox ();
   block_input ();
 
   if (!x_initialized)

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 12 Dec 2020 13:01:04 -0500
> Cc: Bastien <bzg@gnu.org>,
>  João Távora <joaotavora@gmail.com>
> 
>  static void
>  redisplay_internal (void)
>  {
> +  /* Not sure if it's important to avoid redisplay inside a sandbox,
> +     but it seems safer to avoid risking introducing security holes via
> +     image libraries and such.  */
> +  ensure_no_sandbox ();

You cannot usefully call error from redisplay.

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

>>  static void
>>  redisplay_internal (void)
>>  {
>> +  /* Not sure if it's important to avoid redisplay inside a sandbox,
>> +     but it seems safer to avoid risking introducing security holes via
>> +     image libraries and such.  */
>> +  ensure_no_sandbox ();
>
> You cannot usefully call error from redisplay.

Hmm... but this is at the entrance to redisplay, so I though it should
still be safe at that point.  If it's a problem we can replace the above
with

    if (emacs_is_sandboxed)
      return;


-- Stefan

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 45198@debbugs.gnu.org,  bzg@gnu.org,  joaotavora@gmail.com
> Date: Sat, 12 Dec 2020 16:06:54 -0500
> 
> > You cannot usefully call error from redisplay.
> 
> Hmm... but this is at the entrance to redisplay, so I though it should
> still be safe at that point.  If it's a problem we can replace the above
> with
> 
>     if (emacs_is_sandboxed)
>       return;

Yes, I think this is what we should do in this case.

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

>> > You cannot usefully call error from redisplay.
>> Hmm... but this is at the entrance to redisplay, so I though it should
>> still be safe at that point.  If it's a problem we can replace the above
>> with
>>     if (emacs_is_sandboxed)
>>       return;
> Yes, I think this is what we should do in this case.

With the change I just installed into `master`, I can now get
`elisp-flymake-byte-compile` to use sandboxing successfully with the
revised patch below.

Besides the above change, I made the same change in `Fdo_auto_save`
(i.e. `do-auto-save` was made to just silently do nothing instead of
signaling an error since it seemed to be too much trouble to change its
callers to avoid calling it when sandboxed).

I'm still worried that there remain wide open security holes, tho.


        Stefan


diff --git a/etc/NEWS b/etc/NEWS
index 909473f4e7..ee51495ca0 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -85,6 +85,16 @@ useful on systems such as FreeBSD which ships only with "etc/termcap".
 \f
 * Changes in Emacs 28.1
 
+** Sandbox mode to run untrusted code.
+The new function 'sandbox-enter' puts Emacs mode into a state in which
+it can (hopefully) run untrusted code safely.  This mode is restricted such
+that Emacs cannot exit the mode, nor can it write to files or launch
+new processes.  It can still read arbitrary files and communicate over
+pre-existing communication links.  This can only be used in batch mode.
+The expected use is to launch a new batch Emacs session, put it
+into sandbox mode, then run the untrusted code, send back the
+result via 'message', and then exit.
+
 ** Minibuffer scrolling is now conservative by default.
 This is controlled by the new variable 'scroll-minibuffer-conservatively'.
 
diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index b7e0c45228..df839d20b9 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -1836,6 +1836,7 @@ elisp-flymake--batch-compile-for-flymake
             (push (list string position fill level)
                   collected)
             t)))
+    (sandbox-enter)
     (unwind-protect
         (byte-compile-file file)
       (ignore-errors
diff --git a/src/emacs-module.c b/src/emacs-module.c
index b7cd835c83..79ff2b6ce6 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -1091,6 +1091,7 @@ DEFUN ("module-load", Fmodule_load, Smodule_load, 1, 1, 0,
        doc: /* Load module FILE.  */)
   (Lisp_Object file)
 {
+  ensure_no_sandbox ();
   dynlib_handle_ptr handle;
   emacs_init_function module_init;
   void *gpl_sym;
@@ -1151,6 +1152,7 @@ DEFUN ("module-load", Fmodule_load, Smodule_load, 1, 1, 0,
 Lisp_Object
 funcall_module (Lisp_Object function, ptrdiff_t nargs, Lisp_Object *arglist)
 {
+  ensure_no_sandbox ();
   const struct Lisp_Module_Function *func = XMODULE_FUNCTION (function);
   eassume (0 <= func->min_arity);
   if (! (func->min_arity <= nargs
diff --git a/src/emacs.c b/src/emacs.c
index 2a32083ba1..0df70ae43e 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -139,6 +139,8 @@ #define MAIN_PROGRAM
 struct gflags gflags;
 bool initialized;
 
+bool emacs_is_sandboxed;
+
 /* If true, Emacs should not attempt to use a window-specific code,
    but instead should use the virtual terminal under which it was started.  */
 bool inhibit_window_system;
@@ -2886,6 +2888,30 @@ DEFUN ("daemon-initialized", Fdaemon_initialized, Sdaemon_initialized, 0, 0, 0,
   return Qt;
 }
 
+DEFUN ("sandbox-enter", Fsandbox_enter, Ssandbox_enter, 0, 0, 0,
+  doc: /* Put Emacs in sandboxed mode.
+After calling this function, Emacs cannot have externally visible
+side effects any more.  For example, it will not be able to write to files,
+create new processes, open new displays, or call functionality provided by
+modules, ...
+It can still send data to pre-existing processes, on the other hand.
+There is no mechanism to exit sandboxed mode.  */)
+  (void)
+{
+  if (!noninteractive)
+    /* We could allow a sandbox in interactive sessions, but it seems
+       useless, so we'll assume that it was a pilot-error.  */
+    error ("Can't enter sandbox in interactive session");
+  emacs_is_sandboxed = true;
+  return Qnil;
+}
+
+void ensure_no_sandbox (void)
+{
+  if (emacs_is_sandboxed)
+    error ("Sandboxed");
+}
+
 void
 syms_of_emacs (void)
 {
@@ -2906,6 +2932,7 @@ syms_of_emacs (void)
   defsubr (&Sinvocation_directory);
   defsubr (&Sdaemonp);
   defsubr (&Sdaemon_initialized);
+  defsubr (&Ssandbox_enter);
 
   DEFVAR_LISP ("command-line-args", Vcommand_line_args,
 	       doc: /* Args passed by shell to Emacs, as a list of strings.
diff --git a/src/fileio.c b/src/fileio.c
index 702c143828..509341351d 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -689,6 +689,7 @@ DEFUN ("make-temp-file-internal", Fmake_temp_file_internal,
   (Lisp_Object prefix, Lisp_Object dir_flag, Lisp_Object suffix,
    Lisp_Object text)
 {
+  ensure_no_sandbox ();
   CHECK_STRING (prefix);
   CHECK_STRING (suffix);
   Lisp_Object encoded_prefix = ENCODE_FILE (prefix);
@@ -2043,6 +2044,7 @@ DEFUN ("copy-file", Fcopy_file, Scopy_file, 2, 6,
    Lisp_Object keep_time, Lisp_Object preserve_uid_gid,
    Lisp_Object preserve_permissions)
 {
+  ensure_no_sandbox ();
   Lisp_Object handler;
   ptrdiff_t count = SPECPDL_INDEX ();
   Lisp_Object encoded_file, encoded_newname;
@@ -2301,6 +2303,7 @@ DEFUN ("make-directory-internal", Fmake_directory_internal,
        doc: /* Create a new directory named DIRECTORY.  */)
   (Lisp_Object directory)
 {
+  ensure_no_sandbox ();
   const char *dir;
   Lisp_Object handler;
   Lisp_Object encoded_dir;
@@ -2327,6 +2330,7 @@ DEFUN ("delete-directory-internal", Fdelete_directory_internal,
        doc: /* Delete the directory named DIRECTORY.  Does not follow symlinks.  */)
   (Lisp_Object directory)
 {
+  ensure_no_sandbox ();
   const char *dir;
   Lisp_Object encoded_dir;
 
@@ -2356,6 +2360,7 @@ DEFUN ("delete-file", Fdelete_file, Sdelete_file, 1, 2,
 With a prefix argument, TRASH is nil.  */)
   (Lisp_Object filename, Lisp_Object trash)
 {
+  ensure_no_sandbox ();
   Lisp_Object handler;
   Lisp_Object encoded_file;
 
@@ -2494,6 +2499,7 @@ DEFUN ("rename-file", Frename_file, Srename_file, 2, 3,
 This is what happens in interactive use with M-x.  */)
   (Lisp_Object file, Lisp_Object newname, Lisp_Object ok_if_already_exists)
 {
+  ensure_no_sandbox ();
   Lisp_Object handler;
   Lisp_Object encoded_file, encoded_newname;
 
@@ -2614,6 +2620,7 @@ DEFUN ("add-name-to-file", Fadd_name_to_file, Sadd_name_to_file, 2, 3,
 This is what happens in interactive use with M-x.  */)
   (Lisp_Object file, Lisp_Object newname, Lisp_Object ok_if_already_exists)
 {
+  ensure_no_sandbox ();
   Lisp_Object handler;
   Lisp_Object encoded_file, encoded_newname;
 
@@ -2667,6 +2674,7 @@ DEFUN ("make-symbolic-link", Fmake_symbolic_link, Smake_symbolic_link, 2, 3,
 This happens for interactive use with M-x.  */)
   (Lisp_Object target, Lisp_Object linkname, Lisp_Object ok_if_already_exists)
 {
+  ensure_no_sandbox ();
   Lisp_Object handler;
   Lisp_Object encoded_target, encoded_linkname;
 
@@ -3176,6 +3184,7 @@ DEFUN ("set-file-selinux-context", Fset_file_selinux_context,
 or if Emacs was not compiled with SELinux support.  */)
   (Lisp_Object filename, Lisp_Object context)
 {
+  ensure_no_sandbox ();
   Lisp_Object absname;
   Lisp_Object handler;
 #if HAVE_LIBSELINUX
@@ -3307,6 +3316,7 @@ DEFUN ("set-file-acl", Fset_file_acl, Sset_file_acl,
 support.  */)
   (Lisp_Object filename, Lisp_Object acl_string)
 {
+  ensure_no_sandbox ();
 #if USE_ACL
   Lisp_Object absname;
   Lisp_Object handler;
@@ -3392,6 +3402,7 @@ DEFUN ("set-file-modes", Fset_file_modes, Sset_file_modes, 2, 3,
 symbolic notation, like the `chmod' command from GNU Coreutils.  */)
   (Lisp_Object filename, Lisp_Object mode, Lisp_Object flag)
 {
+  ensure_no_sandbox ();
   CHECK_FIXNUM (mode);
   int nofollow = symlink_nofollow_flag (flag);
   Lisp_Object absname = Fexpand_file_name (filename,
@@ -3458,6 +3469,7 @@ DEFUN ("set-file-times", Fset_file_times, Sset_file_times, 1, 3, 0,
 TIMESTAMP is in the format of `current-time'. */)
   (Lisp_Object filename, Lisp_Object timestamp, Lisp_Object flag)
 {
+  ensure_no_sandbox ();
   int nofollow = symlink_nofollow_flag (flag);
 
   struct timespec ts[2];
@@ -5039,6 +5051,7 @@ DEFUN ("write-region", Fwrite_region, Swrite_region, 3, 7,
   (Lisp_Object start, Lisp_Object end, Lisp_Object filename, Lisp_Object append,
    Lisp_Object visit, Lisp_Object lockname, Lisp_Object mustbenew)
 {
+  ensure_no_sandbox ();
   return write_region (start, end, filename, append, visit, lockname, mustbenew,
 		       -1);
 }
@@ -5837,6 +5850,8 @@ DEFUN ("do-auto-save", Fdo_auto_save, Sdo_auto_save, 0, 2, "",
 A non-nil CURRENT-ONLY argument means save only current buffer.  */)
   (Lisp_Object no_message, Lisp_Object current_only)
 {
+  if (emacs_is_sandboxed)
+    return Qnil;
   struct buffer *old = current_buffer, *b;
   Lisp_Object tail, buf, hook;
   bool auto_saved = 0;
diff --git a/src/lisp.h b/src/lisp.h
index e83304462f..107a2d9f03 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -603,6 +603,10 @@ #define ENUM_BF(TYPE) enum TYPE
    subsequent starts.  */
 extern bool initialized;
 
+/* Whether we've been neutralized.  */
+extern bool emacs_is_sandboxed;
+extern void ensure_no_sandbox (void);
+
 extern struct gflags
 {
   /* True means this Emacs instance was born to dump.  */
diff --git a/src/process.c b/src/process.c
index 4fe8ac7fc0..68460868c4 100644
--- a/src/process.c
+++ b/src/process.c
@@ -862,6 +862,8 @@ allocate_pty (char pty_name[PTY_NAME_SIZE])
 static struct Lisp_Process *
 allocate_process (void)
 {
+  ensure_no_sandbox ();
+
   return ALLOCATE_ZEROED_PSEUDOVECTOR (struct Lisp_Process, thread,
 				       PVEC_PROCESS);
 }
diff --git a/src/xdisp.c b/src/xdisp.c
index 96dd4fade2..04972c248e 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -15442,6 +15442,11 @@ #define RESUME_POLLING					\
 static void
 redisplay_internal (void)
 {
+  /* Not sure if it's important to avoid redisplay inside a sandbox,
+     but it seems safer to avoid risking introducing security holes via
+     image libraries and such.  */
+  if (emacs_is_sandboxed)
+    return;
   struct window *w = XWINDOW (selected_window);
   struct window *sw;
   struct frame *fr;
diff --git a/src/xterm.c b/src/xterm.c
index 3de0d2e73c..e8a4d2f29d 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -12698,6 +12698,7 @@ x_term_init (Lisp_Object display_name, char *xrm_option, char *resource_name)
   xcb_connection_t *xcb_conn;
 #endif
 
+  ensure_no_sandbox ();
   block_input ();
 
   if (!x_initialized)

bug#45198: 28.0.50; Sandbox mode

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> > You cannot usefully call error from redisplay.
>>> Hmm... but this is at the entrance to redisplay, so I though it should
>>> still be safe at that point.  If it's a problem we can replace the above
>>> with
>>>     if (emacs_is_sandboxed)
>>>       return;
>> Yes, I think this is what we should do in this case.
>
> With the change I just installed into `master`, I can now get
> `elisp-flymake-byte-compile` to use sandboxing successfully with the
> revised patch below.

Fantastic!

> Besides the above change, I made the same change in `Fdo_auto_save`
> (i.e. `do-auto-save` was made to just silently do nothing instead of
> signaling an error since it seemed to be too much trouble to change its
> callers to avoid calling it when sandboxed).
>
> I'm still worried that there remain wide open security holes, tho.

First, I wouldn't worry that terribly.  This is certainly and
improvement.  I won't be bitten again like that time I accidentally
typed (delete-directory ".") at macroexpand time.

That said, as you said the whitelisting approach is the safest one.
It'd be nice if you we a way to identify system calls and block all by
default.  Then whitelist a bunch of calls (checking arguments).  Not
sure if this can be done portably/systematically, though.  Chroot also
comes to mind, but it's only for linux, right?

João

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

> I'm still worried that there remain wide open security holes, tho.

Yes, and we need defence in depth. In addition to the measures already taken in the patch:

1. Add crash_if_sandboxed() calls in low-level routines that do objectionable things such as opening files for writing, create network connections, spawn processes, do DNS lookups, etc.

2. Platform-specific restrictions. I'll add macOS sandboxing if nobody else does. For Linux there are several options, most a bit messy but possible to use: seccomp (with or without BFP), name spaces, ptrace, etc.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Sa., 12. Dez. 2020 um 20:40 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> One thing I'm particularly eager to hear your opinion about is whether
> there might be more holes to plug (i.e. more places where we need to
> call `ensure_no_sandbox`).  Clearly, from a security perspective, this is
> the main drawback of this approach: it's based on a black list rather
> than on a whitelist.  Still, I have the impression that it should
> be manageable.

I don't think such an approach can work. It assumes perfect knowledge
about anything that might be problematic, and also assumes that all
future changes to Emacs take the sandbox question into account.
Especially the latter point seems unrealistic, and this looks like a
security incident waiting to happen.
Sandboxing is good, but it should happen using an allowlist and
established technology, such as firejail/bubblewrap/Google sandboxed
API/...

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am So., 13. Dez. 2020 um 05:26 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
> I'm still worried that there remain wide open security holes, tho.

Yes, this is still essentially arbitrary RCE.
Running untrusted code needs defence in depth.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am So., 13. Dez. 2020 um 16:32 Uhr schrieb Mattias Engdegård <mattiase@acm.org>:

>  For Linux there are several options, most a bit messy but possible to use: seccomp (with or without BFP), name spaces, ptrace, etc.


Fortunately there are quite a few good wrappers for these: firejail,
bubblewrap, nsjail, minijail, Google Sandbox2.
https://developers.google.com/sandboxed-api/docs/sandbox-overview has
a good overview.

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

> I don't think such an approach can work. It assumes perfect knowledge
> about anything that might be problematic, and also assumes that all
> future changes to Emacs take the sandbox question into account.
> Especially the latter point seems unrealistic, and this looks like a
> security incident waiting to happen.

That's true for the implementation side.
How 'bout the ELisp API side?

> Sandboxing is good, but it should happen using an allowlist and
> established technology, such as firejail/bubblewrap/Google sandboxed
> API/...

I'm all for it, *but*:
- I suspect we'll still want to use the extra "manual" checks I put in
  my code (so as to get clean ELisp errors when bumping against the
  walls of the sandbox, and because of the added in-depth security).
- This will need someone else doing the implementation.
- The ELisp-level API should not depend on the specific implementation
  too much, since none of those established technologies sound like
  things that'll still be maintained 10 years from now.
- We need to have this in Emacs-28 if we want to enable flymake-mode in
  ELisp by default in Emacs-28 (which I sure would like to do).
- I'd like to have this yesterday in order to build the Info files of
  GNU&NonGNU ELPA packages from their .org documentation without having
  to store the Info in the Git branch nor having to maintain some LXC
  container just for that.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am So., 13. Dez. 2020 um 18:57 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> > I don't think such an approach can work. It assumes perfect knowledge
> > about anything that might be problematic, and also assumes that all
> > future changes to Emacs take the sandbox question into account.
> > Especially the latter point seems unrealistic, and this looks like a
> > security incident waiting to happen.
>
> That's true for the implementation side.
> How 'bout the ELisp API side?
>
> > Sandboxing is good, but it should happen using an allowlist and
> > established technology, such as firejail/bubblewrap/Google sandboxed
> > API/...

The sandboxing technologies I'm aware of are process-based (because
Linux namespaces and kernel syscall filters are per-process), so a
"start sandbox from here" function likely can't be implemented. The
interface should rather be something like

(defun run-in-sandbox (form)
  (eql 0 (call-process "bwrap" ... "emacs" "--quick" "--batch" (format
"--eval=%S" form))))

(Maybe with some async variant and the opportunity to return some
result, like emacs-async.)

>
> I'm all for it, *but*:
> - I suspect we'll still want to use the extra "manual" checks I put in
>   my code (so as to get clean ELisp errors when bumping against the
>   walls of the sandbox, and because of the added in-depth security).

That's reasonable, though I'm worried that it will give users a false
sense of security.

> - This will need someone else doing the implementation.

Looks like we already have a volunteer for macOS.
For Linux, this shouldn't be that difficult either. The sandbox needs
to install a mount namespace that only allows read access to Emacs's
installation directory plus any input file and write access to known
output files, and enable syscall filters that forbid everything except
a list of known-safe syscalls (especially exec). I can take a stab at
that, but I can't promise anything ;-)

> - The ELisp-level API should not depend on the specific implementation
>   too much, since none of those established technologies sound like
>   things that'll still be maintained 10 years from now.

Yes, I'd imagine the API to be something like

;; Returns an opaque "sandbox" object.
(cl-defun start-sandbox (form &key input-files output-files) ...)
(defun wait-for-sandbox (sandbox) ...)
That would allow for some extensibility and also asynchronicity. The
API should fail if it can't establish a sandbox (e.g. if none of the
sandbox technologies are installed).

> - We need to have this in Emacs-28 if we want to enable flymake-mode in
>   ELisp by default in Emacs-28 (which I sure would like to do).

I guess having it in Emacs 28 is realistic, unless that is going to be
feature-frozen soon.

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

> The sandboxing technologies I'm aware of are process-based (because
> Linux namespaces and kernel syscall filters are per-process), so a
> "start sandbox from here" function likely can't be implemented. The
> interface should rather be something like
>
> (defun run-in-sandbox (form)
>   (eql 0 (call-process "bwrap" ... "emacs" "--quick" "--batch" (format
> "--eval=%S" form))))
>
> (Maybe with some async variant and the opportunity to return some
> result, like emacs-async.)

Thanks.  We definitely want some output, otherwise there's no point in
running `form`, right?

Also, I think the async option is the most important one.  How 'bout:

    (sandbox-start FUNCTION)

    Lunch a sandboxed Emacs subprocess running FUNCTION.
    Returns a process object.
    FUNCTION is called with no arguments and it can use `sandbox-read`
    to read the data sent to the process object via `process-send-string`,
    and `sandbox-reply` to send back a reply to the parent process
    (which will receive it via its `process-filter`).
    The sandboxed process has read access to all the local files
    but no write access to them, nor any access to the network or
    the display.

WDYT?

>> - I suspect we'll still want to use the extra "manual" checks I put in
>>   my code (so as to get clean ELisp errors when bumping against the
>>   walls of the sandbox, and because of the added in-depth security).
> That's reasonable, though I'm worried that it will give users a false
> sense of security.

That would only be the case if we don't additionally use process-level
isolation, right?

>> - This will need someone else doing the implementation.
> Looks like we already have a volunteer for macOS.
> For Linux, this shouldn't be that difficult either. The sandbox needs
> to install a mount namespace that only allows read access to Emacs's
> installation directory plus any input file and write access to known
> output files, and enable syscall filters that forbid everything except
> a list of known-safe syscalls (especially exec). I can take a stab at
> that, but I can't promise anything ;-)

Looking forward to it.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

>   (eql 0 (call-process "bwrap" ... "emacs" "--quick" "--batch" (format
> "--eval=%S" form))))

BTW, thanks for this reference to bwrap.  It looks like I should be able
to use it for my immediate needs in (Non)GNU ELPA scripting.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I don't think such an approach can work. It assumes perfect knowledge
>> about anything that might be problematic, and also assumes that all
>> future changes to Emacs take the sandbox question into account.
>> Especially the latter point seems unrealistic, and this looks like a
>> security incident waiting to happen.
>
> That's true for the implementation side.
> How 'bout the ELisp API side?

That's well pointed out.  Why can't we just put the gate in the default
expansion of the C DEFUN macro?  There are only so many DEFUN's.  Then
the whitelisting could proceed from there.  DEFUN's are rarely added,
and they would be forbidden in sandbox mode by default.

João

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am So., 13. Dez. 2020 um 19:43 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> > The sandboxing technologies I'm aware of are process-based (because
> > Linux namespaces and kernel syscall filters are per-process), so a
> > "start sandbox from here" function likely can't be implemented. The
> > interface should rather be something like
> >
> > (defun run-in-sandbox (form)
> >   (eql 0 (call-process "bwrap" ... "emacs" "--quick" "--batch" (format
> > "--eval=%S" form))))
> >
> > (Maybe with some async variant and the opportunity to return some
> > result, like emacs-async.)
>
> Thanks.  We definitely want some output, otherwise there's no point in
> running `form`, right?

In some way certainly, but it's not necessarily through stdout. I tend
to write potential output into a file whose filename is passed on the
command line. That's more robust than stdout (which often contains
spurious messages about loading files etc.).

>
> Also, I think the async option is the most important one.  How 'bout:
>
>     (sandbox-start FUNCTION)
>
>     Lunch a sandboxed Emacs subprocess running FUNCTION.

Passing a function here might be confusing because e.g. lexical
closures won't work. It might be preferable to pass a form and state
that both dynamic and lexical bindings are ignored.

>     Returns a process object.

Depending how much we care about forward compatibility, it might be
better to return an opaque sandbox object (which will initially wrap a
process object).

>     FUNCTION is called with no arguments and it can use `sandbox-read`
>     to read the data sent to the process object via `process-send-string`,
>     and `sandbox-reply` to send back a reply to the parent process
>     (which will receive it via its `process-filter`).

That is, sandbox-read and sandbox-reply just read/write stdin/stdout?
That would certainly work, but (a) it doesn't really have anything to
do with sandboxing, so these functions should rather be called
stdin-read and stdout-write or similar, and (b) especially in the
stdout case might be too brittle because Emacs likes to print
arbitrary stuff on stdout/stderr.
Alternatively, the sandbox could use dedicated files or pipes to communicate.

>     The sandboxed process has read access to all the local files
>     but no write access to them, nor any access to the network or
>     the display.

This might be a bit too specific. I'd imagine we'd want to restrict
reading files to the absolute minimum (files that Emacs itself needs
plus a fixed set of input files/directories known in advance), but
often allow writing some output files.

>
> WDYT?

I think the interface is mostly OK, but I think we want to restrict
the set of allowed input/output files.

>
> >> - I suspect we'll still want to use the extra "manual" checks I put in
> >>   my code (so as to get clean ELisp errors when bumping against the
> >>   walls of the sandbox, and because of the added in-depth security).
> > That's reasonable, though I'm worried that it will give users a false
> > sense of security.
>
> That would only be the case if we don't additionally use process-level
> isolation, right?

My worry is that people see a function like enter-sandbox and then
assume that Emacs will be secure after calling it, without actually
verifying the security implications.

>
> >> - This will need someone else doing the implementation.
> > Looks like we already have a volunteer for macOS.
> > For Linux, this shouldn't be that difficult either. The sandbox needs
> > to install a mount namespace that only allows read access to Emacs's
> > installation directory plus any input file and write access to known
> > output files, and enable syscall filters that forbid everything except
> > a list of known-safe syscalls (especially exec). I can take a stab at
> > that, but I can't promise anything ;-)
>
> Looking forward to it.
>

I've looked into this, and what I'd suggest for now is:
1. Add a --seccomp=FILE command-line option that loads seccomp filters
from FILE and applies them directly after startup (first thing in
main). Why do this in Emacs? Because that's the easiest way to prevent
execve. When installing a seccomp filter in a separate process, execve
needs to be allowed because otherwise there'd be no way to execute the
Emacs binary. While there are workarounds (ptrace, LD_PRELOAD), it's
easiest to install the seccomp filter directly in the Emacs process.
2. Generate appropriate seccomp filters using libseccomp or similar.
3. In the sandboxing functions, start Emacs with bwrap to set up
namespaces and invoke Emacs with the new --seccomp flag.

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

> The sandboxing technologies I'm aware of are process-based (because Linux namespaces and kernel syscall filters are per-process), so a "start sandbox from here" function likely can't be implemented. The interface should rather be something like 

If you mean that the sandbox needs to be active from the very start of the process, I don't see why that has to be the case. It does not appear to be necessary for macOS, OpenBSD or FreeBSD, nor for at least some the Linux options I'm aware of.

Perhaps I misunderstood, and there may indeed be some desirable sandboxing methods that require from-exec sandboxing. It is often useful to allow for a set-up period prior to activating restrictions allowing for specific files to be opened and so on and can make the sandboxing itself simpler by being less selective.

From-exec sandboxing also precludes using simple forking (without exec) as a cheap way to start the Emacs subprocess (if somewhat Unix-specific).

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Mo., 14. Dez. 2020 um 12:12 Uhr schrieb Mattias Engdegård <mattiase@acm.org>:
>
> > The sandboxing technologies I'm aware of are process-based (because Linux namespaces and kernel syscall filters are per-process), so a "start sandbox from here" function likely can't be implemented. The interface should rather be something like
>
> If you mean that the sandbox needs to be active from the very start of the process, I don't see why that has to be the case. It does not appear to be necessary for macOS, OpenBSD or FreeBSD, nor for at least some the Linux options I'm aware of.

Yes, it's not strictly required (as in, seccomp and unshare nominally
work at any point), though I think enabling sandboxing while user code
has already run can have confusing/unanticipated consequences. For
example, other threads might already be running in parallel, and they
would then suddenly be blocked from making some syscalls, potentially
in the middle of a critical section or similar. I'd expect that
there's lots of code around that doesn't expect syscalls to suddenly
start failing. Other sandboxing technologies like unsharing the user
namespace also don't work in multithreaded processes. Allowing
sandboxing (at least for now) only at process startup avoids such
issues and should be good enough for the given use case (Flymake
background compilation), since we need to start a new process anyway.

>
> Perhaps I misunderstood, and there may indeed be some desirable sandboxing methods that require from-exec sandboxing. It is often useful to allow for a set-up period prior to activating restrictions allowing for specific files to be opened and so on and can make the sandboxing itself simpler by being less selective.

That is true, though it would require more significant changes to
Emacs. For example, to achieve some amount of capability-based
security, you'd open files before sandboxing and then forbid the open
syscall, but that's not really possible with the current Emacs API
(which doesn't provide any access to open files).

>
> From-exec sandboxing also precludes using simple forking (without exec) as a cheap way to start the Emacs subprocess (if somewhat Unix-specific).
>

Even on Unix, a fork that's not immediately followed by an exec or
exit tends to not work any more. Lots of libraries nowadays assume
that the "weird in-between state" after a fork doesn't exist
permanently, and only a small number of async-signal-safe syscalls are
guaranteed to work between fork and exec.

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

> In some way certainly, but it's not necessarily through stdout. I tend
> to write potential output into a file whose filename is passed on the
> command line. That's more robust than stdout (which often contains
> spurious messages about loading files etc).

Hmm... but that requires write access to some part of the file system,
so it requires a finer granularity of control.  I'd much rather limit
the output to something equivalent to a pipe (the implementation of the
sandboxing doesn't have to use stdout for that if that's a problem).

>> Also, I think the async option is the most important one.  How 'bout:
>>     (sandbox-start FUNCTION)
>>     Lunch a sandboxed Emacs subprocess running FUNCTION.
> Passing a function here might be confusing because e.g. lexical
> closures won't work.

What makes you think they don't?

> It might be preferable to pass a form and state
> that both dynamic and lexical bindings are ignored.

If closures turn out to be a problem I'd rather use FUNCTION + VALUES
than FORM (using FORM implies the use of `eval`, and you have to think
of all those kitten that'll suffer if we do that).

>>     Returns a process object.
> Depending how much we care about forward compatibility, it might be
> better to return an opaque sandbox object (which will initially wrap a
> process object).

We always use process objects to represent file-descriptors, so I can't
find any good reason why this one should be different or why an
implementation might find it difficult to expose a process object.

>>     FUNCTION is called with no arguments and it can use `sandbox-read`
>>     to read the data sent to the process object via `process-send-string`,
>>     and `sandbox-reply` to send back a reply to the parent process
>>     (which will receive it via its `process-filter`).
> That is, sandbox-read and sandbox-reply just read/write stdin/stdout?

While it may use stdin/stdout internally, I can imagine good reasons why
we'd want to use some other file descriptors.

> That would certainly work, but (a) it doesn't really have anything to
> do with sandboxing, so these functions should rather be called
> stdin-read and stdout-write or similar,

I think "the right thing" would be to represent the parent as a process
object inside the child.  I proposed dedicated functions only because
but when it uses stdin/stdout, providing a process object seems awkward
to implement.

>>     The sandboxed process has read access to all the local files
>>     but no write access to them, nor any access to the network or
>>     the display.
> This might be a bit too specific. I'd imagine we'd want to restrict
> reading files to the absolute minimum (files that Emacs itself needs
> plus a fixed set of input files/directories known in advance), but
> often allow writing some output files.

I'm trying to design an API which can be made to work in as many
circumstances as possible without imposing too high a maintenance
burden.  So while I agree that it'd be better to limit the set of files
that can be read and to allow writing to some files, I think I'd rather
start with something more crude.

We can refine it later if/when we have more experience with how it's
used, and how it's implemented in the various OSes.

>> >> - I suspect we'll still want to use the extra "manual" checks I put in
>> >>   my code (so as to get clean ELisp errors when bumping against the
>> >>   walls of the sandbox, and because of the added in-depth security).
>> > That's reasonable, though I'm worried that it will give users a false
>> > sense of security.
>> That would only be the case if we don't additionally use process-level
>> isolation, right?
> My worry is that people see a function like enter-sandbox and then
> assume that Emacs will be secure after calling it, without actually
> verifying the security implications.

This seems universally true and hence suggests we should just forget
about this idea of providing a sandbox functionality.  IOW I'm not sure
what this has to do with the `ensure_no_sandbox` calls I'm suggesting
we keep.

> I've looked into this, and what I'd suggest for now is:
> 1. Add a --seccomp=FILE command-line option that loads seccomp filters
> from FILE and applies them directly after startup (first thing in
> main). Why do this in Emacs? Because that's the easiest way to prevent
> execve. When installing a seccomp filter in a separate process, execve
> needs to be allowed because otherwise there'd be no way to execute the
> Emacs binary. While there are workarounds (ptrace, LD_PRELOAD), it's
> easiest to install the seccomp filter directly in the Emacs process.
> 2. Generate appropriate seccomp filters using libseccomp or similar.
> 3. In the sandboxing functions, start Emacs with bwrap to set up
> namespaces and invoke Emacs with the new --seccomp flag.

Sounds OK, tho I must say I don't understand why we care particularly
about disallowing execve inside the bwrap jail.  AFAIK anything that an
external process can do can also be done directly by Emacs since ELisp
is a fairly fully-featured language (since there's nothing like setuid
inside a bwrap jail).  I mean, I agree that we want to disallow running
subprocesses, but can't think of a good reason why we would need this to
be 100%, so we could rely on `ensure_no_sandbox` for that.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

> namespace also don't work in multithreaded processes. Allowing
> sandboxing (at least for now) only at process startup avoids such
> issues and should be good enough for the given use case (Flymake
> background compilation), since we need to start a new process anyway.

BTW, while flymake does already use a background process, I think
there's a deeper reason why we'll always need a separate process:
there's no way to exit a sandbox.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Mo., 14. Dez. 2020 um 15:44 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> > In some way certainly, but it's not necessarily through stdout. I tend
> > to write potential output into a file whose filename is passed on the
> > command line. That's more robust than stdout (which often contains
> > spurious messages about loading files etc).
>
> Hmm... but that requires write access to some part of the file system,
> so it requires a finer granularity of control.

When using mount namespaces, that comes at a low cost: you mount
exactly those files that you want to write as writable filesystems,
and read-only files as read-only.

> I'd much rather limit
> the output to something equivalent to a pipe (the implementation of the
> sandboxing doesn't have to use stdout for that if that's a problem).

It's definitely true that only accessing open file descriptors is more
secure and simpler ("capability-based security"). However, I assume
that generic Elisp code doesn't work too well if it can't do things
like create temporary files. I've found 211 calls to make-temp-file in
the Emacs codebase alone, and I'd be surprised if none of them were
applicable to sandboxed processes.

>
> >> Also, I think the async option is the most important one.  How 'bout:
> >>     (sandbox-start FUNCTION)
> >>     Lunch a sandboxed Emacs subprocess running FUNCTION.
> > Passing a function here might be confusing because e.g. lexical
> > closures won't work.
>
> What makes you think they don't?

Hmm, you mean printing and reading closure objects? It might work,
though I don't know how portable/robust it is.

>
> > It might be preferable to pass a form and state
> > that both dynamic and lexical bindings are ignored.
>
> If closures turn out to be a problem I'd rather use FUNCTION + VALUES
> than FORM (using FORM implies the use of `eval`, and you have to think
> of all those kitten that'll suffer if we do that).

It is always eval, because that's how Elisp works. How else would you
deserialize and execute code than with read + eval?

>
> >>     Returns a process object.
> > Depending how much we care about forward compatibility, it might be
> > better to return an opaque sandbox object (which will initially wrap a
> > process object).
>
> We always use process objects to represent file-descriptors, so I can't
> find any good reason why this one should be different or why an
> implementation might find it difficult to expose a process object.

That's a fair point, but when thinking about forwards-compatibility we
might want to anticipate reasons that we currently can't think of.

>
> >>     FUNCTION is called with no arguments and it can use `sandbox-read`
> >>     to read the data sent to the process object via `process-send-string`,
> >>     and `sandbox-reply` to send back a reply to the parent process
> >>     (which will receive it via its `process-filter`).
> > That is, sandbox-read and sandbox-reply just read/write stdin/stdout?
>
> While it may use stdin/stdout internally, I can imagine good reasons why
> we'd want to use some other file descriptors.

If the process can write to stdout, then users will do that. Users
would probably just try whether 'print' works, and use it if it does.
So if you want a specialized function, then we'd need to make sure
that 'print' doesn't work (e.g. by having the parent process only read
the new pipe).

>
> > That would certainly work, but (a) it doesn't really have anything to
> > do with sandboxing, so these functions should rather be called
> > stdin-read and stdout-write or similar,
>
> I think "the right thing" would be to represent the parent as a process
> object inside the child.  I proposed dedicated functions only because
> but when it uses stdin/stdout, providing a process object seems awkward
> to implement.

It would be a file descriptor in all cases, so we might as well use a
pipe process (and introduce a function to write to a pipe). Stdout
isn't really different from other open file descriptors.
We'd need some special support for other file descriptors though, as
we'd need to make sure to not open them with O_CLOEXEC.

>
> >>     The sandboxed process has read access to all the local files
> >>     but no write access to them, nor any access to the network or
> >>     the display.
> > This might be a bit too specific. I'd imagine we'd want to restrict
> > reading files to the absolute minimum (files that Emacs itself needs
> > plus a fixed set of input files/directories known in advance), but
> > often allow writing some output files.
>
> I'm trying to design an API which can be made to work in as many
> circumstances as possible without imposing too high a maintenance
> burden.  So while I agree that it'd be better to limit the set of files
> that can be read and to allow writing to some files, I think I'd rather
> start with something more crude.
>
> We can refine it later if/when we have more experience with how it's
> used, and how it's implemented in the various OSes.

Even without specific experience, we can definitely say that
restricting something later is much harder than lifting restrictions.
So if we'd start with a policy that allows full read access, we'd have
a very hard time restricting that later. IOW, I'd be fine with not
providing write access to files (with the exception of maybe a
process-local tmpfs), but I'd try to restrict reading right from the
start to the installation directory and other known sources like the
load path.

>
> >> >> - I suspect we'll still want to use the extra "manual" checks I put in
> >> >>   my code (so as to get clean ELisp errors when bumping against the
> >> >>   walls of the sandbox, and because of the added in-depth security).
> >> > That's reasonable, though I'm worried that it will give users a false
> >> > sense of security.
> >> That would only be the case if we don't additionally use process-level
> >> isolation, right?
> > My worry is that people see a function like enter-sandbox and then
> > assume that Emacs will be secure after calling it, without actually
> > verifying the security implications.
>
> This seems universally true and hence suggests we should just forget
> about this idea of providing a sandbox functionality.  IOW I'm not sure
> what this has to do with the `ensure_no_sandbox` calls I'm suggesting
> we keep.

Seccomp and namespaces are battle-tested kernel-level security
features. If we use them, we'll have a much better chance at providing
an actually secure sandbox.

>
> > I've looked into this, and what I'd suggest for now is:
> > 1. Add a --seccomp=FILE command-line option that loads seccomp filters
> > from FILE and applies them directly after startup (first thing in
> > main). Why do this in Emacs? Because that's the easiest way to prevent
> > execve. When installing a seccomp filter in a separate process, execve
> > needs to be allowed because otherwise there'd be no way to execute the
> > Emacs binary. While there are workarounds (ptrace, LD_PRELOAD), it's
> > easiest to install the seccomp filter directly in the Emacs process.
> > 2. Generate appropriate seccomp filters using libseccomp or similar.
> > 3. In the sandboxing functions, start Emacs with bwrap to set up
> > namespaces and invoke Emacs with the new --seccomp flag.
>
> Sounds OK, tho I must say I don't understand why we care particularly
> about disallowing execve inside the bwrap jail.  AFAIK anything that an
> external process can do can also be done directly by Emacs since ELisp
> is a fairly fully-featured language (since there's nothing like setuid
> inside a bwrap jail).  I mean, I agree that we want to disallow running
> subprocesses, but can't think of a good reason why we would need this to
> be 100%, so we could rely on `ensure_no_sandbox` for that.

It's basically just another way to make the attack surface smaller and
provide defense in depth. If we allow execve, then suddenly the attack
surface increases to include all possible binaries that Emacs might
execute.
I've implemented the --seccomp flag, and it's really benign - mapping
the input file plus one or two syscalls. It's also strictly optional
and doesn't preclude using other technologies.

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

14 dec. 2020 kl. 14.44 skrev Philipp Stephani <p.stephani2@gmail.com>:

> Yes, it's not strictly required (as in, seccomp and unshare nominally
> work at any point), though I think enabling sandboxing while user code
> has already run can have confusing/unanticipated consequences. For
> example, other threads might already be running in parallel, and they
> would then suddenly be blocked from making some syscalls, potentially
> in the middle of a critical section or similar.

There shouldn't be many threads running in non-interactive mode, and those that are must be expected to work with the added restrictions because why should they be exempt and what are they doing that we want to forbid anyway? It seems a bit far-fetched and probably not an immediate concern.

That said, it is very much an implementation matter -- the run-function-in-sandbox Lisp interface seems better than the original enter-sandbox because we get more ways to write the code. Thanks for proposing it!

> For example, to achieve some amount of capability-based
> security, you'd open files before sandboxing and then forbid the open
> syscall, but that's not really possible with the current Emacs API
> (which doesn't provide any access to open files).

Well, almost -- elisp processes serve some of the purposes of open file descriptors, at least for pipes and sockets.

Is it really is practical to restrict file-system visibility? A spawned byte-compiler will need to read almost arbitrary elisp files (autoload, 'require' calls) whose exact names are only known at runtime. Were you planning to build a name-space from a skeleton populated by load-path mounts?

My initial thought was simply inhibit pretty much everything except reading files and writing to already open descriptors (or just stdout/stderr), on the grounds that while it would enable an adversary to read anything, exfiltration would be difficult.

(Some side-channels may be worth thinking about: if the machine cannot trust its file servers, it is possible to exfiltrate data to an already compromised server merely by reading. But then there are probably more direct approaches.)

> Even on Unix, a fork that's not immediately followed by an exec or
> exit tends to not work any more. Lots of libraries nowadays assume
> that the "weird in-between state" after a fork doesn't exist
> permanently, and only a small number of async-signal-safe syscalls are
> guaranteed to work between fork and exec.

Yes, and I'm aware of the difficulties but wouldn't dismiss it out of hand since the gains are attractive. The main trouble stems from fork only bringing the calling thread into the new process, which may cause deadlock if those threads were holding locks which the forked process goes on to acquire later on. (pthread_atfork is supposed to be used by threaded libraries but typically isn't.)

It does work given some care (and I have done so in the past to good effect); it's mainly a matter of not touching anything that you don't want to use anyway such as GUI frameworks. In Emacs, this would be done in some sort of become_noninteractive function which ensures that future program flow will not involve any GUI code whatsoever.

Let's see what latency we get from spawning a typically overloaded Emacs configuration first.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Mo., 14. Dez. 2020 um 16:59 Uhr schrieb Mattias Engdegård <mattiase@acm.org>:
>
> 14 dec. 2020 kl. 14.44 skrev Philipp Stephani <p.stephani2@gmail.com>:
>
> > Yes, it's not strictly required (as in, seccomp and unshare nominally
> > work at any point), though I think enabling sandboxing while user code
> > has already run can have confusing/unanticipated consequences. For
> > example, other threads might already be running in parallel, and they
> > would then suddenly be blocked from making some syscalls, potentially
> > in the middle of a critical section or similar.
>
> There shouldn't be many threads running in non-interactive mode,

Dynamic libraries tend to start threads for background work, so while
there aren't that many, they still exist.

> and those that are must be expected to work with the added restrictions because why should they be exempt and what are they doing that we want to forbid anyway? It seems a bit far-fetched and probably not an immediate concern.

Libraries (and Emacs itself) can do arbitrary background processing as
an implementation detail, so we'd need to take that into account. I
agree though that this isn't a problem in practice, and, if at all,
requires some adjustments to the policy.
Just do give an example:
https://sourceware.org/git/?p=glibc.git;a=blob;f=nptl/pthread_create.c;h=bad4e57a845bd3148ad634acaaccbea08b04dbbd;hb=HEAD#l393
assumes that set_robust_list will work if it worked once. In the case
of an Emacs sandbox, threads are started before entering main (through
dynamic initialization and dynamic linking), so the function assumes
that set_robust_list works. (In that case we can just allow
set_robust_list as it's not dangerous.)

>
> That said, it is very much an implementation matter -- the run-function-in-sandbox Lisp interface seems better than the original enter-sandbox because we get more ways to write the code. Thanks for proposing it!

Sure, I also don't mind adding a load-seccomp-filter function for
post-main invocation. Right now I believe it's not needed though.

>
> > For example, to achieve some amount of capability-based
> > security, you'd open files before sandboxing and then forbid the open
> > syscall, but that's not really possible with the current Emacs API
> > (which doesn't provide any access to open files).
>
> Well, almost -- elisp processes serve some of the purposes of open file descriptors, at least for pipes and sockets.
>
> Is it really is practical to restrict file-system visibility? A spawned byte-compiler will need to read almost arbitrary elisp files (autoload, 'require' calls) whose exact names are only known at runtime. Were you planning to build a name-space from a skeleton populated by load-path mounts?

I haven't tried this out yet, but allowing reads from load-path
entries plus the installation directory should be fine.

>
> My initial thought was simply inhibit pretty much everything except reading files and writing to already open descriptors (or just stdout/stderr), on the grounds that while it would enable an adversary to read anything, exfiltration would be difficult.

Yes, but see my other comment: restricting an open policy after the
fact is much harder than opening up an initially-restrictive one, so
I'd really start with a restrictive one (no file reading allowed
except for allowed directories and files).

>
> (Some side-channels may be worth thinking about: if the machine cannot trust its file servers, it is possible to exfiltrate data to an already compromised server merely by reading. But then there are probably more direct approaches.)
>
> > Even on Unix, a fork that's not immediately followed by an exec or
> > exit tends to not work any more. Lots of libraries nowadays assume
> > that the "weird in-between state" after a fork doesn't exist
> > permanently, and only a small number of async-signal-safe syscalls are
> > guaranteed to work between fork and exec.
>
> Yes, and I'm aware of the difficulties but wouldn't dismiss it out of hand since the gains are attractive. The main trouble stems from fork only bringing the calling thread into the new process, which may cause deadlock if those threads were holding locks which the forked process goes on to acquire later on. (pthread_atfork is supposed to be used by threaded libraries but typically isn't.)

Yes, and because libraries can and do start arbitrary threads, this
issue can't really be mitigated and makes fork without exec extremely
unsafe and largely useless.
The gains are largely realized using threads these days.

>
> It does work given some care (and I have done so in the past to good effect); it's mainly a matter of not touching anything that you don't want to use anyway such as GUI frameworks. In Emacs, this would be done in some sort of become_noninteractive function which ensures that future program flow will not involve any GUI code whatsoever.

I don't think that's enough, even linking against some libraries
already causes background threads to spin up.

>
> Let's see what latency we get from spawning a typically overloaded Emacs configuration first.
>

I'd think that we'd always run the sandboxed Emacs with --quick
--batch and an empty environment (to provide for some reproducibility
and avoid LD_PRELOAD attacks etc.), and then startup tends to be fast
enough (emacs -Q -batch takes ~50 ms on my system).

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

17 dec. 2020 kl. 14.08 skrev Philipp Stephani <p.stephani2@gmail.com>:

> Dynamic libraries tend to start threads for background work, so while
> there aren't that many, they still exist.

Well, there's no accounting for taste. Still, I'm not ready to close the door to possible solutions until they really do appear to lead no way. (It's not an urgent concern since we will need a traditional fork-exec solution first of all.)

> I haven't tried this out yet, but allowing reads from load-path
> entries plus the installation directory should be fine.

Assuming this is sufficient; I think autoloaded definitions can specify files in arbitrary directories, not necessarily in the load-path.

> Yes, but see my other comment: restricting an open policy after the
> fact is much harder than opening up an initially-restrictive one, so
> I'd really start with a restrictive one (no file reading allowed
> except for allowed directories and files).

Depends on the platform I suppose -- macOS and BSD should work either way. On Linux it depends on the method used; I admit not having looked closely at seccomp lately.

> The gains are largely realized using threads these days.

Indeed, although forking still has a few niche uses. (For there record I'm a firm believer that the fork-exec model was a mistake from its inception, but now that it's there...)

Emacs would be better served with threads, too, if it weren't that (I) we don't have a good threading story yet and (II) Elisp code can cause way too much damage at compile time. Fixing either would bring many other benefits!

> I'd think that we'd always run the sandboxed Emacs with --quick
> --batch and an empty environment (to provide for some reproducibility
> and avoid LD_PRELOAD attacks etc.), and then startup tends to be fast
> enough (emacs -Q -batch takes ~50 ms on my system).

That's not quite fair; the byte-compiler needs the right load-path and autoload definitions, and the byte-compiler itself needs to be loaded as well. (Anyone who can set LD_PRELOAD already has the machine.)

The easiest way is to run the user's init file. Perhaps it's possible to just transmit a list of paths and packages to the subprocess as arguments but the user may have things loaded or defined outside the standard package manager.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Do., 17. Dez. 2020 um 18:56 Uhr schrieb Mattias Engdegård <mattiase@acm.org>:
>
> 17 dec. 2020 kl. 14.08 skrev Philipp Stephani <p.stephani2@gmail.com>:
>
> > Dynamic libraries tend to start threads for background work, so while
> > there aren't that many, they still exist.
>
> Well, there's no accounting for taste. Still, I'm not ready to close the door to possible solutions until they really do appear to lead no way. (It's not an urgent concern since we will need a traditional fork-exec solution first of all.)

Yeah, I didn't want to imply to close the door, just to first start
with what we need to solve the task at hand.

>
> > I haven't tried this out yet, but allowing reads from load-path
> > entries plus the installation directory should be fine.
>
> Assuming this is sufficient; I think autoloaded definitions can specify files in arbitrary directories, not necessarily in the load-path.

Sure, but things like Flymake byte-compilation are best-effort anyway.
It's fine to (at least initially) not cover a few "exotic" cases.

>
> > Yes, but see my other comment: restricting an open policy after the
> > fact is much harder than opening up an initially-restrictive one, so
> > I'd really start with a restrictive one (no file reading allowed
> > except for allowed directories and files).
>
> Depends on the platform I suppose -- macOS and BSD should work either way. On Linux it depends on the method used; I admit not having looked closely at seccomp lately.

Ah, I was talking about the engineering/product management aspect, not
about the technical one: If you start with an initially-open sandbox
policy, locking it down in future releases is much harder than the
other way round.

>
> > The gains are largely realized using threads these days.
>
> Indeed, although forking still has a few niche uses. (For there record I'm a firm believer that the fork-exec model was a mistake from its inception, but now that it's there...)
>
> Emacs would be better served with threads, too, if it weren't that (I) we don't have a good threading story yet and (II) Elisp code can cause way too much damage at compile time. Fixing either would bring many other benefits!

Yeah, and while there are a few ideas (the "web worker" model looks
most promising), we're not even close to having a design yet.

>
> > I'd think that we'd always run the sandboxed Emacs with --quick
> > --batch and an empty environment (to provide for some reproducibility
> > and avoid LD_PRELOAD attacks etc.), and then startup tends to be fast
> > enough (emacs -Q -batch takes ~50 ms on my system).
>
> That's not quite fair; the byte-compiler needs the right load-path and autoload definitions, and the byte-compiler itself needs to be loaded as well. (Anyone who can set LD_PRELOAD already has the machine.)
>
> The easiest way is to run the user's init file. Perhaps it's possible to just transmit a list of paths and packages to the subprocess as arguments but the user may have things loaded or defined outside the standard package manager.
>

We should be very hesitant to use the easiest way. Often it's the
wrong way, and fixing things after the fact tends to be hard. Again,
the goal is not to provide a perfect solution that works for all ELisp
files, but something that works well for "typical" use cases. We
should definitely run the subprocess with --quick --batch and an empty
environment by default, not only for security and speed, but also for
reproducibility. That's also what Flycheck does
(https://github.com/flycheck/flycheck/blob/a11b789807d1d942d6fcfac17508d072b9cf7ba8/flycheck.el#L8435)

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

18 dec. 2020 kl. 16.21 skrev Philipp Stephani <p.stephani2@gmail.com>:

> Ah, I was talking about the engineering/product management aspect, not
> about the technical one: If you start with an initially-open sandbox
> policy, locking it down in future releases is much harder than the
> other way round.

I assumed we were just building a mechanism for our own consumption at this stage, even if the eventual aim is something available for general use.

>  We
> should definitely run the subprocess with --quick --batch and an empty
> environment by default, not only for security and speed, but also for
> reproducibility. That's also what Flycheck does
> (https://github.com/flycheck/flycheck/blob/a11b789807d1d942d6fcfac17508d072b9cf7ba8/flycheck.el#L8435)

Thanks for the reference, and you may very well be right. A counterpoint is that since the facility would be enabled by default, a user met with complaints about perfectly fine code will immediately disable the checks and thus foil our plan to nudge his coding habits in a desirable direction.

I take it that you don't suggest that we skip on loading autoloads (possibly in the shape of quickstart) though? A bit rough to byte-compile without those, unless we deprecate autoloads altogether.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Fr., 18. Dez. 2020 um 19:50 Uhr schrieb Mattias Engdegård <mattiase@acm.org>:
>
> 18 dec. 2020 kl. 16.21 skrev Philipp Stephani <p.stephani2@gmail.com>:
>
> > Ah, I was talking about the engineering/product management aspect, not
> > about the technical one: If you start with an initially-open sandbox
> > policy, locking it down in future releases is much harder than the
> > other way round.
>
> I assumed we were just building a mechanism for our own consumption at this stage, even if the eventual aim is something available for general use.

I'd say these two questions are somewhat independent of each other.
Even with an internal-only interface, people will start to assume that
reading arbitrary files works.
I'm personally not a huge fan of such internal interfaces though. They
are necessary in some cases, but a high-level UI framework like
Flymake shouldn't need to use them. Besides, since Flymake is released
as an external package, it should rather not use internal interfaces
in the first place.

>
> >  We
> > should definitely run the subprocess with --quick --batch and an empty
> > environment by default, not only for security and speed, but also for
> > reproducibility. That's also what Flycheck does
> > (https://github.com/flycheck/flycheck/blob/a11b789807d1d942d6fcfac17508d072b9cf7ba8/flycheck.el#L8435)
>
> Thanks for the reference, and you may very well be right. A counterpoint is that since the facility would be enabled by default, a user met with complaints about perfectly fine code will immediately disable the checks and thus foil our plan to nudge his coding habits in a desirable direction.

Maybe, though I wouldn't be so sure. Elisp compilation in Flycheck is
enabled by default and presumably suffers from the same problems.
There are also similar problems with other languages: for example,
when I visit src/lisp.h and enable Flymake, I get 2287 errors, 154
warnings, and 4002 notices (which is an actual problem since the huge
number of overlays makes Emacs sluggish - probably Flymake should just
stop after 20 diagnostics or so...). I totally agree that we need to
keep the false positive rate low, but I wouldn't say that any nonzero
rate would make Flymake useless.

>
> I take it that you don't suggest that we skip on loading autoloads (possibly in the shape of quickstart) though? A bit rough to byte-compile without those, unless we deprecate autoloads altogether.
>

Good question. I'd say we disable them initially and see what happens.
It'll be a while until Emacs 28 gets released, so we have enough time
to gather feedback and make adjustments.
I also think that packages shouldn't rely on autoloads from other
packages. I generally dislike autoloads and think they are overused.
They make programming unnecessarily brittle because they assume not
only that the load path is set up correctly, but also that the correct
loaddefs files are already loaded. Autoloads are probably fine for
interactive commands to avoid unnecessarily loading rarely-used
packages, but inter-package dependencies should just use 'require'.

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

19 dec. 2020 kl. 16.08 skrev Philipp Stephani <p.stephani2@gmail.com>:

> I'd say these two questions are somewhat independent of each other.
> Even with an internal-only interface, people will start to assume that
> reading arbitrary files works.
> I'm personally not a huge fan of such internal interfaces though. They
> are necessary in some cases, but a high-level UI framework like
> Flymake shouldn't need to use them. Besides, since Flymake is released
> as an external package, it should rather not use internal interfaces
> in the first place.

What I meant is that there is no way of knowing whether an API is rubbish or not without having put it to use ourselves first (preferably in two or more ways), so let's not front-load the design. We know that this is true regardless of how good programmers we think we are. 
Flymake would be a natural user, but it must cope with our own demands first.

>> Thanks for the reference, and you may very well be right. A counterpoint is that since the facility would be enabled by default, a user met with complaints about perfectly fine code will immediately disable the checks and thus foil our plan to nudge his coding habits in a desirable direction.
> 
> Maybe, though I wouldn't be so sure. Elisp compilation in Flycheck is
> enabled by default and presumably suffers from the same problems.
> There are also similar problems with other languages: for example,
> when I visit src/lisp.h and enable Flymake, I get 2287 errors, 154
> warnings, and 4002 notices (which is an actual problem since the huge
> number of overlays makes Emacs sluggish - probably Flymake should just
> stop after 20 diagnostics or so...). I totally agree that we need to
> keep the false positive rate low, but I wouldn't say that any nonzero
> rate would make Flymake useless.

There's a difference though: flycheck is installed by someone who wants to use it and is presumably ready for some setting-up. In contrast, we are aiming at an on-by-default zero-configuration Emacs feature, which means that the bar is higher. It's meant precisely for those who would not install and configure flycheck, so false positives may have effects opposite the intended.

> I also think that packages shouldn't rely on autoloads from other
> packages. I generally dislike autoloads and think they are overused.
> They make programming unnecessarily brittle because they assume not
> only that the load path is set up correctly, but also that the correct
> loaddefs files are already loaded. Autoloads are probably fine for
> interactive commands to avoid unnecessarily loading rarely-used
> packages, but inter-package dependencies should just use 'require'.

I don't necessarily disagree but it would be interesting to hear what Stefan thinks about it. Since it's somewhat of an opinionated tool after all, it's squarely within our remit to lay down policy...

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

[ I'm afraid I dropped out of the discussion, so I may lack some
  context.  ]

> What I meant is that there is no way of knowing whether an API is rubbish or
> not without having put it to use ourselves first (preferably in two or more
> ways), so let's not front-load the design. We know that this is true
> regardless of how good programmers we think we are. 
> Flymake would be a natural user, but it must cope with our own demands first.

IIUC the discussion surrounds mostly around restricting
file-reads, right?  I agree it'd be good to have a sandbox that
restricts file-reads, but we should think about how to design such
a thing, i.e. how to specify (and then enforce) which files can be read.

My experience with the GNU ELPA scripts using bwrap is that it might not
be easy: we definitely don't want to give read access to /etc, yet it
seems very likely that some files in /etc may be needed, even in very
mundane circumstances (e.g. /etc/alternatives or /etc/emacs or ...).
This gets quickly very OS-dependent (and doesn't depend just on
Windows-vs-GNU/Linux but varies also between distributions of
GNU/Linux).  If we stick to an "in-process" sandbox (i.e. you can't run
sub-processes) it makes this a bit simpler (you don't need to worry
about read access to /lib vs /lib64 and other such things), so maybe it
is manageable.  But I think the starting point would be to give read
access to everything that's under one of the directories mentioned in
`load-path`.

> There's a difference though: flycheck is installed by someone who wants to
> use it and is presumably ready for some setting-up. In contrast, we are
> aiming at an on-by-default zero-configuration Emacs feature, which means
> that the bar is higher. It's meant precisely for those who would not install
> and configure flycheck, so false positives may have effects opposite
> the intended.

Indeed, in order to enable flymake by default we're going to have to be
extra careful to try and make sure the user isn't smothered in warnings.
That probably means for example to keep flymake disabled when visiting
the init file.

>> I also think that packages shouldn't rely on autoloads from other
>> packages. I generally dislike autoloads and think they are overused.
>> They make programming unnecessarily brittle because they assume not
>> only that the load path is set up correctly, but also that the correct
>> loaddefs files are already loaded. Autoloads are probably fine for
>> interactive commands to avoid unnecessarily loading rarely-used
>> packages, but inter-package dependencies should just use 'require'.
> I don't necessarily disagree but it would be interesting to hear what Stefan
> thinks about it. Since it's somewhat of an opinionated tool after all, it's
> squarely within our remit to lay down policy...

I must say I don't know what's being discussed here.
What autoloads?  Why do we care?


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

Am Mo., 14. Dez. 2020 um 12:05 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> > >> - This will need someone else doing the implementation.
> > > Looks like we already have a volunteer for macOS.
> > > For Linux, this shouldn't be that difficult either. The sandbox needs
> > > to install a mount namespace that only allows read access to Emacs's
> > > installation directory plus any input file and write access to known
> > > output files, and enable syscall filters that forbid everything except
> > > a list of known-safe syscalls (especially exec). I can take a stab at
> > > that, but I can't promise anything ;-)
> >
> > Looking forward to it.
> >
>
> I've looked into this, and what I'd suggest for now is:
> 1. Add a --seccomp=FILE command-line option that loads seccomp filters
> from FILE and applies them directly after startup (first thing in
> main). Why do this in Emacs? Because that's the easiest way to prevent
> execve. When installing a seccomp filter in a separate process, execve
> needs to be allowed because otherwise there'd be no way to execute the
> Emacs binary. While there are workarounds (ptrace, LD_PRELOAD), it's
> easiest to install the seccomp filter directly in the Emacs process.

I've attached a patch for this.

[-- Attachment #2: 0001-Add-support-for-seccomp-command-line-option.patch --]
[-- Type: text/x-patch, Size: 13272 bytes --]

From 80fc0ab92e1f025a8a41047d7909be6a97f5b09a Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Mon, 14 Dec 2020 21:25:11 +0100
Subject: [PATCH] Add support for --seccomp command-line option.

When passing this option on GNU/Linux, Emacs installs a Secure
Computing kernel system call filter.  See Bug#45198.

* configure.ac: Check for seccomp header.

* src/emacs.c (usage_message): Document --seccomp option.
(emacs_seccomp): New wrapper for 'seccomp' syscall.
(load_seccomp, maybe_load_seccomp, read_full): New helper functions.
(main): Potentially load seccomp filters during startup.
(standard_args): Add --seccomp option.

* lisp/startup.el (command-line): Detect and ignore --seccomp option.

* test/src/emacs-tests.el (emacs-tests/seccomp/absent-file)
(emacs-tests/seccomp/empty-file)
(emacs-tests/seccomp/invalid-file-size): New unit tests.
(emacs-tests--with-temp-file): New helper macro.

* etc/NEWS: Document new --seccomp option.
---
 configure.ac            |   2 +
 etc/NEWS                |  10 +++
 lisp/startup.el         |   4 +-
 src/emacs.c             | 168 +++++++++++++++++++++++++++++++++++++++-
 test/src/emacs-tests.el |  86 ++++++++++++++++++++
 5 files changed, 267 insertions(+), 3 deletions(-)
 create mode 100644 test/src/emacs-tests.el

diff --git a/configure.ac b/configure.ac
index 888b415148..087dd67e18 100644
--- a/configure.ac
+++ b/configure.ac
@@ -4184,6 +4184,8 @@ AC_DEFUN
 AC_SUBST([BLESSMAIL_TARGET])
 AC_SUBST([LIBS_MAIL])
 
+AC_CHECK_HEADERS([linux/seccomp.h])
+
 OLD_LIBS=$LIBS
 LIBS="$LIB_PTHREAD $LIB_MATH $LIBS"
 AC_CHECK_FUNCS(accept4 fchdir gethostname \
diff --git a/etc/NEWS b/etc/NEWS
index 4a8e70e6a6..bd255faca4 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -82,6 +82,16 @@ lacks the terminfo database, you can instruct Emacs to support 24-bit
 true color by setting 'COLORTERM=truecolor' in the environment.  This is
 useful on systems such as FreeBSD which ships only with "etc/termcap".
 
+** On GNU/Linux systems, Emacs now supports loading a Secure Computing
+filter.  To use this, you can pass a --seccomp=FILE command-line
+option to Emacs.  FILE must name a binary file containing an array of
+'struct sock_filter' structures.  Emacs will then install that list of
+Secure Computing filters into its own process early during the startup
+process.  You can use this functionality to put an Emacs process in a
+sandbox to avoid security issues when executing untrusted code.  See
+the manual page for 'seccomp' for details about Secure Computing
+filters.
+
 \f
 * Changes in Emacs 28.1
 
diff --git a/lisp/startup.el b/lisp/startup.el
index b1128f6d02..9b1346e448 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -1094,7 +1094,7 @@ command-line
                          ("--no-x-resources") ("--debug-init")
                          ("--user") ("--iconic") ("--icon-type") ("--quick")
 			 ("--no-blinking-cursor") ("--basic-display")
-                         ("--dump-file") ("--temacs")))
+                         ("--dump-file") ("--temacs") ("--seccomp")))
              (argi (pop args))
              (orig-argi argi)
              argval)
@@ -1146,7 +1146,7 @@ command-line
 	  (push '(visibility . icon) initial-frame-alist))
 	 ((member argi '("-nbc" "-no-blinking-cursor"))
 	  (setq no-blinking-cursor t))
-         ((member argi '("-dump-file" "-temacs"))  ; Handled in C
+         ((member argi '("-dump-file" "-temacs" "-seccomp"))  ; Handled in C
           (or argval (pop args))
           (setq argval nil))
 	 ;; Push the popped arg back on the list of arguments.
diff --git a/src/emacs.c b/src/emacs.c
index 2a32083ba1..0a939fc901 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -61,6 +61,13 @@ #define MAIN_PROGRAM
 # include <sys/socket.h>
 #endif
 
+#ifdef HAVE_LINUX_SECCOMP_H
+# include <linux/seccomp.h>
+# include <linux/filter.h>
+# include <sys/prctl.h>
+# include <sys/syscall.h>
+#endif
+
 #ifdef HAVE_WINDOW_SYSTEM
 #include TERM_HEADER
 #endif /* HAVE_WINDOW_SYSTEM */
@@ -239,6 +246,11 @@ #define MAIN_PROGRAM
     "\
 --dump-file FILE            read dumped state from FILE\n\
 ",
+#endif
+#ifdef HAVE_LINUX_SECCOMP_H
+    "\
+--sandbox=FILE              read Seccomp BPF filter from FILE\n\
+"
 #endif
     "\
 --no-build-details          do not add build details such as time stamps\n\
@@ -937,6 +949,150 @@ load_pdump (int argc, char **argv)
 }
 #endif /* HAVE_PDUMPER */
 
+#ifdef HAVE_LINUX_SECCOMP_H
+
+/* Wrapper function for the `seccomp' system call on GNU/Linux.  This system
+   call usually doesn't have a wrapper function.  See the manual page of
+   `seccomp' for the signature.  */
+
+static int
+emacs_seccomp (unsigned int operation, unsigned int flags, void *args)
+{
+#ifdef SYS_seccomp
+  return syscall (SYS_seccomp, operation, flags, args);
+#else
+  errno = ENOSYS;
+  return -1;
+#endif
+}
+
+/* Read exactly SIZE bytes into BUFFER.  Return false on short read.  */
+
+static bool
+read_full (int fd, void *buffer, size_t size)
+{
+  if (SSIZE_MAX < size)
+    {
+      errno = EFBIG;
+      return false;
+    }
+  char *ptr = buffer;
+  while (size != 0)
+    {
+      /* We can't use `emacs_read' yet because quitting doesn't work here
+         yet.  */
+      ssize_t ret = TEMP_FAILURE_RETRY (read (fd, ptr, size));
+      if (ret < 0)
+        return false;
+      if (ret == 0)
+        break;  /* Avoid infinite loop on encountering EOF.  */
+      eassert (ret <= size);
+      size -= ret;
+      ptr += ret;
+    }
+  if (size != 0)
+    errno = ENODATA;
+  return size == 0;
+}
+
+/* Attempt to load Secure Computing filters from FILE.  Return false if that
+   doesn't work for some reason.  */
+
+static bool
+load_seccomp (const char *file)
+{
+  bool success = false;
+  struct sock_fprog program = {0, NULL};
+  /* We can't use `emacs_open' yet because quitting doesn't work here yet.  */
+  int fd = TEMP_FAILURE_RETRY (open (file, O_RDONLY | O_BINARY | O_CLOEXEC));
+  if (fd < 0)
+    {
+      emacs_perror ("open");
+      goto out;
+    }
+  struct stat stat;
+  if (fstat (fd, &stat) < 0)
+    {
+      emacs_perror ("fstat");
+      goto out;
+    }
+  if (stat.st_size <= 0 || SIZE_MAX < stat.st_size
+      || (size_t) stat.st_size % sizeof *program.filter != 0)
+    {
+      fprintf (stderr, "seccomp filter %s has invalid size %jd\n", file,
+               (intmax_t) stat.st_size);
+      goto out;
+    }
+  size_t size = stat.st_size;
+  size_t count = size / sizeof *program.filter;
+  eassert (0 < size && 0 < count);
+  if (USHRT_MAX < count)
+    {
+      fprintf (stderr, "seccomp filter %s is too big", file);
+      goto out;
+    }
+  program.len = count;
+  program.filter = malloc (size);
+  if (program.filter == NULL)
+    {
+      emacs_perror ("malloc");
+      goto out;
+    }
+  if (!read_full (fd, program.filter, size))
+    {
+      emacs_perror ("read");
+      goto out;
+    }
+  if (close (fd) != 0)
+    emacs_perror ("close");  /* not critical */
+  fd = -1;
+
+  /* See man page of `seccomp' why this is necessary.  Note that we
+     intentionally don't check the return value: a parent process might have
+     made this call before, in which case it would fail; or, if enabling
+     privilege-restricting mode fails, the `seccomp' syscall will fail
+     anyway.  */
+  prctl (PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+  /* Install the filter.  Make sure that potential other threads can't escape
+     it.  */
+  if (emacs_seccomp (SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_TSYNC,
+                     &program)
+      != 0)
+    {
+      emacs_perror ("seccomp");
+      goto out;
+    }
+  success = true;
+
+ out:
+  free (program.filter);
+  close (fd);
+  return success;
+}
+
+/* Load Secure Computing filter from file specified with the --seccomp option.
+   Exit if that fails.  */
+
+static void
+maybe_load_seccomp (int argc, char **argv)
+{
+  int skip_args = 0;
+  char *file = NULL;
+  while (skip_args < argc - 1)
+    {
+      if (argmatch (argv, argc, "-seccomp", "--seccomp", 9, &file, &skip_args)
+          || argmatch (argv, argc, "--", NULL, 2, NULL, &skip_args))
+        break;
+      ++skip_args;
+    }
+  if (file == NULL)
+    return;
+  if (!load_seccomp (file))
+    fatal ("cannot enable seccomp filter from %s", file);
+}
+
+#endif  /* HAVE_LINUX_SECCOMP_H */
+
 int
 main (int argc, char **argv)
 {
@@ -944,6 +1100,13 @@ main (int argc, char **argv)
      for pointers.  */
   void *stack_bottom_variable;
 
+  /* First, check whether we should apply a seccomp filter.  This should come at
+     the very beginning to allow the filter to protect the initialization
+     phase.  */
+#ifdef HAVE_LINUX_SECCOMP_H
+  maybe_load_seccomp (argc, argv);
+#endif
+
   bool no_loadup = false;
   char *junk = 0;
   char *dname_arg = 0;
@@ -2137,12 +2300,15 @@ main (int argc, char **argv)
   { "-color", "--color", 5, 0},
   { "-no-splash", "--no-splash", 3, 0 },
   { "-no-desktop", "--no-desktop", 3, 0 },
-  /* The following two must be just above the file-name args, to get
+  /* The following three must be just above the file-name args, to get
      them out of our way, but without mixing them with file names.  */
   { "-temacs", "--temacs", 1, 1 },
 #ifdef HAVE_PDUMPER
   { "-dump-file", "--dump-file", 1, 1 },
 #endif
+#ifdef HAVE_LINUX_SECCOMP_H
+  { "-seccomp", "--seccomp", 1, 1 },
+#endif
 #ifdef HAVE_NS
   { "-NSAutoLaunch", 0, 5, 1 },
   { "-NXAutoLaunch", 0, 5, 1 },
diff --git a/test/src/emacs-tests.el b/test/src/emacs-tests.el
new file mode 100644
index 0000000000..279ecb210c
--- /dev/null
+++ b/test/src/emacs-tests.el
@@ -0,0 +1,86 @@
+;;; emacs-tests.el --- unit tests for emacs.c -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2020  Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Unit tests for src/emacs.c.
+
+;;; Code:
+
+(require 'cl-lib)
+(require 'ert)
+
+(ert-deftest emacs-tests/seccomp/absent-file ()
+  (let ((emacs (expand-file-name invocation-name invocation-directory))
+        (process-environment nil))
+    (skip-unless (file-executable-p emacs))
+    (should-not (file-exists-p "/does-not-exist.bpf"))
+    (should-not
+     (eql (call-process emacs nil nil nil
+                        "--quick" "--batch" "--seccomp=/does-not-exist.bpf")
+          0))))
+
+(cl-defmacro emacs-tests--with-temp-file
+    (var (prefix &optional suffix text) &rest body)
+  "Evaluate BODY while a new temporary file exists.
+Bind VAR to the name of the file.  Pass PREFIX, SUFFIX, and TEXT
+to `make-temp-file', which see."
+  (declare (indent 2) (debug (symbolp (form form form) body)))
+  (cl-check-type var symbol)
+  ;; Use an uninterned symbol so that the code still works if BODY changes VAR.
+  (let ((filename (make-symbol "filename")))
+    `(let ((,filename (make-temp-file ,prefix nil ,suffix ,text)))
+       (unwind-protect
+           (let ((,var ,filename))
+             ,@body)
+         (delete-file ,filename)))))
+
+(ert-deftest emacs-tests/seccomp/empty-file ()
+  (let ((emacs (expand-file-name invocation-name invocation-directory))
+        (process-environment nil))
+    (skip-unless (file-executable-p emacs))
+    (emacs-tests--with-temp-file filter ("seccomp-invalid-" ".bpf")
+      ;; The --seccomp option is processed early, without filename handlers.
+      ;; Therefore remote or quoted filenames wouldn't work.
+      (should-not (file-remote-p filter))
+      (cl-callf file-name-unquote filter)
+      ;; According to the Seccomp man page, a filter must have at least one
+      ;; element, so Emacs should reject an empty file.
+      (should-not
+       (eql (call-process emacs nil nil nil
+                          "--quick" "--batch" (concat "--seccomp=" filter))
+            0)))))
+
+(ert-deftest emacs-tests/seccomp/invalid-file-size ()
+  (let ((emacs (expand-file-name invocation-name invocation-directory))
+        (process-environment nil))
+    (skip-unless (file-executable-p emacs))
+    (emacs-tests--with-temp-file filter ("seccomp-invalid-" ".bpf" "123456")
+      ;; The --seccomp option is processed early, without filename handlers.
+      ;; Therefore remote or quoted filenames wouldn't work.
+      (should-not (file-remote-p filter))
+      (cl-callf file-name-unquote filter)
+      ;; The Seccomp filter file must have a file size that's a multiple of the
+      ;; size of struct sock_filter, which is 8 or 16, but never 6.
+      (should-not
+       (eql (call-process emacs nil nil nil
+                          "--quick" "--batch" (concat "--seccomp=" filter))
+            0)))))
+
+;;; emacs-tests.el ends here
-- 
2.29.2.729.g45daf8777d-goog

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

19 dec. 2020 kl. 19.11 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> I must say I don't know what's being discussed here.
> What autoloads?  Why do we care?

If a user looks at elisp code that depends on autoloads and the checker process cannot load those for reasons of sandbox policy or whatnot, there will be false positives about missing functions. This is fine if we are phasing out autoloads but as far as I know we're not.

bug#45198: 28.0.50; Sandbox mode

[João Távora]

On Sat, Dec 19, 2020 at 6:47 PM Mattias Engdegård <mattiase@acm.org> wrote:
>
> 19 dec. 2020 kl. 19.11 skrev Stefan Monnier <monnier@iro.umontreal.ca>:
>
> > I must say I don't know what's being discussed here.
> > What autoloads?  Why do we care?
>
> If a user looks at elisp code that depends on autoloads and the checker process cannot load those for reasons of sandbox policy or whatnot, there will be false positives about missing functions. This is fine if we are phasing out autoloads but as far as I know we're not.

It would have to depend on autoloads at compilation time.  As far as
I can tell, that's not extremely common.  It's quite more common
that things are `require`d and thus `load`ed at compilation time,
so if we block that, the Flymake compilation output becomes quite
useless in most cases, producing big single error in that first require.

Such things can and already do happen with processes that
have non-standard  load-paths mechanisms, such as one of mine.
The only load-path that the Flymake currently sees is the current
directory's.  I suppose extending that is possible (and it's been in my
plans). It's also possibly another "hazard", though one incurred in
consciously by the user,  whom we may warn via the usual "unsafe
local variables" machinery.

João

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

>> I must say I don't know what's being discussed here.
>> What autoloads?  Why do we care?
> If a user looks at elisp code that depends on autoloads and the checker
> process cannot load those for reasons of sandbox policy or whatnot, there
> will be false positives about missing functions. This is fine if we are
> phasing out autoloads but as far as I know we're not.

I don't understand why it would be different for autoloads than for
functions provided by `require`d packages.

Also, if we add all the dirs in `load-path` to the set of readable
directories (and I do think we want to do that) this should not be
an issue, right?


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 854 bytes --]

Am Mo., 14. Dez. 2020 um 12:05 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:

> > >> - This will need someone else doing the implementation.
> > > Looks like we already have a volunteer for macOS.
> > > For Linux, this shouldn't be that difficult either. The sandbox needs
> > > to install a mount namespace that only allows read access to Emacs's
> > > installation directory plus any input file and write access to known
> > > output files, and enable syscall filters that forbid everything except
> > > a list of known-safe syscalls (especially exec). I can take a stab at
> > > that, but I can't promise anything ;-)
> >
> > Looking forward to it.
> >
>
> I've looked into this, and what I'd suggest for now is:
> […]
> 2. Generate appropriate seccomp filters using libseccomp or similar.

Here's a patch for this step.

[-- Attachment #2: 0002-Add-a-helper-binary-to-create-a-basic-Secure-Computi.patch --]
[-- Type: text/x-patch, Size: 18813 bytes --]

From f5d218ef354ec21d8566271570948d7c6b776649 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Thu, 17 Dec 2020 11:20:55 +0100
Subject: [PATCH 2/2] Add a helper binary to create a basic Secure Computing
 filter.

The binary uses the 'seccomp' helper library.  The library isn't
needed to load the generated Secure Computing filter.

* configure.ac: Check for 'seccomp' header and library.

* lib-src/seccomp-filter.c: New helper binary to generate a generic
Secure Computing filter for GNU/Linux.

* lib-src/Makefile.in (DONT_INSTALL): Add 'seccomp-filter' helper
binary if possible.
(all): Add Secure Computing filter file if possible.
(seccomp-filter$(EXEEXT)): Compile helper binary.
(seccomp-filter.bpf seccomp-filter.pfc): Generate filter files.

* test/src/emacs-tests.el (emacs-tests/seccomp/allows-stdout)
(emacs-tests/seccomp/forbids-subprocess): New unit tests.

* test/Makefile.in (src/emacs-tests.log): Add dependency on the helper
binary.
---
 .gitignore                                  |   5 +
 configure.ac                                |   5 +
 lib-src/Makefile.in                         |  19 ++
 lib-src/seccomp-filter.c                    | 303 ++++++++++++++++++++
 test/Makefile.in                            |   2 +
 test/src/emacs-resources/seccomp-filter.bpf |   1 +
 test/src/emacs-tests.el                     |  44 +++
 7 files changed, 379 insertions(+)
 create mode 100644 lib-src/seccomp-filter.c
 create mode 120000 test/src/emacs-resources/seccomp-filter.bpf

diff --git a/.gitignore b/.gitignore
index bf7e934981..8c8b1f7584 100644
--- a/.gitignore
+++ b/.gitignore
@@ -187,6 +187,7 @@ lib-src/make-docfile
 lib-src/make-fingerprint
 lib-src/movemail
 lib-src/profile
+lib-src/seccomp-filter
 lib-src/test-distrib
 lib-src/update-game-score
 nextstep/Cocoa/Emacs.base/Contents/Info.plist
@@ -298,3 +299,7 @@ nt/emacs.rc
 nt/emacsclient.rc
 src/gdb.ini
 /var/
+
+# Seccomp filter files.
+lib-src/seccomp-filter.bpf
+lib-src/seccomp-filter.pfc
diff --git a/configure.ac b/configure.ac
index 087dd67e18..cb698e34fe 100644
--- a/configure.ac
+++ b/configure.ac
@@ -4186,6 +4186,11 @@ AC_DEFUN
 
 AC_CHECK_HEADERS([linux/seccomp.h])
 
+LIBSECCOMP=
+AC_CHECK_HEADER([seccomp.h],
+  [AC_CHECK_LIB([seccomp], [seccomp_init], [LIBSECCOMP=-lseccomp])])
+AC_SUBST([LIBSECCOMP])
+
 OLD_LIBS=$LIBS
 LIBS="$LIB_PTHREAD $LIB_MATH $LIBS"
 AC_CHECK_FUNCS(accept4 fchdir gethostname \
diff --git a/lib-src/Makefile.in b/lib-src/Makefile.in
index a2d27eab00..72a980e4de 100644
--- a/lib-src/Makefile.in
+++ b/lib-src/Makefile.in
@@ -209,6 +209,12 @@ LIB_EACCESS=
 ## empty or -lwsock2 for MinGW
 LIB_WSOCK32=@LIB_WSOCK32@
 
+LIBSECCOMP=@LIBSECCOMP@
+
+ifneq ($(LIBSECCOMP),)
+DONT_INSTALL += seccomp-filter$(EXEEXT)
+endif
+
 ## Extra libraries to use when linking movemail.
 LIBS_MOVE = $(LIBS_MAIL) $(KRB4LIB) $(DESLIB) $(KRB5LIB) $(CRYPTOLIB) \
             $(COM_ERRLIB) $(LIBHESIOD) $(LIBRESOLV) $(LIB_WSOCK32)
@@ -238,6 +244,10 @@ config_h =
 
 all: ${EXE_FILES} ${SCRIPTS}
 
+ifneq ($(LIBSECCOMP),)
+all: seccomp-filter.bpf
+endif
+
 .PHONY: all need-blessmail maybe-blessmail
 
 LOADLIBES = ../lib/libgnu.a $(LIBS_SYSTEM)
@@ -420,4 +430,13 @@ update-game-score${EXEEXT}:
 emacsclient.res: ../nt/emacsclient.rc $(NTINC)/../icons/emacs.ico
 	$(AM_V_RC)$(WINDRES) -O coff --include-dir=$(NTINC)/.. -o $@ $<
 
+ifneq ($(LIBSECCOMP),)
+seccomp-filter$(EXEEXT): $(srcdir)/seccomp-filter.c $(config_h)
+	$(AM_V_CCLD)$(CC) $(ALL_CFLAGS) $< $(LIBSECCOMP) -o $@
+
+seccomp-filter.bpf seccomp-filter.pfc: seccomp-filter$(EXEEXT)
+	$(AM_V_GEN)./seccomp-filter$(EXEEXT) \
+	  seccomp-filter.bpf seccomp-filter.pfc
+endif
+
 ## Makefile ends here.
diff --git a/lib-src/seccomp-filter.c b/lib-src/seccomp-filter.c
new file mode 100644
index 0000000000..e0fea0b421
--- /dev/null
+++ b/lib-src/seccomp-filter.c
@@ -0,0 +1,303 @@
+/* Generate a Secure Computing filter definition file.
+
+Copyright (C) 2020 Free Software Foundation, Inc.
+
+This file is part of GNU Emacs.
+
+GNU Emacs is free software: you can redistribute it and/or modify it under the
+terms of the GNU General Public License as published by the Free Software
+Foundation, either version 3 of the License, or (at your option) any later
+version.
+
+GNU Emacs is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
+PARTICULAR PURPOSE.  See the GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License along with GNU
+Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
+
+/* This program creates a small Secure Computing filter usable for a typical
+minimal Emacs sandbox.  See the man page for `seccomp' for details about Secure
+Computing filters.  This program requires the `libseccomp' library.  However,
+the resulting filter file requires only a Linux kernel supporting the Secure
+Computing extension.
+
+Usage:
+
+  seccomp-filter out.bpf out.pfc
+
+This writes the raw `struct sock_filter' array to out.bpf and a human-readable
+representation to out.pfc.  */
+
+#include "config.h"
+
+#include <errno.h>
+#include <limits.h>
+#include <stdarg.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdio.h>
+
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+#include <sys/prctl.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <linux/futex.h>
+#include <fcntl.h>
+#include <sched.h>
+#include <seccomp.h>
+#include <unistd.h>
+
+#include "verify.h"
+
+static ATTRIBUTE_FORMAT_PRINTF (2, 3) _Noreturn void
+fail (int error, const char *format, ...)
+{
+  va_list ap;
+  va_start (ap, format);
+  if (error == 0)
+    vfprintf (stderr, format, ap);
+  else
+    {
+      char buffer[1000];
+      vsnprintf (buffer, sizeof buffer, format, ap);
+      errno = error;
+      perror (buffer);
+    }
+  va_end (ap);
+  fflush (NULL);
+  exit (EXIT_FAILURE);
+}
+
+/* This binary is trivial, so we use a single global filter context object that
+   we release using `atexit'.  */
+
+static scmp_filter_ctx ctx;
+
+static void
+release_context (void)
+{
+  seccomp_release (ctx);
+}
+
+/* Wrapper functions and macros for libseccomp functions.  We exit immediately
+   upon any error to avoid error checking noise.  */
+
+static void
+set_attribute (enum scmp_filter_attr attr, uint32_t value)
+{
+  int status = seccomp_attr_set (ctx, attr, value);
+  if (status < 0)
+    fail (-status, "seccomp_attr_set (ctx, %u, %u)", attr, value);
+}
+
+/* Like `seccomp_rule_add (ACTION, SYSCALL, ...)', except that you don't have to
+   specify the number of comparator arguments, and any failure will exit the
+   process.  */
+
+#define RULE(action, syscall, ...)                                      \
+  do                                                                    \
+    {                                                                   \
+      const struct scmp_arg_cmp arg_array[] = {__VA_ARGS__};            \
+      enum { arg_cnt = sizeof arg_array / sizeof *arg_array };          \
+      int status = seccomp_rule_add_array (ctx, (action), (syscall),    \
+                                           arg_cnt, arg_array);         \
+      if (status < 0)                                                   \
+        fail (-status, "seccomp_rule_add_array (%s, %s, %d, {%s})",	\
+	      #action, #syscall, arg_cnt, #__VA_ARGS__);		\
+    }                                                                   \
+  while (false)
+
+static void
+export_filter (const char *file, int (*function) (const scmp_filter_ctx, int),
+               const char *name)
+{
+  int fd = TEMP_FAILURE_RETRY (
+    open (file, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_CLOEXEC, 0644));
+  if (fd < 0)
+    fail (errno, "open %s", file);
+  int status = function (ctx, fd);
+  if (status < 0)
+    fail (-status, "%s", name);
+  if (close (fd) != 0)
+    fail (errno, "close");
+}
+
+#define EXPORT_FILTER(file, function) \
+  export_filter ((file), (function), #function)
+
+int
+main (int argc, char **argv)
+{
+  if (argc != 3)
+    fail (0, "usage: %s out.bpf out.pfc", argv[0]);
+
+  /* Any unhandled syscall should abort the Emacs process.  */
+  ctx = seccomp_init (SCMP_ACT_KILL_PROCESS);
+  if (ctx == NULL)
+    fail (0, "seccomp_init");
+  atexit (release_context);
+
+  /* We want to abort immediately if the architecture is unknown.  */
+  set_attribute (SCMP_FLTATR_ACT_BADARCH, SCMP_ACT_KILL_PROCESS);
+  set_attribute (SCMP_FLTATR_CTL_NNP, 1);
+  set_attribute (SCMP_FLTATR_CTL_TSYNC, 1);
+  set_attribute (SCMP_FLTATR_CTL_LOG, 0);
+
+  verify (CHAR_BIT == 8);
+  verify (sizeof (int) == 4 && INT_MIN == INT32_MIN && INT_MAX == INT32_MAX);
+  verify (sizeof (void *) == 8);
+  verify ((uintptr_t) NULL == 0);
+
+  /* Allow a clean exit.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (exit));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (exit_group));
+
+  /* Allow `mmap' and friends.  This is necessary for dynamic loading, reading
+     the portable dump file, and thread creation.  We don't allow pages to be
+     both writable and executable.  */
+  verify (MAP_PRIVATE != 0);
+  verify (MAP_SHARED != 0);
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (mmap),
+        SCMP_A2_32 (SCMP_CMP_MASKED_EQ, ~(PROT_NONE | PROT_READ | PROT_WRITE)),
+        /* Only support known flags.  MAP_DENYWRITE is ignored, but some
+           versions of the dynamic loader still use it.  Also allow allocating
+           thread stacks.  */
+        SCMP_A3_32 (SCMP_CMP_MASKED_EQ,
+                    ~(MAP_PRIVATE | MAP_FILE | MAP_ANONYMOUS | MAP_FIXED
+                      | MAP_DENYWRITE | MAP_STACK | MAP_NORESERVE),
+                    0));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (mmap),
+        SCMP_A2_32 (SCMP_CMP_MASKED_EQ, ~(PROT_NONE | PROT_READ | PROT_EXEC)),
+        /* Only support known flags.  MAP_DENYWRITE is ignored, but some
+           versions of the dynamic loader still use it. */
+        SCMP_A3_32 (SCMP_CMP_MASKED_EQ,
+                    ~(MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED | MAP_DENYWRITE),
+                    0));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (munmap));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (mprotect),
+	/* Don't allow making pages executable.  */
+        SCMP_A2_32 (SCMP_CMP_MASKED_EQ, ~(PROT_NONE | PROT_READ | PROT_WRITE),
+                    0));
+
+  /* Futexes are used everywhere.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (futex),
+        SCMP_A1_32 (SCMP_CMP_EQ, FUTEX_WAKE_PRIVATE));
+
+  /* Allow basic dynamic memory management.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (brk));
+
+  /* Allow some status inquiries.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (uname));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (getuid));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (geteuid));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (getpid));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (getpgrp));
+
+  /* Allow operations on open file descriptors.  File descriptors are
+     capabilities, and operating on them shouldn't cause security issues.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (read));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (write));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (close));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (lseek));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (dup));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (dup2));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (fstat));
+
+  /* Allow read operations on the filesystem.  If necessary, these should be
+     further restricted using mount namespaces.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (access));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (faccessat));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (stat));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (stat64));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (lstat));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (lstat64));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (fstatat64));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (newfstatat));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (readlink));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (readlinkat));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (getcwd));
+
+  /* Allow opening files, assuming they are only opened for reading.  */
+  verify (O_WRONLY != 0);
+  verify (O_RDWR != 0);
+  verify (O_CREAT != 0);
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (open),
+        SCMP_A1_32 (SCMP_CMP_MASKED_EQ,
+                    ~(O_RDONLY | O_BINARY | O_CLOEXEC | O_PATH | O_DIRECTORY),
+                    0));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (openat),
+        SCMP_A2_32 (SCMP_CMP_MASKED_EQ,
+                    ~(O_RDONLY | O_BINARY | O_CLOEXEC | O_PATH | O_DIRECTORY),
+                    0));
+
+  /* Allow `tcgetpgrp'.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (ioctl), SCMP_A0_32 (SCMP_CMP_EQ, STDIN_FILENO),
+        SCMP_A1_32 (SCMP_CMP_EQ, TIOCGPGRP));
+
+  /* Allow reading (but not setting) file flags.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (fcntl), SCMP_A1_32 (SCMP_CMP_EQ, F_GETFL));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (fcntl64), SCMP_A1_32 (SCMP_CMP_EQ, F_GETFL));
+
+  /* Allow reading random numbers from the kernel.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (getrandom));
+
+  /* Changing the umask is uncritical.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (umask));
+
+  /* Allow creation of pipes.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (pipe));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (pipe2));
+
+  /* Allow reading (but not changing) resource limits.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (getrlimit));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (prlimit64),
+	SCMP_A0_32 (SCMP_CMP_EQ, 0) /* pid == 0 (current process) */,
+        SCMP_A2_64 (SCMP_CMP_EQ, 0) /* new_limit == NULL */);
+
+  /* Block changing resource limits, but don't crash.  */
+  RULE (SCMP_ACT_ERRNO (EPERM), SCMP_SYS (prlimit64),
+        SCMP_A0_32 (SCMP_CMP_EQ, 0) /* pid == 0 (current process) */,
+        SCMP_A2_64 (SCMP_CMP_NE, 0) /* new_limit != NULL */);
+
+  /* Emacs installs signal handlers, which is harmless.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (sigaction));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (rt_sigaction));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (sigprocmask));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (rt_sigprocmask));
+
+  /* Allow timer support.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (timer_create));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (timerfd_create));
+
+  /* Allow thread creation.  See the NOTES section in the manual page for the
+     `clone' function.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (clone),
+        SCMP_A0_64 (SCMP_CMP_MASKED_EQ,
+		    /* Flags needed to create threads.  See create_thread in
+		       libc.  */
+                    ~(CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SYSVSEM
+                      | CLONE_SIGHAND | CLONE_THREAD | CLONE_SETTLS
+                      | CLONE_PARENT_SETTID | CLONE_CHILD_CLEARTID),
+                    0));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (sigaltstack));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (set_robust_list));
+
+  /* Allow setting the process name for new threads.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (prctl), SCMP_A0_32 (SCMP_CMP_EQ, PR_SET_NAME));
+
+  /* Allow some event handling functions used by glib.  */
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (eventfd));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (eventfd2));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (wait4));
+  RULE (SCMP_ACT_ALLOW, SCMP_SYS (poll));
+
+  /* Don't allow creating sockets (network access would be extremely dangerous),
+     but also don't crash.  */
+  RULE (SCMP_ACT_ERRNO (EACCES), SCMP_SYS (socket));
+
+  EXPORT_FILTER (argv[1], seccomp_export_bpf);
+  EXPORT_FILTER (argv[2], seccomp_export_pfc);
+}
diff --git a/test/Makefile.in b/test/Makefile.in
index 67d203df29..99f69f7fcd 100644
--- a/test/Makefile.in
+++ b/test/Makefile.in
@@ -271,6 +271,8 @@ $(test_module): $(test_module:
 	  $(srcdir)/../lib/timespec.c $(srcdir)/../lib/gettime.c
 endif
 
+src/emacs-tests.log: ../lib-src/seccomp-filter.c
+
 ## Check that there is no 'automated' subdirectory, which would
 ## indicate an incomplete merge from an older version of Emacs where
 ## the tests were arranged differently.
diff --git a/test/src/emacs-resources/seccomp-filter.bpf b/test/src/emacs-resources/seccomp-filter.bpf
new file mode 120000
index 0000000000..b3d603d0ae
--- /dev/null
+++ b/test/src/emacs-resources/seccomp-filter.bpf
@@ -0,0 +1 @@
+../../../lib-src/seccomp-filter.bpf
\ No newline at end of file
diff --git a/test/src/emacs-tests.el b/test/src/emacs-tests.el
index 279ecb210c..9599c26783 100644
--- a/test/src/emacs-tests.el
+++ b/test/src/emacs-tests.el
@@ -25,6 +25,8 @@
 
 (require 'cl-lib)
 (require 'ert)
+(require 'ert-x)
+(require 'subr-x)
 
 (ert-deftest emacs-tests/seccomp/absent-file ()
   (let ((emacs (expand-file-name invocation-name invocation-directory))
@@ -83,4 +85,46 @@ emacs-tests/seccomp/invalid-file-size
                           "--quick" "--batch" (concat "--seccomp=" filter))
             0)))))
 
+(ert-deftest emacs-tests/seccomp/allows-stdout ()
+  (let ((emacs (expand-file-name invocation-name invocation-directory))
+        (filter (ert-resource-file "seccomp-filter.bpf"))
+        (process-environment nil))
+    (skip-unless (file-executable-p emacs))
+    (skip-unless (file-readable-p filter))
+    ;; The --seccomp option is processed early, without filename handlers.
+    ;; Therefore remote or quoted filenames wouldn't work.
+    (should-not (file-remote-p filter))
+    (cl-callf file-name-unquote filter)
+    (with-temp-buffer
+      (let ((status (call-process emacs nil t nil
+                                  "--quick" "--batch"
+                                  (concat "--seccomp=" filter)
+                                  (concat "--eval=" (prin1-to-string
+                                                     '(message "Hi"))))))
+        (ert-info ((format "Process output: %s" (buffer-string)))
+          (should (eql status 0)))
+        (should (equal (string-trim (buffer-string)) "Hi"))))))
+
+(ert-deftest emacs-tests/seccomp/forbids-subprocess ()
+  (let ((emacs (expand-file-name invocation-name invocation-directory))
+        (filter (ert-resource-file "seccomp-filter.bpf"))
+        (process-environment nil))
+    (skip-unless (file-executable-p emacs))
+    (skip-unless (file-readable-p filter))
+    ;; The --seccomp option is processed early, without filename handlers.
+    ;; Therefore remote or quoted filenames wouldn't work.
+    (should-not (file-remote-p filter))
+    (cl-callf file-name-unquote filter)
+    (with-temp-buffer
+      (let ((status
+             (call-process
+              emacs nil t nil
+              "--quick" "--batch"
+              (concat "--seccomp=" filter)
+              (concat "--eval=" (prin1-to-string
+                                 `(call-process ,emacs nil nil nil
+                                                "--version"))))))
+        (ert-info ((format "Process output: %s" (buffer-string)))
+          (should-not (eql status 0)))))))
+
 ;;; emacs-tests.el ends here
-- 
2.29.2.729.g45daf8777d-goog

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Mo., 14. Dez. 2020 um 15:44 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
> >>     Returns a process object.
> > Depending how much we care about forward compatibility, it might be
> > better to return an opaque sandbox object (which will initially wrap a
> > process object).
>
> We always use process objects to represent file-descriptors, so I can't
> find any good reason why this one should be different or why an
> implementation might find it difficult to expose a process object.

If we return a single pipe process object here, then there would be no
way to access or wait for the actual Emacs process object. That might
be OK, but maybe it's not. Does Emacs guarantee that (while
(accept-process-output PIPE)) for some pipe process reads the entire
process output and finishes in due time (not too long after the actual
process)? Also, how would we report errors from the subprocess? A pipe
process can't really fail, right?

>
> >>     FUNCTION is called with no arguments and it can use `sandbox-read`
> >>     to read the data sent to the process object via `process-send-string`,
> >>     and `sandbox-reply` to send back a reply to the parent process
> >>     (which will receive it via its `process-filter`).
> > That is, sandbox-read and sandbox-reply just read/write stdin/stdout?
>
> While it may use stdin/stdout internally, I can imagine good reasons why
> we'd want to use some other file descriptors.

Yes, it should in many cases not be stdin/stdout. The standard output
and error are polluted with log messages, and stdin should likely be
closed or empty to avoid Emacs trying to read from it.
It should definitely be possible to create a "magic" pipe process
(similar to the "magic network process" created for systemd socket
activation) that wraps a file descriptor pair passed on the
command-line.
OTOH, for the case at hand using stdout/stderr seems right: the error
messages are printed there, and the parent Emacs process can parse
them.
Or do you suggest sending the error messages in a structured format
(e.g. JSON) over the pipe?

>
> > That would certainly work, but (a) it doesn't really have anything to
> > do with sandboxing, so these functions should rather be called
> > stdin-read and stdout-write or similar,
>
> I think "the right thing" would be to represent the parent as a process
> object inside the child.  I proposed dedicated functions only because
> but when it uses stdin/stdout, providing a process object seems awkward
> to implement.

What would be the difference? It would be a pipe process wrapping some
open file descriptor pair in both cases.

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

>> I think "the right thing" would be to represent the parent as a process
>> object inside the child.  I proposed dedicated functions only because
>> but when it uses stdin/stdout, providing a process object seems awkward
>> to implement.
> What would be the difference? It would be a pipe process wrapping some
> open file descriptor pair in both cases.

Differences:

1- we don't have this pipe process wrapping of stdin/stdout so it's extra
   C code we currently don't have.
2- I suspect that it wouldn't be quite so easy because it may interfere
   with other preexisting uses of stdin/stdout in the C code.

If/when someone implements that, then indeed we can just use a process
object to represent the parent in the client as well.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am So., 20. Dez. 2020 um 00:17 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> >> I think "the right thing" would be to represent the parent as a process
> >> object inside the child.  I proposed dedicated functions only because
> >> but when it uses stdin/stdout, providing a process object seems awkward
> >> to implement.
> > What would be the difference? It would be a pipe process wrapping some
> > open file descriptor pair in both cases.
>
> Differences:
>
> 1- we don't have this pipe process wrapping of stdin/stdout so it's extra
>    C code we currently don't have.

But we also don't have pipe processes wrapping file descriptors passed
on the command line, or pipes that aren't close-on-exec, so core
changes are needed in any case.

> 2- I suspect that it wouldn't be quite so easy because it may interfere
>    with other preexisting uses of stdin/stdout in the C code.

Yes, the read end of the pipe would have to deal with interleaved
output from process-send-string and print.

>
> If/when someone implements that, then indeed we can just use a process
> object to represent the parent in the client as well.
>

Yes, but again, there's no difference between the standard streams and
using newly-allocated file descriptors. In both cases, you need a
variant of Fmake_pipe_process that doesn't call pipe2 twice, but wraps
two existing file descriptors in its infd and outfd. Whether those
happen to be 0 and 1 or something passed on the command line makes no
difference.
On the parent process side, if you want to use a separate pipe pair,
you need a way to create a pipe process that doesn't use O_CLOEXEC and
allows reading out the open file descriptors to be able to pass them
to the subprocess on the command line.
These changes aren't large, but they are necessary if you want to go
the "extra pipe pair" route.

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

19 dec. 2020 kl. 22.01 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> I don't understand why it would be different for autoloads than for
> functions provided by `require`d packages.

It was suggested that no init files be loaded to make the Emacs subprocess start faster. (I'm neutral here.)

> Also, if we add all the dirs in `load-path` to the set of readable
> directories (and I do think we want to do that) this should not be
> an issue, right?

Certainly, as long as we run the code defining the autoloads to begin with.

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

>> I don't understand why it would be different for autoloads than for
>> functions provided by `require`d packages.
> It was suggested that no init files be loaded to make the Emacs subprocess
> start faster. (I'm neutral here.)

I still have no idea what this has to do with autoloads.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

20 dec. 2020 kl. 15.02 skrev Stefan Monnier <monnier@iro.umontreal.ca>:
> 
>> It was suggested that no init files be loaded to make the Emacs subprocess
>> start faster. (I'm neutral here.)
> 
> I still have no idea what this has to do with autoloads.

If Emacs does not read the user's init file in the checking process, then autoloads for external packages are not defined and the byte-compiler will complain when encountering calls to functions defined in external packages not explicitly required. Other people seem to believe it's not a big deal; I'm unsure.

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

>>> It was suggested that no init files be loaded to make the Emacs subprocess
>>> start faster. (I'm neutral here.)
>> I still have no idea what this has to do with autoloads.
> If Emacs does not read the user's init file in the checking process, then
> autoloads for external packages are not defined and the byte-compiler will
> complain when encountering calls to functions defined in external packages
> not explicitly required. Other people seem to believe it's not a big deal;
> I'm unsure.

Oh, I think I see what you mean.  I suspect that "autoload" is not the
crux of the matter (and "init file" isn't either) and we'll only know
how important the issue is and how best to fix it with more experience.
E.g. maybe the better option will be for `elisp-flymake-byte-compile` to
try and pass down some of that contextual info
(e.g. `package-directory-list` and `load-path`) to the subprocess, or
maybe we'll want `elisp-flymake-byte-compile` to recognize the
`Package-Requires:` header, ...

I think the more important pressing issue will be to distinguish "config
file" (i.e. packages whose main purpose is to change the behavior of
Emacs when they're loaded) from "ELisp package file" (packages which,
according to the coding convention, should not change the behavior of
Emacs when loaded), since config files are likely to be full of false
positives that can't be conveniently fixed by the user whereas for
genuine ELisp package files we have a range of tools to fix
the warnings.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 19 Dec 2020 23:22:08 +0100
> Cc: Bastien <bzg@gnu.org>, 45198@debbugs.gnu.org,
>  João Távora <joaotavora@gmail.com>
> 
> diff --git a/configure.ac b/configure.ac
> index 087dd67e18..cb698e34fe 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -4186,6 +4186,11 @@ AC_DEFUN
>  
>  AC_CHECK_HEADERS([linux/seccomp.h])
>  
> +LIBSECCOMP=
> +AC_CHECK_HEADER([seccomp.h],
> +  [AC_CHECK_LIB([seccomp], [seccomp_init], [LIBSECCOMP=-lseccomp])])
> +AC_SUBST([LIBSECCOMP])
> +

Please also add this to the list in EMACS_CONFIG_FEATURES.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am So., 20. Dez. 2020 um 16:10 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sat, 19 Dec 2020 23:22:08 +0100
> > Cc: Bastien <bzg@gnu.org>, 45198@debbugs.gnu.org,
> >  João Távora <joaotavora@gmail.com>
> >
> > diff --git a/configure.ac b/configure.ac
> > index 087dd67e18..cb698e34fe 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -4186,6 +4186,11 @@ AC_DEFUN
> >
> >  AC_CHECK_HEADERS([linux/seccomp.h])
> >
> > +LIBSECCOMP=
> > +AC_CHECK_HEADER([seccomp.h],
> > +  [AC_CHECK_LIB([seccomp], [seccomp_init], [LIBSECCOMP=-lseccomp])])
> > +AC_SUBST([LIBSECCOMP])
> > +
>
> Please also add this to the list in EMACS_CONFIG_FEATURES.

Hmm, this isn't really an Emacs feature, just an internal helper
binary, so I don't think it should be in any features list.

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sun, 20 Dec 2020 19:14:07 +0100
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Bastien <bzg@gnu.org>, 45198@debbugs.gnu.org, 
> 	João Távora <joaotavora@gmail.com>
> 
> Am So., 20. Dez. 2020 um 16:10 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> >
> > > +LIBSECCOMP=
> > > +AC_CHECK_HEADER([seccomp.h],
> > > +  [AC_CHECK_LIB([seccomp], [seccomp_init], [LIBSECCOMP=-lseccomp])])
> > > +AC_SUBST([LIBSECCOMP])
> > > +
> >
> > Please also add this to the list in EMACS_CONFIG_FEATURES.
> 
> Hmm, this isn't really an Emacs feature, just an internal helper
> binary, so I don't think it should be in any features list.

What about the --seccomp command-line switch to Emacs: isn't its
support a feature that should be called out?

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am So., 20. Dez. 2020 um 19:29 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sun, 20 Dec 2020 19:14:07 +0100
> > Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Bastien <bzg@gnu.org>, 45198@debbugs.gnu.org,
> >       João Távora <joaotavora@gmail.com>
> >
> > Am So., 20. Dez. 2020 um 16:10 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> > >
> > > > +LIBSECCOMP=
> > > > +AC_CHECK_HEADER([seccomp.h],
> > > > +  [AC_CHECK_LIB([seccomp], [seccomp_init], [LIBSECCOMP=-lseccomp])])
> > > > +AC_SUBST([LIBSECCOMP])
> > > > +
> > >
> > > Please also add this to the list in EMACS_CONFIG_FEATURES.
> >
> > Hmm, this isn't really an Emacs feature, just an internal helper
> > binary, so I don't think it should be in any features list.
>
> What about the --seccomp command-line switch to Emacs: isn't its
> support a feature that should be called out?

OK, that's in a different patch though. I'll add a feature for it in
that patch then.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am So., 20. Dez. 2020 um 13:28 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> > If/when someone implements that, then indeed we can just use a process
> > object to represent the parent in the client as well.
> >
>
> Yes, but again, there's no difference between the standard streams and
> using newly-allocated file descriptors. In both cases, you need a
> variant of Fmake_pipe_process that doesn't call pipe2 twice, but wraps
> two existing file descriptors in its infd and outfd. Whether those
> happen to be 0 and 1 or something passed on the command line makes no
> difference.
> On the parent process side, if you want to use a separate pipe pair,
> you need a way to create a pipe process that doesn't use O_CLOEXEC and
> allows reading out the open file descriptors to be able to pass them
> to the subprocess on the command line.
> These changes aren't large, but they are necessary if you want to go
> the "extra pipe pair" route.

I've played around with this a bit (both with the pipe pair and with
the socketpair approach), but one issue is that Emacs doesn't know
about half-closed processes (that you can only write to, but not read
from). Such a state is crucial because the subprocesses will want to
read the entire input before sending output, which isn't possible if
the output gets closed after EOF from the input (and that's what
wait_reading_process_output does for both pipes and sockets). So we'd
need to introduce the 'half-closed' process state first, which
requires somewhat larger changes to Emacs's process design and
interface.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Sa., 19. Dez. 2020 um 18:19 Uhr schrieb Mattias Engdegård <mattiase@acm.org>:
>
> 19 dec. 2020 kl. 16.08 skrev Philipp Stephani <p.stephani2@gmail.com>:
>
> > I'd say these two questions are somewhat independent of each other.
> > Even with an internal-only interface, people will start to assume that
> > reading arbitrary files works.
> > I'm personally not a huge fan of such internal interfaces though. They
> > are necessary in some cases, but a high-level UI framework like
> > Flymake shouldn't need to use them. Besides, since Flymake is released
> > as an external package, it should rather not use internal interfaces
> > in the first place.
>
> What I meant is that there is no way of knowing whether an API is rubbish or not without having put it to use ourselves first (preferably in two or more ways), so let's not front-load the design. We know that this is true regardless of how good programmers we think we are.
> Flymake would be a natural user, but it must cope with our own demands first.

I agree, but we should use the time until Emacs 28 gets released to
gain experience with the API as well, so we should design the API
rather sooner than later, because once Emacs 28 is released, we can't
change it any more in incompatible ways.

> There's a difference though: flycheck is installed by someone who wants to use it and is presumably ready for some setting-up. In contrast, we are aiming at an on-by-default zero-configuration Emacs feature, which means that the bar is higher. It's meant precisely for those who would not install and configure flycheck, so false positives may have effects opposite the intended.

Yes, we should aim for a low false-positive rate, but it doesn't have
to be zero.
As I said, the false-positive rate (or rather, false-discovery rate)
for C currently is often 100%, so maybe we should start with that
first.

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

> Such a state is crucial because the subprocesses will want to
> read the entire input before sending output, which isn't possible if

I wouldn't presume this.  If/when such a thing is needed, it's probably
easier to pass the input via a temp file.  E.g. that's what
`elisp-flymake-byte-compile` does.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Stefan Kangas]

Philipp Stephani <p.stephani2@gmail.com> writes:

> I agree, but we should use the time until Emacs 28 gets released to
> gain experience with the API as well, so we should design the API
> rather sooner than later, because once Emacs 28 is released, we can't
> change it any more in incompatible ways.

IMO, we could release it as an experimental feature and prominently
announce that API changes might happen between major versions of Emacs.
That would give us room to make even backward-incompatible changes,
if/when necessary.

I don't necessarily advocate this; I only want to point out that this is
an option.

(Of course, this would not preclude trying to make it as stable as
possible before Emacs 28.)

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Sa., 19. Dez. 2020 um 23:22 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Am Mo., 14. Dez. 2020 um 12:05 Uhr schrieb Philipp Stephani
> <p.stephani2@gmail.com>:
>
> > > >> - This will need someone else doing the implementation.
> > > > Looks like we already have a volunteer for macOS.
> > > > For Linux, this shouldn't be that difficult either. The sandbox needs
> > > > to install a mount namespace that only allows read access to Emacs's
> > > > installation directory plus any input file and write access to known
> > > > output files, and enable syscall filters that forbid everything except
> > > > a list of known-safe syscalls (especially exec). I can take a stab at
> > > > that, but I can't promise anything ;-)
> > >
> > > Looking forward to it.
> > >
> >
> > I've looked into this, and what I'd suggest for now is:
> > […]
> > 2. Generate appropriate seccomp filters using libseccomp or similar.
>
> Here's a patch for this step.

I've pushed a modified version of these two patches onto the
scratch/seccomp branch. It would be great if someone could test
whether the Windows build still works after that, thanks!

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Mo., 28. Dez. 2020 um 09:23 Uhr schrieb Stefan Kangas
<stefankangas@gmail.com>:
>
> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> > I agree, but we should use the time until Emacs 28 gets released to
> > gain experience with the API as well, so we should design the API
> > rather sooner than later, because once Emacs 28 is released, we can't
> > change it any more in incompatible ways.
>
> IMO, we could release it as an experimental feature and prominently
> announce that API changes might happen between major versions of Emacs.
> That would give us room to make even backward-incompatible changes,
> if/when necessary.
>
> I don't necessarily advocate this; I only want to point out that this is
> an option.

It's an option, though I'm not sure whether such announcements really
work all that well. Once an API becomes widely used, it becomes hard
to change it, even if it was announced to be unstable. Thus I'd
advocate for starting with the most conservative and malleable
approach possible:
- Don't allow reading the entire filesystem, but only selected files
and directories.
- Don't allow writing files (for now), communication should happen
through stdout. (That's probably good enough for Flymake, but soon
we'll need to find a more flexible approach.)
- Don't return a process object, but an opaque sandbox object.

For example:

(cl-defun start-sandbox (function &key readable-directories stdout-buffer) ...)
(defun wait-for-sandbox (sandbox) ...)

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Tue, 29 Dec 2020 14:50:56 +0100
> Cc: Bastien <bzg@gnu.org>, 45198@debbugs.gnu.org,
>  João Távora <joaotavora@gmail.com>
> 
> I've pushed a modified version of these two patches onto the
> scratch/seccomp branch.

Thank you for working on this.

> It would be great if someone could test whether the Windows build
> still works after that, thanks!

I only skimmed the changes, but I'm already quite sure they will break
the Windows build.

This feature is not relevant to the MS-Windows build of Emacs, at
least currently (I don't think Windows implements equivalent features
in a way that is even remotely similar to GNU/Linux).  So to make sure
the Windows port doesn't break, we must take every measure to avoid
compiling any of the related code on MS-Windows.  In particular:

  . Gnulib modules pulled to support seccomp should be disabled in the
    MS-Windows build, by suitable changes to nt/mingw-cfg.site and/or
    nt/gnulib-cfg.mk; and
  . header files used to support seccomp code should be #ifdef'ed by
    HAVE_LINUX_SECCOMP_H or similar, and likewise with any code needed
    for seccomp and unnecessary otherwise.

Btw, I wonder why you needed to import the read-file module from
Gnulib -- does it provide any features that we couldn't implement on
our own?  I'm asking because that module caused you to pull in quite a
few dependency modules from Gnulib, and I'm asking myself whether that
is really justified.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Di., 29. Dez. 2020 um 16:44 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > It would be great if someone could test whether the Windows build
> > still works after that, thanks!
>
> I only skimmed the changes, but I'm already quite sure they will break
> the Windows build.

Why? Everything should be behind ifdefs/conditional compilation,
otherwise compilation on macOS would also break.

>
> This feature is not relevant to the MS-Windows build of Emacs, at
> least currently (I don't think Windows implements equivalent features
> in a way that is even remotely similar to GNU/Linux).  So to make sure
> the Windows port doesn't break, we must take every measure to avoid
> compiling any of the related code on MS-Windows.  In particular:
>
>   . Gnulib modules pulled to support seccomp should be disabled in the
>     MS-Windows build, by suitable changes to nt/mingw-cfg.site and/or
>     nt/gnulib-cfg.mk; and

This shouldn't be necessary, as Gnulib is compatible with Windows (I
guess, since we use it elsewhere), and the MS C library provides an
emulation layer for some parts of the POSIX API (e.g. file
descriptors). OTOH, conditional compilation incurs a maintenance cost,
so we should avoid it if possible.
(That's also what gnulib-cfg.mk says: "In general, do NOT omit modules
that don't need to be omitted, to minimize the differences from
upstream gnulib.mk and thus make the maintenance easier.")

>   . header files used to support seccomp code should be #ifdef'ed by
>     HAVE_LINUX_SECCOMP_H or similar, and likewise with any code needed
>     for seccomp and unnecessary otherwise.

That should already be the case.
Turning the question around: is building the branch on Windows
actually broken? If so, what are the error messages?

>
> Btw, I wonder why you needed to import the read-file module from
> Gnulib -- does it provide any features that we couldn't implement on
> our own?  I'm asking because that module caused you to pull in quite a
> few dependency modules from Gnulib, and I'm asking myself whether that
> is really justified.

We could implement it ourselves, if we wanted, and in an earlier
version of the code I did that. But it's easier and less error-prone
to reuse an existing library.

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Tue, 29 Dec 2020 17:05:33 +0100
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Bastien <bzg@gnu.org>, 45198@debbugs.gnu.org, 
> 	João Távora <joaotavora@gmail.com>
> 
> Am Di., 29. Dez. 2020 um 16:44 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> >
> > > It would be great if someone could test whether the Windows build
> > > still works after that, thanks!
> >
> > I only skimmed the changes, but I'm already quite sure they will break
> > the Windows build.
> 
> Why? Everything should be behind ifdefs/conditional compilation,
> otherwise compilation on macOS would also break.

Because some/many Gnulib replacements are incompatible with how the
w32 port of Emacs works.  In particular, functions which operate on
file names don't support UTF-8 encoded file names; Gnulib's 'stat' and
'fstat' don't support security-related features from which we glean
owner and group of files; network-related functions need special
handling in Emacs to support subprocess functionality; etc.

> >   . Gnulib modules pulled to support seccomp should be disabled in the
> >     MS-Windows build, by suitable changes to nt/mingw-cfg.site and/or
> >     nt/gnulib-cfg.mk; and
> 
> This shouldn't be necessary, as Gnulib is compatible with Windows (I
> guess, since we use it elsewhere)

Not when Emacs is concerned, see above.  We use only small parts of
Gnulib on Windows, for those reasons.  And we don't use any Gnulib
functions that accept file names.

> (That's also what gnulib-cfg.mk says: "In general, do NOT omit modules
> that don't need to be omitted, to minimize the differences from
> upstream gnulib.mk and thus make the maintenance easier.")

I know; I wrote that.  However, this talks about code that will be
actually _used_ in the Windows build, whereas here we are talking
about "ballast": the related code will not be used at all.  So it
makes sense not to make our job harder by adding unnecessary code,
which then will need to be audited for compatibility.  Your Gnulib
import adds quite a lot of modules, which makes the audit a large job.

> >   . header files used to support seccomp code should be #ifdef'ed by
> >     HAVE_LINUX_SECCOMP_H or similar, and likewise with any code needed
> >     for seccomp and unnecessary otherwise.
> 
> That should already be the case.

It isn't, at least not in general.  Just one example:

  diff --git a/src/emacs.c b/src/emacs.c
  index 2a32083..a044535 100644
  --- a/src/emacs.c
  +++ b/src/emacs.c
  @@ -33,6 +33,8 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
   #include "lisp.h"
   #include "sysstdio.h"

  +#include <read-file.h>  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

This header is included unconditionally, although it shouldn't be
needed on Windows.

> Turning the question around: is building the branch on Windows
> actually broken? If so, what are the error messages?

I didn't have time to build it, and probably won't have enough time in
the following days, sorry.  I have the co-maintainer job to do, and
there's a pretest of Emacs 27.2 under way on top of that.  So I cannot
show the error messages.  Others are encouraged to try the branch and
report the actual problems; I just wanted to give a heads-up, in the
hope that it will help adapting and merging the branch sooner.

> > Btw, I wonder why you needed to import the read-file module from
> > Gnulib -- does it provide any features that we couldn't implement on
> > our own?  I'm asking because that module caused you to pull in quite a
> > few dependency modules from Gnulib, and I'm asking myself whether that
> > is really justified.
> 
> We could implement it ourselves, if we wanted, and in an earlier
> version of the code I did that. But it's easier and less error-prone
> to reuse an existing library.

I question the wisdom of that, FWIW.  Every unnecessary dependency on
Gnulib is IMO an addition to the maintenance burden.  It also makes
the job of adapting the Windows build harder, because for example
Gnulib's fopen cannot be used in Emacs on Windows, whereas we have
emacs_fopen for the same purpose, which doesn't have that problem.

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 279 bytes --]

Here is a bare-bones macOS sandbox implementation. In practice, it would probably be called in an --eval argument to guard anything executed later. It should be sufficient for the typical untrusted flymake checker running in an Emacs subprocess and printing to stdout/stderr.


[-- Attachment #2: macos-sandbox.diff --]
[-- Type: application/octet-stream, Size: 1986 bytes --]

diff --git a/lisp/subr.el b/lisp/subr.el
index ed0d6978d0..729c4ac70b 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -6036,4 +6036,18 @@ internal--format-docstring-line
 This is intended for internal use only."
   (internal--fill-string-single-line (apply #'format string objects)))
 
+(defun sandbox-enter (dirs)
+  "Enter a sandbox only permitting reading files under DIRS.
+DIRS is a list of directory names.  Most other operations such as
+writing files and network access are disallowed.
+Existing open descriptors can still be used freely."
+  (unless (eq system-type 'darwin)
+    (error "not implemented on this platform"))
+  (macos-sandbox-init
+   (concat "(version 1)\n"
+           "(deny default)\n"
+           (mapconcat (lambda (dir)
+                        (format "(allow file-read* (subpath %S))\n" dir))
+                      dirs ""))))
+
 ;;; subr.el ends here
diff --git a/src/sysdep.c b/src/sysdep.c
index eeb9d18494..3b2da8c637 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -4054,8 +4054,33 @@ str_collate (Lisp_Object s1, Lisp_Object s2,
 }
 #endif	/* WINDOWSNT */
 
+#ifdef DARWIN_OS
+
+/* This call is not in the platform header files.  You just Have to Know. */
+int sandbox_init_with_parameters(const char *profile,
+                                 uint64_t flags,
+                                 const char *const parameters[],
+                                 char **errorbuf);
+
+DEFUN ("macos-sandbox-init", Fmacos_sandbox_init, Smacos_sandbox_init,
+       1, 1, 0,
+       doc: /* Enter a sandbox whose permitted access is curtailed by PROFILE.
+Already open descriptors can be used freely. */)
+  (Lisp_Object profile)
+{
+  char *err = NULL;
+  if (sandbox_init_with_parameters (SSDATA (profile), 0, NULL, &err) != 0)
+    error ("sandbox error: %s", err);
+  return Qnil;
+}
+
+#endif	/* DARWIN_OS */
+
 void
 syms_of_sysdep (void)
 {
   defsubr (&Sget_internal_run_time);
+#ifdef DARWIN_OS
+  defsubr (&Smacos_sandbox_init);
+#endif
 }

bug#45198: 28.0.50; Sandbox mode

[Alan Third]

On Wed, Dec 30, 2020 at 03:59:19PM +0100, Mattias Engdegård wrote:
> Here is a bare-bones macOS sandbox implementation. In practice, it
> would probably be called in an --eval argument to guard anything
> executed later. It should be sufficient for the typical untrusted
> flymake checker running in an Emacs subprocess and printing to
> stdout/stderr.

It may make more sense to use darwin instead of macos in the name,
unless it is actually specific to macOS.

I believe OpenDarwin is still a thing.

-- 
Alan Third

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Di., 29. Dez. 2020 um 18:09 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Tue, 29 Dec 2020 17:05:33 +0100
> > Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Bastien <bzg@gnu.org>, 45198@debbugs.gnu.org,
> >       João Távora <joaotavora@gmail.com>
> >
> > Am Di., 29. Dez. 2020 um 16:44 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> > >
> > > > It would be great if someone could test whether the Windows build
> > > > still works after that, thanks!
> > >
> > > I only skimmed the changes, but I'm already quite sure they will break
> > > the Windows build.
> >
> > Why? Everything should be behind ifdefs/conditional compilation,
> > otherwise compilation on macOS would also break.
>
> Because some/many Gnulib replacements are incompatible with how the
> w32 port of Emacs works.  In particular, functions which operate on
> file names don't support UTF-8 encoded file names; Gnulib's 'stat' and
> 'fstat' don't support security-related features from which we glean
> owner and group of files; network-related functions need special
> handling in Emacs to support subprocess functionality; etc.

That's a fair point, but does the mere presence of these replacements
really break the build? Could we somehow make them not break the
build?

>
> > >   . Gnulib modules pulled to support seccomp should be disabled in the
> > >     MS-Windows build, by suitable changes to nt/mingw-cfg.site and/or
> > >     nt/gnulib-cfg.mk; and
> >
> > This shouldn't be necessary, as Gnulib is compatible with Windows (I
> > guess, since we use it elsewhere)
>
> Not when Emacs is concerned, see above.  We use only small parts of
> Gnulib on Windows, for those reasons.  And we don't use any Gnulib
> functions that accept file names.
>
> > (That's also what gnulib-cfg.mk says: "In general, do NOT omit modules
> > that don't need to be omitted, to minimize the differences from
> > upstream gnulib.mk and thus make the maintenance easier.")
>
> I know; I wrote that.  However, this talks about code that will be
> actually _used_ in the Windows build, whereas here we are talking
> about "ballast": the related code will not be used at all.  So it
> makes sense not to make our job harder by adding unnecessary code,
> which then will need to be audited for compatibility.  Your Gnulib
> import adds quite a lot of modules, which makes the audit a large job.

Independent of this discussion, are there ways to reduce the size of
the auditing job? I'd hope that eventually the only thing needed would
be to run the test suite (which ideally would happen automatically on
Gitlab/Emba).
For developers not on Windows, it's unfortunately difficult to predict
whether a specific change will break the Windows build or not, and the
large difference between the Windows and other builds doesn't make
this easier.

>
> > >   . header files used to support seccomp code should be #ifdef'ed by
> > >     HAVE_LINUX_SECCOMP_H or similar, and likewise with any code needed
> > >     for seccomp and unnecessary otherwise.
> >
> > That should already be the case.
>
> It isn't, at least not in general.  Just one example:
>
>   diff --git a/src/emacs.c b/src/emacs.c
>   index 2a32083..a044535 100644
>   --- a/src/emacs.c
>   +++ b/src/emacs.c
>   @@ -33,6 +33,8 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
>    #include "lisp.h"
>    #include "sysstdio.h"
>
>   +#include <read-file.h>  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>
> This header is included unconditionally, although it shouldn't be
> needed on Windows.

Sure, it's not needed, but does it actually break the build? I'd
rather limit the number of preprocessor statements as much as possible
(and reasonable).

>
> > Turning the question around: is building the branch on Windows
> > actually broken? If so, what are the error messages?
>
> I didn't have time to build it, and probably won't have enough time in
> the following days, sorry.  I have the co-maintainer job to do, and
> there's a pretest of Emacs 27.2 under way on top of that.  So I cannot
> show the error messages.  Others are encouraged to try the branch and
> report the actual problems; I just wanted to give a heads-up, in the
> hope that it will help adapting and merging the branch sooner.

Thanks, no pressure, this clearly isn't urgent.

>
> > > Btw, I wonder why you needed to import the read-file module from
> > > Gnulib -- does it provide any features that we couldn't implement on
> > > our own?  I'm asking because that module caused you to pull in quite a
> > > few dependency modules from Gnulib, and I'm asking myself whether that
> > > is really justified.
> >
> > We could implement it ourselves, if we wanted, and in an earlier
> > version of the code I did that. But it's easier and less error-prone
> > to reuse an existing library.
>
> I question the wisdom of that, FWIW.  Every unnecessary dependency on
> Gnulib is IMO an addition to the maintenance burden.  It also makes
> the job of adapting the Windows build harder, because for example
> Gnulib's fopen cannot be used in Emacs on Windows, whereas we have
> emacs_fopen for the same purpose, which doesn't have that problem.

Yeah, there are pros and cons for either approach. Typically, reusing
code is the way to go, since code is a liability, and the less we have
to maintain, the better. But I do see your point that using Gnulib is
too much of a burden for Windows right now (though I still hope that
we can eventually improve Gnulib enough that we can use it directly on
Windows). I've now created a new branch scratch/seccomp-no-gnulib-2
that uses fopen directly, without Gnulib dependencies. (I considered
using emacs_fopen, but since that allows users to quit and therefore
in theory can run Lisp code, I'm not sure we can use it here. Also,
since this only works on GNU/Linux, we don't have to deal with Windows
filenames.)

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Thu, 31 Dec 2020 16:05:52 +0100
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Bastien <bzg@gnu.org>, 45198@debbugs.gnu.org, 
> 	João Távora <joaotavora@gmail.com>
> 
> Am Di., 29. Dez. 2020 um 18:09 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> >
> > Because some/many Gnulib replacements are incompatible with how the
> > w32 port of Emacs works.  In particular, functions which operate on
> > file names don't support UTF-8 encoded file names; Gnulib's 'stat' and
> > 'fstat' don't support security-related features from which we glean
> > owner and group of files; network-related functions need special
> > handling in Emacs to support subprocess functionality; etc.
> 
> That's a fair point, but does the mere presence of these replacements
> really break the build?

If the function compiles cleanly and is not used on MS-Windows, then
no, the build won't break.

> Could we somehow make them not break the build?

It's not up to us, because we don't maintain Gnulib code.  And it
isn't always practical, since the Windows port of Emacs has its own
replacements for some Posix functionality that conflict with the
Gnulib replacements.

> > I know; I wrote that.  However, this talks about code that will be
> > actually _used_ in the Windows build, whereas here we are talking
> > about "ballast": the related code will not be used at all.  So it
> > makes sense not to make our job harder by adding unnecessary code,
> > which then will need to be audited for compatibility.  Your Gnulib
> > import adds quite a lot of modules, which makes the audit a large job.
> 
> Independent of this discussion, are there ways to reduce the size of
> the auditing job? I'd hope that eventually the only thing needed would
> be to run the test suite (which ideally would happen automatically on
> Gitlab/Emba).

The test suite is doesn't yet have enough coverage.  And if Emacs
fails to build, the test suite cannot help.

> For developers not on Windows, it's unfortunately difficult to predict
> whether a specific change will break the Windows build or not, and the
> large difference between the Windows and other builds doesn't make
> this easier.

Sure, that's when posting a patch or a separate branch are a good
means to let the code be fixed before it lands.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Sa., 19. Dez. 2020 um 19:18 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Am Mo., 14. Dez. 2020 um 12:05 Uhr schrieb Philipp Stephani
> <p.stephani2@gmail.com>:
> >
> > > >> - This will need someone else doing the implementation.
> > > > Looks like we already have a volunteer for macOS.
> > > > For Linux, this shouldn't be that difficult either. The sandbox needs
> > > > to install a mount namespace that only allows read access to Emacs's
> > > > installation directory plus any input file and write access to known
> > > > output files, and enable syscall filters that forbid everything except
> > > > a list of known-safe syscalls (especially exec). I can take a stab at
> > > > that, but I can't promise anything ;-)
> > >
> > > Looking forward to it.
> > >
> >
> > I've looked into this, and what I'd suggest for now is:
> > 1. Add a --seccomp=FILE command-line option that loads seccomp filters
> > from FILE and applies them directly after startup (first thing in
> > main). Why do this in Emacs? Because that's the easiest way to prevent
> > execve. When installing a seccomp filter in a separate process, execve
> > needs to be allowed because otherwise there'd be no way to execute the
> > Emacs binary. While there are workarounds (ptrace, LD_PRELOAD), it's
> > easiest to install the seccomp filter directly in the Emacs process.
>
> I've attached a patch for this.

I've verified that a slight variant of this patch doesn't break either
the Windows or macOS builds, and pushed it to master as commit
be8328acf9aa464f848e682e63e417a18529af9e.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Sa., 19. Dez. 2020 um 23:22 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Am Mo., 14. Dez. 2020 um 12:05 Uhr schrieb Philipp Stephani
> <p.stephani2@gmail.com>:
>
> > > >> - This will need someone else doing the implementation.
> > > > Looks like we already have a volunteer for macOS.
> > > > For Linux, this shouldn't be that difficult either. The sandbox needs
> > > > to install a mount namespace that only allows read access to Emacs's
> > > > installation directory plus any input file and write access to known
> > > > output files, and enable syscall filters that forbid everything except
> > > > a list of known-safe syscalls (especially exec). I can take a stab at
> > > > that, but I can't promise anything ;-)
> > >
> > > Looking forward to it.
> > >
> >
> > I've looked into this, and what I'd suggest for now is:
> > […]
> > 2. Generate appropriate seccomp filters using libseccomp or similar.
>
> Here's a patch for this step.

I've now pushed a variant of this patch as commit
1060289f51ee1bf269bb45940892eb272d35af97, after verifying that it
doesn't break the macOS or Windows builds.

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 344 bytes --]

Slightly updated patch for macOS. Obviously not nearly as fancy as the seccomp one but for running something in batch mode that reads from files and writes to stdout/stderr it should do.

It works and can be pushed right away but it would be nice to have a place to use it, for validation and for tuning the interface. Any plans for that?


[-- Attachment #2: darwin-sandbox.diff --]
[-- Type: application/octet-stream, Size: 5785 bytes --]

diff --git a/lisp/subr.el b/lisp/subr.el
index c2be26a15f..4994771c33 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -6262,4 +6262,20 @@ internal--format-docstring-line
 This is intended for internal use only."
   (internal--fill-string-single-line (apply #'format string objects)))
 
+(when (eq system-type 'darwin)
+  (defun sandbox-enter (dirs)
+    "Enter a sandbox only permitting reading files under DIRS.
+DIRS is a list of directory names.  Most other operations such as
+writing files and network access are disallowed.
+Existing open descriptors can still be used freely."
+    (darwin-sandbox-init
+     (concat "(version 1)\n"
+             "(deny default)\n"
+             ;; Emacs seems to need /dev/null; allowing it does no harm.
+             "(allow file-read* (path \"/dev/null\"))\n"
+             (mapconcat (lambda (dir)
+                          (format "(allow file-read* (subpath %S))\n" dir))
+                        dirs ""))))
+  )
+
 ;;; subr.el ends here
diff --git a/src/sysdep.c b/src/sysdep.c
index d940acc4e0..b6c402ba33 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -4286,8 +4286,33 @@ str_collate (Lisp_Object s1, Lisp_Object s2,
 }
 #endif	/* WINDOWSNT */
 
+#ifdef DARWIN_OS
+
+/* This function prototype is not in the platform header files. */
+int sandbox_init_with_parameters(const char *profile,
+                                 uint64_t flags,
+                                 const char *const parameters[],
+                                 char **errorbuf);
+
+DEFUN ("darwin-sandbox-init", Fdarwin_sandbox_init, Sdarwin_sandbox_init,
+       1, 1, 0,
+       doc: /* Enter a sandbox whose permitted access is curtailed by PROFILE.
+Already open descriptors can be used freely. */)
+  (Lisp_Object profile)
+{
+  char *err = NULL;
+  if (sandbox_init_with_parameters (SSDATA (profile), 0, NULL, &err) != 0)
+    error ("sandbox error: %s", err);
+  return Qnil;
+}
+
+#endif	/* DARWIN_OS */
+
 void
 syms_of_sysdep (void)
 {
   defsubr (&Sget_internal_run_time);
+#ifdef DARWIN_OS
+  defsubr (&Sdarwin_sandbox_init);
+#endif
 }
diff --git a/test/src/emacs-tests.el b/test/src/emacs-tests.el
index 09f9a248ef..a1dc7f3501 100644
--- a/test/src/emacs-tests.el
+++ b/test/src/emacs-tests.el
@@ -210,4 +210,74 @@ emacs-tests/bwrap/allows-stdout
           (should (eql status 0)))
         (should (equal (string-trim (buffer-string)) "Hi"))))))
 
+(defun emacs-tests--darwin-run-sandboxed-emacs (sandbox-dirs body)
+  "Run Emacs and evaluate BODY, only allowing reads from SANDBOX-DIRS.
+If SANDBOX-DIRS is `no-sandbox', then run without sandbox.
+Return (EXIT-STATUS . OUTPUT), where OUTPUT is stderr and stdout."
+  (let ((emacs (expand-file-name invocation-name invocation-directory))
+        (process-environment nil))
+    (with-temp-buffer
+      (let* ((prog `(progn
+                      ,@(and (not (eq sandbox-dirs 'no-sandbox))
+                             `((sandbox-enter ',sandbox-dirs)))
+                      ,@body))
+             (res (call-process emacs nil t nil
+                                "--quick" "--batch"
+                                (format "--eval=%S" prog))))
+        (cons res (buffer-string))))))
+
+(ert-deftest emacs-tests/darwin-sandbox ()
+  (skip-unless (eq system-type 'darwin))
+  (emacs-tests--with-temp-file test-file ("test")
+    (let ((some-text "abcdef")
+          (other-text "ghijkl")
+          (test-file (file-truename test-file)))   ; resolve symlinks
+      (with-temp-buffer
+        (insert some-text)
+        (write-file test-file))
+
+      ;; Read the file without allowing its directory -- should fail.
+      (let ((res-out (emacs-tests--darwin-run-sandboxed-emacs
+                      nil
+                      `((find-file-literally ,test-file)
+                        (message "OK: %s" (buffer-string))))))
+        (ert-info ((cdr res-out) :prefix "output: ")
+          (should-not (equal (car res-out) 0))
+          (should-not (string-search some-text (cdr res-out)))))
+
+      ;; Read the file allowing its directory -- should succeed.
+      (let ((res-out (emacs-tests--darwin-run-sandboxed-emacs
+                      (list (file-name-directory test-file))
+                      `((find-file-literally ,test-file)
+                        (message "OK: %s" (buffer-string))))))
+        (should (equal res-out (cons 0 (format "OK: %s\n" some-text)))))
+
+      ;; Write to the file allowing directory reads -- should fail.
+      (let ((res-out (emacs-tests--darwin-run-sandboxed-emacs
+                      (list (file-name-directory test-file))
+                      `((with-temp-buffer
+                          (insert ,other-text)
+                          (write-file ,test-file))))))
+        (ert-info ((cdr res-out) :prefix "output: ")
+          (should-not (equal (car res-out) 0))
+          ;; The file should be unchanged.
+          (let ((contents (with-temp-buffer
+                            (insert-file-literally test-file)
+                            (buffer-string))))
+            (should (equal contents some-text)))))
+
+      ;; Write to the file without sandbox -- should succeed.
+      (let ((res-out (emacs-tests--darwin-run-sandboxed-emacs
+                      'no-sandbox
+                      `((with-temp-buffer
+                          (insert ,other-text)
+                          (write-file ,test-file))))))
+        (ert-info ((cdr res-out) :prefix "output: ")
+          (should (equal (car res-out) 0))
+          ;; The file should be changed.
+          (let ((contents (with-temp-buffer
+                            (insert-file-literally test-file)
+                            (buffer-string))))
+            (should (equal contents other-text))))))))
+
 ;;; emacs-tests.el ends here

bug#45198: 28.0.50; Sandbox mode

[Philipp]

> Am 17.04.2021 um 17:26 schrieb Mattias Engdegård <mattiase@acm.org>:
> 
> Slightly updated patch for macOS. Obviously not nearly as fancy as the seccomp one but for running something in batch mode that reads from files and writes to stdout/stderr it should do.
> 
> It works and can be pushed right away but it would be nice to have a place to use it, for validation and for tuning the interface. Any plans for that?
> 

I think it would be better to first implement the mechanism and not the high-level `sandbox-enter' function (I think that one needs a bit more discussion), and implement the mechanism as a command-line flag.  This would not only be consistent with the Seccomp implementation, but also be somewhat more conservative in that it wouldn't require the sandboxing functionality to work in arbitrary running Emacs processes.  As we gain more experience with these sandboxing mechanisms, we can look at relaxing these restrictions, but I think initially we should be conservative.


> diff --git a/lisp/subr.el b/lisp/subr.el
> index c2be26a15f..4994771c33 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -6262,4 +6262,20 @@ internal--format-docstring-line
>  This is intended for internal use only."
>    (internal--fill-string-single-line (apply #'format string objects)))
>  
> +(when (eq system-type 'darwin)
> +  (defun sandbox-enter (dirs)
> +    "Enter a sandbox only permitting reading files under DIRS.
> +DIRS is a list of directory names.  Most other operations such as
> +writing files and network access are disallowed.
> +Existing open descriptors can still be used freely."
> +    (darwin-sandbox-init
> +     (concat "(version 1)\n"
> +             "(deny default)\n"
> +             ;; Emacs seems to need /dev/null; allowing it does no harm.
> +             "(allow file-read* (path \"/dev/null\"))\n"
> +             (mapconcat (lambda (dir)
> +                          (format "(allow file-read* (subpath %S))\n" dir))
> +                        dirs ""))))
> +  )
> +
>  ;;; subr.el ends here

I think it would be better to not commit to a high-level interface like `sandbox-enter' yet.  I intentionally held off adding such an interface in my patch because I think it deserves more discussion about the right design and interface.

> diff --git a/src/sysdep.c b/src/sysdep.c
> index d940acc4e0..b6c402ba33 100644
> --- a/src/sysdep.c
> +++ b/src/sysdep.c
> @@ -4286,8 +4286,33 @@ str_collate (Lisp_Object s1, Lisp_Object s2,
>  }
>  #endif	/* WINDOWSNT */
>  
> +#ifdef DARWIN_OS
> +
> +/* This function prototype is not in the platform header files. */

Is there any documentation you could refer to, even only an unofficial one?

> +int sandbox_init_with_parameters(const char *profile,
> +                                 uint64_t flags,
> +                                 const char *const parameters[],
> +                                 char **errorbuf);
> +
> +DEFUN ("darwin-sandbox-init", Fdarwin_sandbox_init, Sdarwin_sandbox_init,
> +       1, 1, 0,
> +       doc: /* Enter a sandbox whose permitted access is curtailed by PROFILE.

I think it would be better to define this as command-line flag, at least initially.  That way, the sandbox can protect code that happens early on, e.g. the startup code.

This needs to somehow document what PROFILE is.

> +Already open descriptors can be used freely. */)

What does this mean?  Emacs doesn't really expose file descriptors to users.

> +  (Lisp_Object profile)
> +{
> +  char *err = NULL;
> +  if (sandbox_init_with_parameters (SSDATA (profile), 0, NULL, &err) != 0)

Missing CHECK_STRING (profile).

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Philipp <p.stephani2@gmail.com>
> Date: Sat, 17 Apr 2021 17:44:06 +0200
> Cc: João Távora <joaotavora@gmail.com>,
>  45198@debbugs.gnu.org, Stefan Kangas <stefankangas@gmail.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>, Alan Third <alan@idiocy.org>
> 
> > It works and can be pushed right away but it would be nice to have a place to use it, for validation and for tuning the interface. Any plans for that?
> > 
> 
> I think it would be better to first implement the mechanism and not the high-level `sandbox-enter' function (I think that one needs a bit more discussion), and implement the mechanism as a command-line flag.

IMO, if we have no reasonably clear idea how this will be used on the
high level, it might make sense to move this to a feature branch,
instead of working on master.  That's because without a high-level
view, there's no way people could try implementing the same
functionality on platforms other than GNU/Linux, let alone use it.  So
from my POV, at this point this is just an experiment, and we have
feature branches for that.

bug#45198: 28.0.50; Sandbox mode

[Philipp]

> Am 17.04.2021 um 17:57 schrieb Eli Zaretskii <eliz@gnu.org>:
> 
>> From: Philipp <p.stephani2@gmail.com>
>> Date: Sat, 17 Apr 2021 17:44:06 +0200
>> Cc: João Távora <joaotavora@gmail.com>,
>> 45198@debbugs.gnu.org, Stefan Kangas <stefankangas@gmail.com>,
>> Stefan Monnier <monnier@iro.umontreal.ca>, Alan Third <alan@idiocy.org>
>> 
>>> It works and can be pushed right away but it would be nice to have a place to use it, for validation and for tuning the interface. Any plans for that?
>>> 
>> 
>> I think it would be better to first implement the mechanism and not the high-level `sandbox-enter' function (I think that one needs a bit more discussion), and implement the mechanism as a command-line flag.
> 
> IMO, if we have no reasonably clear idea how this will be used on the
> high level,

I have a relatively clear idea how I want the high-level interface to look like:

(cl-defun start-sandbox (function &key readable-directories stdout-buffer) ...)
(defun wait-for-sandbox (sandbox) ...)

where start-sandbox returns an opaque sandbox object running FUNCTION that wait-for-sandbox can wait for.  That should be generic enough that it's extensible and implementable on several platforms, and doesn't lock us into specific implementation choices.

If that's OK with everyone, then I'm happy to write the code for it.

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Philipp <p.stephani2@gmail.com>
> Date: Sat, 17 Apr 2021 18:10:14 +0200
> Cc: mattiase@acm.org,
>  joaotavora@gmail.com,
>  45198@debbugs.gnu.org,
>  stefankangas@gmail.com,
>  monnier@iro.umontreal.ca,
>  alan@idiocy.org
> 
> > IMO, if we have no reasonably clear idea how this will be used on the
> > high level,
> 
> I have a relatively clear idea how I want the high-level interface to look like:
> 
> (cl-defun start-sandbox (function &key readable-directories stdout-buffer) ...)
> (defun wait-for-sandbox (sandbox) ...)
> 
> where start-sandbox returns an opaque sandbox object running FUNCTION that wait-for-sandbox can wait for.  That should be generic enough that it's extensible and implementable on several platforms, and doesn't lock us into specific implementation choices.
> 
> If that's OK with everyone, then I'm happy to write the code for it.

I'm sorry, but I don't really understand what the above means in
practice.

What I'm missing is some details about what operations (in Emacs
terms) should not be allowed in the sandbox, and how can users take
advantage of that.  I asked more questions about this a few days ago,
but got no responses.  I don't really understand how we can
intelligently talk about using this in Emacs while we remain on the
level of file descriptors and syscalls.

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> Date: Sat, 17 Apr 2021 19:15:06 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: alan@idiocy.org, mattiase@acm.org, 45198@debbugs.gnu.org,
>  stefankangas@gmail.com, joaotavora@gmail.com, monnier@iro.umontreal.ca
> 
> What I'm missing is some details about what operations (in Emacs
> terms) should not be allowed in the sandbox, and how can users take
> advantage of that.  I asked more questions about this a few days ago,
> but got no responses.  I don't really understand how we can
> intelligently talk about using this in Emacs while we remain on the
> level of file descriptors and syscalls.

Btw, please don't interpret the above as pressure to get these issues
discussed and figured out.  There's no pressure; all I'm saying is
that if this is preliminary code for which we don't yet have a clear
common idea about its practical usage, it should be on a branch.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Sa., 17. Apr. 2021 um 18:15 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp <p.stephani2@gmail.com>
> > Date: Sat, 17 Apr 2021 18:10:14 +0200
> > Cc: mattiase@acm.org,
> >  joaotavora@gmail.com,
> >  45198@debbugs.gnu.org,
> >  stefankangas@gmail.com,
> >  monnier@iro.umontreal.ca,
> >  alan@idiocy.org
> >
> > > IMO, if we have no reasonably clear idea how this will be used on the
> > > high level,
> >
> > I have a relatively clear idea how I want the high-level interface to look like:
> >
> > (cl-defun start-sandbox (function &key readable-directories stdout-buffer) ...)
> > (defun wait-for-sandbox (sandbox) ...)
> >
> > where start-sandbox returns an opaque sandbox object running FUNCTION that wait-for-sandbox can wait for.  That should be generic enough that it's extensible and implementable on several platforms, and doesn't lock us into specific implementation choices.
> >
> > If that's OK with everyone, then I'm happy to write the code for it.
>
> I'm sorry, but I don't really understand what the above means in
> practice.
>
> What I'm missing is some details about what operations (in Emacs
> terms) should not be allowed in the sandbox, and how can users take
> advantage of that.  I asked more questions about this a few days ago,
> but got no responses.  I don't really understand how we can
> intelligently talk about using this in Emacs while we remain on the
> level of file descriptors and syscalls.

That's a fair statement, and I'll try to answer here (and hopefully
later in the other thread as well). The sandbox should be able to
perform operations that are in some sense not security-relevant:
mostly performing computations, reading some necessary files, and
writing some diagnostics to standard output. The initial use case can
be running byte compilation in a Flymake backend. This would allow us
to enable Flymake byte compilation support by default, even on
untrusted code, because due to the sandbox that code could never
perform harmful operations. The Flymake backend would then use the
high-level sandbox functions to asynchronously start byte compilation
in a sandbox. The start-sandbox function in turn would launch an Emacs
subprocess using bwrap or similar to set up appropriate mount
namespaces and apply a Seccomp filter (in the GNU/Linux case).

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 17 Apr 2021 18:20:15 +0200
> Cc: Mattias Engdegård <mattiase@acm.org>, 
> 	João Távora <joaotavora@gmail.com>, 
> 	45198@debbugs.gnu.org, Stefan Kangas <stefankangas@gmail.com>, 
> 	Stefan Monnier <monnier@iro.umontreal.ca>, Alan Third <alan@idiocy.org>
> 
> That's a fair statement, and I'll try to answer here (and hopefully
> later in the other thread as well). The sandbox should be able to
> perform operations that are in some sense not security-relevant:
> mostly performing computations, reading some necessary files, and
> writing some diagnostics to standard output. The initial use case can
> be running byte compilation in a Flymake backend. This would allow us
> to enable Flymake byte compilation support by default, even on
> untrusted code, because due to the sandbox that code could never
> perform harmful operations. The Flymake backend would then use the
> high-level sandbox functions to asynchronously start byte compilation
> in a sandbox. The start-sandbox function in turn would launch an Emacs
> subprocess using bwrap or similar to set up appropriate mount
> namespaces and apply a Seccomp filter (in the GNU/Linux case).

Thanks.  I think I understand the general idea, but not how to
translate that into real life.

"Performing computations" in Emacs corresponds to invoking gobs of
system interfaces, and if we are going to filter most of them, I fear
we will get a dysfunctional Emacs.  E.g., cursor blinking requires
accessing the system time, displaying a busy cursor requires interval
timers, profiling requires signals, and you cannot do anything in
Emacs without being able to allocate memory.  If we leave Emacs only
with capabilities to read and write to a couple of descriptors, how
will the result be useful?  Even if Flymake byte compilation can live
in such a sandbox (and I'm not yet certain it can), is that the most
important situation where untrusted code could be run by Emacs?

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

> It works and can be pushed right away but it would be nice to have a place
> to use it, for validation and for tuning the interface. Any plans for that?

My primary target is `elisp-flymake--batch-compile-for-flymake`.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 17 Apr 2021 12:58:55 -0400
> Cc: João Távora <joaotavora@gmail.com>,
>  Philipp Stephani <p.stephani2@gmail.com>, Stefan Kangas <stefan@marxist.se>,
>  45198@debbugs.gnu.org, Alan Third <alan@idiocy.org>
> 
> My primary target is `elisp-flymake--batch-compile-for-flymake`.

What does that mean in practice? what does that "target" require?

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

17 apr. 2021 kl. 17.44 skrev Philipp <p.stephani2@gmail.com>:

> I think it would be better to first implement the mechanism and not the high-level `sandbox-enter' function

Sorry, there's a misunderstanding here -- it's just a name (and not meant to be a high-level function). I've given it a more platform-specific name. It is not meant to be a general interface to which any thing else has to conform.

Whether it should use --darwin-sandbox instead of --eval "(darwin-sandbox '(\"DIR\"))" is not very important at this point. It's not intended for general use in any case (and the doc strings now make this clear).

In particular, we do not benefit from artificially restricting the macOS sandboxing until we know what is needed. Nothing like a Lisp interface for experimentation!

> As we gain more experience with these sandboxing mechanisms, we can look at relaxing these restrictions, but I think initially we should be conservative.

I take the opposite view, but our goals are the same and we will converge.

> Is there any documentation you could refer to, even only an unofficial one?

Well, I dug up some web links that will be gone tomorrow...

> This needs to somehow document what PROFILE is.

You are right; elaborated.

>> +Already open descriptors can be used freely. */)
> 
> What does this mean?  Emacs doesn't really expose file descriptors to users.

It sort of does (in the form of processes), but there could also be descriptors not directly exposed. It would be incomplete not to mention the possibility. It looks like the seccomp filter generator uses the same policy, treating descriptors as capabilities.

> Missing CHECK_STRING (profile).

Thanks! Fixed.

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

17 apr. 2021 kl. 18.10 skrev Philipp <p.stephani2@gmail.com>:

> (cl-defun start-sandbox (function &key readable-directories stdout-buffer) ...)
> (defun wait-for-sandbox (sandbox) ...)
> 
> where start-sandbox returns an opaque sandbox object running FUNCTION that wait-for-sandbox can wait for.  That should be generic enough that it's extensible and implementable on several platforms, and doesn't lock us into specific implementation choices.

That's probably a nice interface. A slightly more low-level mechanism is what I had in mind, a `make-process` variant that starts an Emacs subprocess with the required arguments to set up a sandbox and leaving it to the user to supply remaining arguments. But maybe we are really talking about more or less the same thing.


17 apr. 2021 kl. 18.58 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> My primary target is `elisp-flymake--batch-compile-for-flymake`.

Very reasonable. Or would you prefer having the sandboxing in flymake?

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

>> My primary target is `elisp-flymake--batch-compile-for-flymake`.
> What does that mean in practice? what does that "target" require?

It needs to take untrusted ELisp code and run it (with no need for user
interaction) in a way that doesn't introduce any security risk.

Currently the code starts a new Emacs process in batch mode and lets it
do whatever it wants, with all the security problems this entails.

Normally, this untrusted ELisp code (the one present within
`eval-when-compile` and macros defined within the file) limits itself to
quite simple sexp manipulation, so the sandboxing can be quite
restrictive, disallowing things like user interaction, uses of
subprocesses, or writing to files.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

>> As we gain more experience with these sandboxing mechanisms, we can look
>> at relaxing these restrictions, but I think initially we should
>> be conservative.
>
> I take the opposite view, but our goals are the same and we will converge.

I guess the conversion goes like:
- define "low-level" interfaces to OS-specific functionality.  They can
  be as close to the OS's own featureset as we want.  They don't
  really need to be stable over time (especially not at the beginning).
- define an OS-agnostic API on top of them.  This one needs to be
  conservative and should evolve more slowly, paying attention to
  backward compatibility.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: mattiase@acm.org,  joaotavora@gmail.com,  p.stephani2@gmail.com,
>   stefan@marxist.se,  45198@debbugs.gnu.org,  alan@idiocy.org
> Date: Sat, 17 Apr 2021 13:53:34 -0400
> 
> >> My primary target is `elisp-flymake--batch-compile-for-flymake`.
> > What does that mean in practice? what does that "target" require?
> 
> It needs to take untrusted ELisp code and run it (with no need for user
> interaction) in a way that doesn't introduce any security risk.

That's too general to allow any meaningful discussion, in particular
whether seccomp could be the basis for satisfying those requirements.

> Currently the code starts a new Emacs process in batch mode and lets it
> do whatever it wants, with all the security problems this entails.
> 
> Normally, this untrusted ELisp code (the one present within
> `eval-when-compile` and macros defined within the file) limits itself to
> quite simple sexp manipulation, so the sandboxing can be quite
> restrictive, disallowing things like user interaction, uses of
> subprocesses, or writing to files.

How is this different from byte-compiling some code, e.g. one
downloaded from some elpa?

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

>> My primary target is `elisp-flymake--batch-compile-for-flymake`.
> Very reasonable. Or would you prefer having the sandboxing in flymake?

AFAICT this question refers to a minor implementation detail ;-)


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

>> Normally, this untrusted ELisp code (the one present within
>> `eval-when-compile` and macros defined within the file) limits itself to
>> quite simple sexp manipulation, so the sandboxing can be quite
>> restrictive, disallowing things like user interaction, uses of
>> subprocesses, or writing to files.
> How is this different from byte-compiling some code, e.g. one
> downloaded from some elpa?

The normal way to enable flymake is something like

    (add-hook 'emacs-lisp-mode #'flymake-mode)

so the file gets compiled just because you're looking at it.
That's quite different from an explicit request from the user to compile
a file.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 331 bytes --]

17 apr. 2021 kl. 20.21 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

>> Very reasonable. Or would you prefer having the sandboxing in flymake?
> 
> AFAICT this question refers to a minor implementation detail ;-)

Of course, sorry.

Looks like I forgot to attach the updated patch in a previous message. Here it is.


[-- Attachment #2: 0001-Add-macOS-sandboxing-bug-45198.patch --]
[-- Type: application/octet-stream, Size: 7400 bytes --]

From 78906da41140dc33119b419a410c4a4f0a3aee80 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sat, 17 Apr 2021 20:53:39 +0200
Subject: [PATCH] Add macOS sandboxing (bug#45198)

This is the corresponding low-level sandboxing facility corresponding
to the recently added Seccomp for Linux.  `darwin-sandbox-init` gives
direct access to the system sandboxing call; `darwin-sandbox-enter` is
a wrapper that takes a list of directories under which files can be
read.  These should be considered internal mechanisms for now.

* lisp/subr.el (darwin-sandbox-enter):
* src/sysdep.c (Fdarwin_sandbox_init): New functions.
* test/src/emacs-tests.el (emacs-tests/darwin-sandbox): New test.
---
 lisp/subr.el            | 18 +++++++++++
 src/sysdep.c            | 32 +++++++++++++++++++
 test/src/emacs-tests.el | 70 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 120 insertions(+)

diff --git a/lisp/subr.el b/lisp/subr.el
index c2be26a15f..196512c0c6 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -6262,4 +6262,22 @@ internal--format-docstring-line
 This is intended for internal use only."
   (internal--fill-string-single-line (apply #'format string objects)))
 
+(when (eq system-type 'darwin)
+  (defun darwin-sandbox-enter (dirs)
+    "Enter a sandbox only permitting reading files under DIRS.
+DIRS is a list of directory names.  Most other operations such as
+writing files and network access are disallowed.
+Existing open descriptors can still be used freely.
+
+This is not a supported interface and is for internal use only."
+    (darwin-sandbox-init
+     (concat "(version 1)\n"
+             "(deny default)\n"
+             ;; Emacs seems to need /dev/null; allowing it does no harm.
+             "(allow file-read* (path \"/dev/null\"))\n"
+             (mapconcat (lambda (dir)
+                          (format "(allow file-read* (subpath %S))\n" dir))
+                        dirs ""))))
+  )
+
 ;;; subr.el ends here
diff --git a/src/sysdep.c b/src/sysdep.c
index d940acc4e0..44e8b82bc6 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -4286,8 +4286,40 @@ str_collate (Lisp_Object s1, Lisp_Object s2,
 }
 #endif	/* WINDOWSNT */
 
+#ifdef DARWIN_OS
+
+/* This function prototype is not in the platform header files.
+   See https://reverse.put.as/wp-content/uploads/2011/09/Apple-Sandbox-Guide-v1.0.pdf
+   and https://chromium.googlesource.com/chromium/src/+/master/sandbox/mac/seatbelt_sandbox_design.md */
+int sandbox_init_with_parameters(const char *profile,
+                                 uint64_t flags,
+                                 const char *const parameters[],
+                                 char **errorbuf);
+
+DEFUN ("darwin-sandbox-init", Fdarwin_sandbox_init, Sdarwin_sandbox_init,
+       1, 1, 0,
+       doc: /* Enter a sandbox whose permitted access is curtailed by PROFILE.
+Already open descriptors can be used freely.
+PROFILE is a string in the macOS sandbox profile language,
+a set of rules in a Lisp-like syntax.
+
+This is not a supported interface and is for internal use only. */)
+  (Lisp_Object profile)
+{
+  CHECK_STRING (profile);
+  char *err = NULL;
+  if (sandbox_init_with_parameters (SSDATA (profile), 0, NULL, &err) != 0)
+    error ("sandbox error: %s", err);
+  return Qnil;
+}
+
+#endif	/* DARWIN_OS */
+
 void
 syms_of_sysdep (void)
 {
   defsubr (&Sget_internal_run_time);
+#ifdef DARWIN_OS
+  defsubr (&Sdarwin_sandbox_init);
+#endif
 }
diff --git a/test/src/emacs-tests.el b/test/src/emacs-tests.el
index 09f9a248ef..c1a741c359 100644
--- a/test/src/emacs-tests.el
+++ b/test/src/emacs-tests.el
@@ -210,4 +210,74 @@ emacs-tests/bwrap/allows-stdout
           (should (eql status 0)))
         (should (equal (string-trim (buffer-string)) "Hi"))))))
 
+(defun emacs-tests--darwin-run-sandboxed-emacs (sandbox-dirs body)
+  "Run Emacs and evaluate BODY, only allowing reads from SANDBOX-DIRS.
+If SANDBOX-DIRS is `no-sandbox', then run without sandbox.
+Return (EXIT-STATUS . OUTPUT), where OUTPUT is stderr and stdout."
+  (let ((emacs (expand-file-name invocation-name invocation-directory))
+        (process-environment nil))
+    (with-temp-buffer
+      (let* ((prog `(progn
+                      ,@(and (not (eq sandbox-dirs 'no-sandbox))
+                             `((darwin-sandbox-enter ',sandbox-dirs)))
+                      ,@body))
+             (res (call-process emacs nil t nil
+                                "--quick" "--batch"
+                                (format "--eval=%S" prog))))
+        (cons res (buffer-string))))))
+
+(ert-deftest emacs-tests/darwin-sandbox ()
+  (skip-unless (eq system-type 'darwin))
+  (emacs-tests--with-temp-file test-file ("test")
+    (let ((some-text "abcdef")
+          (other-text "ghijkl")
+          (test-file (file-truename test-file)))   ; resolve symlinks
+      (with-temp-buffer
+        (insert some-text)
+        (write-file test-file))
+
+      ;; Read the file without allowing its directory -- should fail.
+      (let ((res-out (emacs-tests--darwin-run-sandboxed-emacs
+                      nil
+                      `((find-file-literally ,test-file)
+                        (message "OK: %s" (buffer-string))))))
+        (ert-info ((cdr res-out) :prefix "output: ")
+          (should-not (equal (car res-out) 0))
+          (should-not (string-search some-text (cdr res-out)))))
+
+      ;; Read the file allowing its directory -- should succeed.
+      (let ((res-out (emacs-tests--darwin-run-sandboxed-emacs
+                      (list (file-name-directory test-file))
+                      `((find-file-literally ,test-file)
+                        (message "OK: %s" (buffer-string))))))
+        (should (equal res-out (cons 0 (format "OK: %s\n" some-text)))))
+
+      ;; Write to the file allowing directory reads -- should fail.
+      (let ((res-out (emacs-tests--darwin-run-sandboxed-emacs
+                      (list (file-name-directory test-file))
+                      `((with-temp-buffer
+                          (insert ,other-text)
+                          (write-file ,test-file))))))
+        (ert-info ((cdr res-out) :prefix "output: ")
+          (should-not (equal (car res-out) 0))
+          ;; The file should be unchanged.
+          (let ((contents (with-temp-buffer
+                            (insert-file-contents-literally test-file)
+                            (buffer-string))))
+            (should (equal contents some-text)))))
+
+      ;; Write to the file without sandbox -- should succeed.
+      (let ((res-out (emacs-tests--darwin-run-sandboxed-emacs
+                      'no-sandbox
+                      `((with-temp-buffer
+                          (insert ,other-text)
+                          (write-file ,test-file))))))
+        (ert-info ((cdr res-out) :prefix "output: ")
+          (should (equal (car res-out) 0))
+          ;; The file should be changed.
+          (let ((contents (with-temp-buffer
+                            (insert-file-contents-literally test-file)
+                            (buffer-string))))
+            (should (equal contents other-text))))))))
+
 ;;; emacs-tests.el ends here
-- 
2.21.1 (Apple Git-122.3)

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Sa., 17. Apr. 2021 um 18:33 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sat, 17 Apr 2021 18:20:15 +0200
> > Cc: Mattias Engdegård <mattiase@acm.org>,
> >       João Távora <joaotavora@gmail.com>,
> >       45198@debbugs.gnu.org, Stefan Kangas <stefankangas@gmail.com>,
> >       Stefan Monnier <monnier@iro.umontreal.ca>, Alan Third <alan@idiocy.org>
> >
> > That's a fair statement, and I'll try to answer here (and hopefully
> > later in the other thread as well). The sandbox should be able to
> > perform operations that are in some sense not security-relevant:
> > mostly performing computations, reading some necessary files, and
> > writing some diagnostics to standard output. The initial use case can
> > be running byte compilation in a Flymake backend. This would allow us
> > to enable Flymake byte compilation support by default, even on
> > untrusted code, because due to the sandbox that code could never
> > perform harmful operations. The Flymake backend would then use the
> > high-level sandbox functions to asynchronously start byte compilation
> > in a sandbox. The start-sandbox function in turn would launch an Emacs
> > subprocess using bwrap or similar to set up appropriate mount
> > namespaces and apply a Seccomp filter (in the GNU/Linux case).
>
> Thanks.  I think I understand the general idea, but not how to
> translate that into real life.
>
> "Performing computations" in Emacs corresponds to invoking gobs of
> system interfaces, and if we are going to filter most of them, I fear
> we will get a dysfunctional Emacs.  E.g., cursor blinking requires
> accessing the system time, displaying a busy cursor requires interval
> timers, profiling requires signals, and you cannot do anything in
> Emacs without being able to allocate memory.  If we leave Emacs only
> with capabilities to read and write to a couple of descriptors, how
> will the result be useful?

We would definitely allow more stuff (e.g. some other syscalls are
required for Emacs to even start up). For example, Emacs needs to
allocate memory and thus needs mmap/sbrk. Timing functions are not
security-sensitive (timing attacks exist, but should be prevented in
this case by blocking any relevant use of the data such obtained), and
signals only affect the sandboxed Emacs process. The two big things we
need to prevent is writing arbitrary files and creating sockets.
At least initially we should only care about batch mode, though -
nothing prevents interactive mode in a sandbox in principle, but batch
mode is much easier to deal with, and suffices for the Flymake use
case.

>  Even if Flymake byte compilation can live
> in such a sandbox (and I'm not yet certain it can), is that the most
> important situation where untrusted code could be run by Emacs?

It's at least the situation described here, and I think it's pretty
important. Another potential use case would be to allow some
buffer-local evaluation.

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: mattiase@acm.org,  joaotavora@gmail.com,  p.stephani2@gmail.com,
>   stefan@marxist.se,  45198@debbugs.gnu.org,  alan@idiocy.org
> Date: Sat, 17 Apr 2021 14:47:34 -0400
> 
> >> Normally, this untrusted ELisp code (the one present within
> >> `eval-when-compile` and macros defined within the file) limits itself to
> >> quite simple sexp manipulation, so the sandboxing can be quite
> >> restrictive, disallowing things like user interaction, uses of
> >> subprocesses, or writing to files.
> > How is this different from byte-compiling some code, e.g. one
> > downloaded from some elpa?
> 
> The normal way to enable flymake is something like
> 
>     (add-hook 'emacs-lisp-mode #'flymake-mode)
> 
> so the file gets compiled just because you're looking at it.
> That's quite different from an explicit request from the user to compile
> a file.

It is?  Sorry, I don't see the difference, not a significant one.  If
you are implying that one does something conscious and deliberate
before byte-compiling a file, then one could also remove Flymake from
the hook while at that.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Sa., 17. Apr. 2021 um 19:22 Uhr schrieb Mattias Engdegård <mattiase@acm.org>:
> > As we gain more experience with these sandboxing mechanisms, we can look at relaxing these restrictions, but I think initially we should be conservative.
>
> I take the opposite view, but our goals are the same and we will converge.

As long as they converge before releasing Emacs 28, fine. After that
it will be very difficult to restrict an initially-open interface.

> >> +Already open descriptors can be used freely. */)
> >
> > What does this mean?  Emacs doesn't really expose file descriptors to users.
>
> It sort of does (in the form of processes), but there could also be descriptors not directly exposed. It would be incomplete not to mention the possibility. It looks like the seccomp filter generator uses the same policy, treating descriptors as capabilities.

Yes, but since it's only a command-line flag right now, there
shouldn't be any open file descriptors except the standard ones, so
this specific bit of complexity is avoided.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Sa., 17. Apr. 2021 um 19:48 Uhr schrieb Mattias Engdegård <mattiase@acm.org>:
>
> 17 apr. 2021 kl. 18.10 skrev Philipp <p.stephani2@gmail.com>:
>
> > (cl-defun start-sandbox (function &key readable-directories stdout-buffer) ...)
> > (defun wait-for-sandbox (sandbox) ...)
> >
> > where start-sandbox returns an opaque sandbox object running FUNCTION that wait-for-sandbox can wait for.  That should be generic enough that it's extensible and implementable on several platforms, and doesn't lock us into specific implementation choices.
>
> That's probably a nice interface. A slightly more low-level mechanism is what I had in mind, a `make-process` variant that starts an Emacs subprocess with the required arguments to set up a sandbox and leaving it to the user to supply remaining arguments. But maybe we are really talking about more or less the same thing.

Yes, that would essentially be how start-sandbox would get
implemented. In the Seccomp case, something like (conceptually)
(start-process "bwrap ... -- emacs --seccomp=... --quick --batch
--eval=FUNCTION")
where bwrap can set up mount namespaces to restrict the filesystem.

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Sa., 17. Apr. 2021 um 19:57 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> >> As we gain more experience with these sandboxing mechanisms, we can look
> >> at relaxing these restrictions, but I think initially we should
> >> be conservative.
> >
> > I take the opposite view, but our goals are the same and we will converge.
>
> I guess the conversion goes like:
> - define "low-level" interfaces to OS-specific functionality.  They can
>   be as close to the OS's own featureset as we want.  They don't
>   really need to be stable over time (especially not at the beginning).

I guess it would still make sense to stabilize them with Emacs 28.
These interfaces can be useful in their own right, and I guess people
will start using them once available.

> - define an OS-agnostic API on top of them.  This one needs to be
>   conservative and should evolve more slowly, paying attention to
>   backward compatibility.
>
>

Yes.

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 17 Apr 2021 21:14:02 +0200
> Cc: Mattias Engdegård <mattiase@acm.org>, 
> 	João Távora <joaotavora@gmail.com>, 
> 	45198@debbugs.gnu.org, Stefan Kangas <stefankangas@gmail.com>, 
> 	Stefan Monnier <monnier@iro.umontreal.ca>, Alan Third <alan@idiocy.org>
> 
> > "Performing computations" in Emacs corresponds to invoking gobs of
> > system interfaces, and if we are going to filter most of them, I fear
> > we will get a dysfunctional Emacs.  E.g., cursor blinking requires
> > accessing the system time, displaying a busy cursor requires interval
> > timers, profiling requires signals, and you cannot do anything in
> > Emacs without being able to allocate memory.  If we leave Emacs only
> > with capabilities to read and write to a couple of descriptors, how
> > will the result be useful?
> 
> We would definitely allow more stuff (e.g. some other syscalls are
> required for Emacs to even start up). For example, Emacs needs to
> allocate memory and thus needs mmap/sbrk. Timing functions are not
> security-sensitive (timing attacks exist, but should be prevented in
> this case by blocking any relevant use of the data such obtained), and
> signals only affect the sandboxed Emacs process. The two big things we
> need to prevent is writing arbitrary files and creating sockets.

So you are going to suggest that we rely on some auditing of the
syscalls Emacs uses now to decide which ones to filter and which not?
If so, how will this work in the future, when Emacs might decide to
issue some additional syscalls? who and how will remember to update
the filter definitions?  And what about users who make local changes
in their Emacs?

> At least initially we should only care about batch mode, though -
> nothing prevents interactive mode in a sandbox in principle, but batch
> mode is much easier to deal with, and suffices for the Flymake use
> case.

I understand why batch mode might be easier to deal with, but I'm not
sure we should care more about it just because it's easier.

bug#45198: 28.0.50; Sandbox mode

[Philipp]

> Am 17.04.2021 um 20:59 schrieb Mattias Engdegård <mattiase@acm.org>:
> 
> 17 apr. 2021 kl. 20.21 skrev Stefan Monnier <monnier@iro.umontreal.ca>:
> 
>>> Very reasonable. Or would you prefer having the sandboxing in flymake?
>> 
>> AFAICT this question refers to a minor implementation detail ;-)
> 
> Of course, sorry.
> 
> Looks like I forgot to attach the updated patch in a previous message. Here it is.
> 
> <0001-Add-macOS-sandboxing-bug-45198.patch>

Looks generally fine, just a few minor comments.


> diff --git a/lisp/subr.el b/lisp/subr.el
> index c2be26a15f..196512c0c6 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el

Should this maybe be in a platform-specific file such as ns-fns.el (like dos-fns.el, w32-fns.el)?

> @@ -6262,4 +6262,22 @@ internal--format-docstring-line
>  This is intended for internal use only."
>    (internal--fill-string-single-line (apply #'format string objects)))
>  
> +(when (eq system-type 'darwin)
> +  (defun darwin-sandbox-enter (dirs)

I don't think we use the "darwin-" prefix anywhere else in Emacs.  Maybe use a "ns-" prefix.
Also I think such internal functions commonly have an "internal" piece somewhere in their name, e.g. "ns-enter-sandbox-internal".

> +    "Enter a sandbox only permitting reading files under DIRS.
> +DIRS is a list of directory names.  Most other operations such as
> +writing files and network access are disallowed.
> +Existing open descriptors can still be used freely.
> +
> +This is not a supported interface and is for internal use only."
> +    (darwin-sandbox-init

Can you also refer to the documents listed below, so that readers aren't wondering what this syntax means?

> +     (concat "(version 1)\n"

Since this uses Lisp syntax, maybe use (prin1-to-string `(...))?

> +             "(deny default)\n"
> +             ;; Emacs seems to need /dev/null; allowing it does no harm.
> +             "(allow file-read* (path \"/dev/null\"))\n"
> +             (mapconcat (lambda (dir)
> +                          (format "(allow file-read* (subpath %S))\n" dir))

Are you sure that the string quoting syntaxes are compatible?  We really need to avoid injection attacks here.

> +                        dirs ""))))
> +  )
> +
>  ;;; subr.el ends here
> diff --git a/src/sysdep.c b/src/sysdep.c
> index d940acc4e0..44e8b82bc6 100644
> --- a/src/sysdep.c
> +++ b/src/sysdep.c
> @@ -4286,8 +4286,40 @@ str_collate (Lisp_Object s1, Lisp_Object s2,
>  }
>  #endif	/* WINDOWSNT */
>  
> +#ifdef DARWIN_OS
> +
> +/* This function prototype is not in the platform header files.
> +   See https://reverse.put.as/wp-content/uploads/2011/09/Apple-Sandbox-Guide-v1.0.pdf
> +   and https://chromium.googlesource.com/chromium/src/+/master/sandbox/mac/seatbelt_sandbox_design.md */

Thanks, I'd expect at least the Chromium reference to stick around.

> +int sandbox_init_with_parameters(const char *profile,
> +                                 uint64_t flags,
> +                                 const char *const parameters[],
> +                                 char **errorbuf);
> +
> +DEFUN ("darwin-sandbox-init", Fdarwin_sandbox_init, Sdarwin_sandbox_init,

Similar comments about naming here; maybe call this ns-init-sandbox-internal or so.

> +       1, 1, 0,
> +       doc: /* Enter a sandbox whose permitted access is curtailed by PROFILE.
> +Already open descriptors can be used freely.
> +PROFILE is a string in the macOS sandbox profile language,
> +a set of rules in a Lisp-like syntax.

I'd also refer to the Chromium document here, otherwise C-h f for this function won't be very useful.

> +
> +This is not a supported interface and is for internal use only. */)
> +  (Lisp_Object profile)
> +{
> +  CHECK_STRING (profile);
> +  char *err = NULL;
> +  if (sandbox_init_with_parameters (SSDATA (profile), 0, NULL, &err) != 0)

If you're using SSDATA, better check that the string doesn't contain embedded null bytes.
Also does this need to be encoded somehow?  What happens if the string contains non-Unicode characters like raw bytes?

> +    error ("sandbox error: %s", err);

This looks like an error that clients could handle, so I'd signal a specific error symbol here.

> +  return Qnil;
> +}
> +
> +#endif	/* DARWIN_OS */
> +
>  void
>  syms_of_sysdep (void)
>  {
>    defsubr (&Sget_internal_run_time);
> +#ifdef DARWIN_OS
> +  defsubr (&Sdarwin_sandbox_init);
> +#endif
>  }
> diff --git a/test/src/emacs-tests.el b/test/src/emacs-tests.el

This should probably be in subr-tests.el since it tests a function in subr.el.

> index 09f9a248ef..c1a741c359 100644
> --- a/test/src/emacs-tests.el
> +++ b/test/src/emacs-tests.el
> @@ -210,4 +210,74 @@ emacs-tests/bwrap/allows-stdout
>            (should (eql status 0)))
>          (should (equal (string-trim (buffer-string)) "Hi"))))))
>  
> +(defun emacs-tests--darwin-run-sandboxed-emacs (sandbox-dirs body)
> +  "Run Emacs and evaluate BODY, only allowing reads from SANDBOX-DIRS.
> +If SANDBOX-DIRS is `no-sandbox', then run without sandbox.
> +Return (EXIT-STATUS . OUTPUT), where OUTPUT is stderr and stdout."
> +  (let ((emacs (expand-file-name invocation-name invocation-directory))
> +        (process-environment nil))
> +    (with-temp-buffer
> +      (let* ((prog `(progn
> +                      ,@(and (not (eq sandbox-dirs 'no-sandbox))
> +                             `((darwin-sandbox-enter ',sandbox-dirs)))
> +                      ,@body))
> +             (res (call-process emacs nil t nil
> +                                "--quick" "--batch"
> +                                (format "--eval=%S" prog))))
> +        (cons res (buffer-string))))))

This is more of a personal style, feel free to ignore:
I like tests that are somewhat repetitive but more decoupled and easier to read better than tests with factored-out assertions.  See e.g. the arguments in https://www.maaikebrinkhof.nl/2016/03/repetition-in-testing/ and https://stackoverflow.com/a/130038.

> +
> +(ert-deftest emacs-tests/darwin-sandbox ()
> +  (skip-unless (eq system-type 'darwin))
> +  (emacs-tests--with-temp-file test-file ("test")
> +    (let ((some-text "abcdef")
> +          (other-text "ghijkl")
> +          (test-file (file-truename test-file)))   ; resolve symlinks
> +      (with-temp-buffer
> +        (insert some-text)
> +        (write-file test-file))
> +
> +      ;; Read the file without allowing its directory -- should fail.
> +      (let ((res-out (emacs-tests--darwin-run-sandboxed-emacs
> +                      nil
> +                      `((find-file-literally ,test-file)
> +                        (message "OK: %s" (buffer-string))))))
> +        (ert-info ((cdr res-out) :prefix "output: ")
> +          (should-not (equal (car res-out) 0))
> +          (should-not (string-search some-text (cdr res-out)))))
> +
> +      ;; Read the file allowing its directory -- should succeed.
> +      (let ((res-out (emacs-tests--darwin-run-sandboxed-emacs
> +                      (list (file-name-directory test-file))
> +                      `((find-file-literally ,test-file)
> +                        (message "OK: %s" (buffer-string))))))
> +        (should (equal res-out (cons 0 (format "OK: %s\n" some-text)))))
> +
> +      ;; Write to the file allowing directory reads -- should fail.
> +      (let ((res-out (emacs-tests--darwin-run-sandboxed-emacs
> +                      (list (file-name-directory test-file))
> +                      `((with-temp-buffer
> +                          (insert ,other-text)
> +                          (write-file ,test-file))))))
> +        (ert-info ((cdr res-out) :prefix "output: ")
> +          (should-not (equal (car res-out) 0))
> +          ;; The file should be unchanged.
> +          (let ((contents (with-temp-buffer
> +                            (insert-file-contents-literally test-file)
> +                            (buffer-string))))
> +            (should (equal contents some-text)))))
> +
> +      ;; Write to the file without sandbox -- should succeed.
> +      (let ((res-out (emacs-tests--darwin-run-sandboxed-emacs
> +                      'no-sandbox
> +                      `((with-temp-buffer
> +                          (insert ,other-text)
> +                          (write-file ,test-file))))))
> +        (ert-info ((cdr res-out) :prefix "output: ")
> +          (should (equal (car res-out) 0))
> +          ;; The file should be changed.
> +          (let ((contents (with-temp-buffer
> +                            (insert-file-contents-literally test-file)
> +                            (buffer-string))))
> +            (should (equal contents other-text))))))))

These should be four separate tests, as they test four separate things.

bug#45198: 28.0.50; Sandbox mode

[Philipp]

> Am 17.04.2021 um 21:23 schrieb Eli Zaretskii <eliz@gnu.org>:
> 
>> From: Philipp Stephani <p.stephani2@gmail.com>
>> Date: Sat, 17 Apr 2021 21:14:02 +0200
>> Cc: Mattias Engdegård <mattiase@acm.org>, 
>> 	João Távora <joaotavora@gmail.com>, 
>> 	45198@debbugs.gnu.org, Stefan Kangas <stefankangas@gmail.com>, 
>> 	Stefan Monnier <monnier@iro.umontreal.ca>, Alan Third <alan@idiocy.org>
>> 
>>> "Performing computations" in Emacs corresponds to invoking gobs of
>>> system interfaces, and if we are going to filter most of them, I fear
>>> we will get a dysfunctional Emacs.  E.g., cursor blinking requires
>>> accessing the system time, displaying a busy cursor requires interval
>>> timers, profiling requires signals, and you cannot do anything in
>>> Emacs without being able to allocate memory.  If we leave Emacs only
>>> with capabilities to read and write to a couple of descriptors, how
>>> will the result be useful?
>> 
>> We would definitely allow more stuff (e.g. some other syscalls are
>> required for Emacs to even start up). For example, Emacs needs to
>> allocate memory and thus needs mmap/sbrk. Timing functions are not
>> security-sensitive (timing attacks exist, but should be prevented in
>> this case by blocking any relevant use of the data such obtained), and
>> signals only affect the sandboxed Emacs process. The two big things we
>> need to prevent is writing arbitrary files and creating sockets.
> 
> So you are going to suggest that we rely on some auditing of the
> syscalls Emacs uses now to decide which ones to filter and which not?

I don't mean that we should wade through all potential syscalls that Emacs could make.  Typically you can come up with such a Seccomp policy iteratively: run Seccomp in advisory mode (i.e. only log syscalls), then allow the syscalls that are both necessary and harmless in the policy.

> If so, how will this work in the future, when Emacs might decide to
> issue some additional syscalls? who and how will remember to update
> the filter definitions?

There are unit tests that ensure that the behavior we expect works.  For example, an existing unit test verifies that the sandboxed Emacs process can write to standard output (and it has already failed a few times on various systems, which is expected and is how we can find new syscalls to add).  So we only need to remember to run the unit tests (and have good test coverage).

>  And what about users who make local changes
> in their Emacs?

They can provide their own Seccomp policies or modify the ones included in Emacs.

> 
>> At least initially we should only care about batch mode, though -
>> nothing prevents interactive mode in a sandbox in principle, but batch
>> mode is much easier to deal with, and suffices for the Flymake use
>> case.
> 
> I understand why batch mode might be easier to deal with, but I'm not
> sure we should care more about it just because it's easier.

We care about it in the scope of the feature being discussed (Flymake) because Flymake runs Emacs in batch mode anyway.

bug#45198: 28.0.50; Sandbox mode

[Alan Third]

On Sat, Apr 17, 2021 at 09:42:31PM +0200, Philipp wrote:
> > diff --git a/lisp/subr.el b/lisp/subr.el
> > index c2be26a15f..196512c0c6 100644
> > --- a/lisp/subr.el
> > +++ b/lisp/subr.el
> 
> Should this maybe be in a platform-specific file such as ns-fns.el (like dos-fns.el, w32-fns.el)?

> > @@ -6262,4 +6262,22 @@ internal--format-docstring-line
> >  This is intended for internal use only."
> >    (internal--fill-string-single-line (apply #'format string objects)))
> >  
> > +(when (eq system-type 'darwin)
> > +  (defun darwin-sandbox-enter (dirs)
> 
> I don't think we use the "darwin-" prefix anywhere else in Emacs.
> Maybe use a "ns-" prefix. Also I think such internal functions
> commonly have an "internal" piece somewhere in their name, e.g.
> "ns-enter-sandbox-internal".

I'm coming to this late, so I may be misunderstanding what this is,
but as far as I can tell it has nothing to do with Cocoa or GNUstep,
so I would suggest not using the ns- prefix.

If you're running, say, terminal Emacs on a Darwin system, ns-fns.el
is not called.

NS =/= Darwin.

-- 
Alan Third

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

>> The normal way to enable flymake is something like
>>
>>     (add-hook 'emacs-lisp-mode #'flymake-mode)
>>
>> so the file gets compiled just because you're looking at it.
>> That's quite different from an explicit request from the user to compile
>> a file.
>
> It is?  Sorry, I don't see the difference, not a significant one.

It make `C-x C-f` a tool to run arbitrary code (since the file may end
with something apparently harmless like `.txt` but may actually use
`emacs-lisp-mode`).

> If you are implying that one does something conscious and deliberate
> before byte-compiling a file,

Have you ever byte-compiled a random ELisp file sent to you from some
unknown email address without looking at it first?

Have you ever viewed with Emacs a file sent from some unknown
email address?

For me the answers are "no, never" and "yes, many times".
Enabling flymake mode as above currently blurs the difference between
those two cases in terms of risks.

> then one could also remove Flymake from the hook while at that.

The whole point of the sandboxing exercise is so as to be able to have
flymake-mode in the hook without exposing yourself to
these vulnerabilities.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Philipp <p.stephani2@gmail.com>
> Date: Sat, 17 Apr 2021 21:52:59 +0200
> Cc: Mattias Engdegård <mattiase@acm.org>,
>  João Távora <joaotavora@gmail.com>,
>  45198@debbugs.gnu.org,
>  stefankangas@gmail.com,
>  monnier@iro.umontreal.ca,
>  alan@idiocy.org
> 
> > So you are going to suggest that we rely on some auditing of the
> > syscalls Emacs uses now to decide which ones to filter and which not?
> 
> I don't mean that we should wade through all potential syscalls that Emacs could make.  Typically you can come up with such a Seccomp policy iteratively: run Seccomp in advisory mode (i.e. only log syscalls), then allow the syscalls that are both necessary and harmless in the policy.

Emacs can be invoked to do many different things, and will
correspondingly present very different profiles of syscalls.  Is the
procedure you envision practical, let alone seamless, given that it
will have to become part of the maintenance and the release process?

> > If so, how will this work in the future, when Emacs might decide to
> > issue some additional syscalls? who and how will remember to update
> > the filter definitions?
> 
> There are unit tests that ensure that the behavior we expect works.

Unit tests are a far cry from what Emacs does in real-life usage, so I
very much doubt that unit tests will suffice in this case.

> >  And what about users who make local changes
> > in their Emacs?
> 
> They can provide their own Seccomp policies or modify the ones included in Emacs.

What does providing a policy entail? can you describe the procedure of
tailoring a policy to changes in the Emacs code?

> >> At least initially we should only care about batch mode, though -
> >> nothing prevents interactive mode in a sandbox in principle, but batch
> >> mode is much easier to deal with, and suffices for the Flymake use
> >> case.
> > 
> > I understand why batch mode might be easier to deal with, but I'm not
> > sure we should care more about it just because it's easier.
> 
> We care about it in the scope of the feature being discussed (Flymake) because Flymake runs Emacs in batch mode anyway.

So we will make running Flymake safe, but what about all the other use
cases where similar dangers exist?  Flymake is just a tiny fraction of
what Emacs does, so it sounds strange, to say the least, to work on
solution for that use case alone, when it is clear from the get-go
that many other use cases aren't covered.

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: mattiase@acm.org,  joaotavora@gmail.com,  p.stephani2@gmail.com,
>   stefan@marxist.se,  45198@debbugs.gnu.org,  alan@idiocy.org
> Date: Sat, 17 Apr 2021 16:26:25 -0400
> 
> > If you are implying that one does something conscious and deliberate
> > before byte-compiling a file,
> 
> Have you ever byte-compiled a random ELisp file sent to you from some
> unknown email address without looking at it first?

No, but I also don't use Flymake and never install packages via
package.el.

IOW, my own workflows are not very relevant to the issues at hand, and
neither are yours.  What matters is what others do.

> The whole point of the sandboxing exercise is so as to be able to have
> flymake-mode in the hook without exposing yourself to
> these vulnerabilities.

So we are going to introduce all this non-trivial machinery into Emacs
just to solve the Flymake use case?  Is that reasonable from the
project management POV, in your eyes?

bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am So., 18. Apr. 2021 um 08:21 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp <p.stephani2@gmail.com>
> > Date: Sat, 17 Apr 2021 21:52:59 +0200
> > Cc: Mattias Engdegård <mattiase@acm.org>,
> >  João Távora <joaotavora@gmail.com>,
> >  45198@debbugs.gnu.org,
> >  stefankangas@gmail.com,
> >  monnier@iro.umontreal.ca,
> >  alan@idiocy.org
> >
> > > So you are going to suggest that we rely on some auditing of the
> > > syscalls Emacs uses now to decide which ones to filter and which not?
> >
> > I don't mean that we should wade through all potential syscalls that Emacs could make.  Typically you can come up with such a Seccomp policy iteratively: run Seccomp in advisory mode (i.e. only log syscalls), then allow the syscalls that are both necessary and harmless in the policy.
>
> Emacs can be invoked to do many different things, and will
> correspondingly present very different profiles of syscalls.  Is the
> procedure you envision practical, let alone seamless, given that it
> will have to become part of the maintenance and the release process?

Yes. There aren't that many syscalls to begin with, and Emacs uses
only a small subset of them. New Emacs or libc versions occasionally
introduce new syscalls, but finding and allowing them tends to be not
that big of a deal.

> > >  And what about users who make local changes
> > > in their Emacs?
> >
> > They can provide their own Seccomp policies or modify the ones included in Emacs.
>
> What does providing a policy entail? can you describe the procedure of
> tailoring a policy to changes in the Emacs code?

1. Run the Emacs sandbox with the code you want to run.
2. Emacs will crash with SIGSYS if it hits a forbidden/unknown
syscall. Ensure that this generates a coredump.
3. Check the backtrace for the coredump (e.g. coredumpctl debug)
and/or the Seccomp audit logs (ausearch) for the syscall that
triggered the signal.
4. Add a rule for the syscall and its arguments to the BPF generation
program, e.g. lib-src/seccom-filter.c.
5. Regenerate the BPF rule file.

bug#45198: 28.0.50; Sandbox mode

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sun, 18 Apr 2021 11:11:28 +0200
> Cc: Mattias Engdegård <mattiase@acm.org>, 
> 	João Távora <joaotavora@gmail.com>, 
> 	45198@debbugs.gnu.org, Stefan Kangas <stefankangas@gmail.com>, 
> 	Stefan Monnier <monnier@iro.umontreal.ca>, Alan Third <alan@idiocy.org>
> 
> > > >  And what about users who make local changes
> > > > in their Emacs?
> > >
> > > They can provide their own Seccomp policies or modify the ones included in Emacs.
> >
> > What does providing a policy entail? can you describe the procedure of
> > tailoring a policy to changes in the Emacs code?
> 
> 1. Run the Emacs sandbox with the code you want to run.
> 2. Emacs will crash with SIGSYS if it hits a forbidden/unknown
> syscall. Ensure that this generates a coredump.
> 3. Check the backtrace for the coredump (e.g. coredumpctl debug)
> and/or the Seccomp audit logs (ausearch) for the syscall that
> triggered the signal.
> 4. Add a rule for the syscall and its arguments to the BPF generation
> program, e.g. lib-src/seccom-filter.c.
> 5. Regenerate the BPF rule file.

Sounds complicated, and requires non-trivial low-level knowledge and
tools.

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier]

>> The whole point of the sandboxing exercise is so as to be able to have
>> flymake-mode in the hook without exposing yourself to
>> these vulnerabilities.
>
> So we are going to introduce all this non-trivial machinery into Emacs
> just to solve the Flymake use case?  Is that reasonable from the
> project management POV, in your eyes?

To the extent that this machinery will only be used by those rare places
that need it (e.g. flymake), yes, as long as the low-level interface
(e.g. the code that added the support for the `--seccomp` argument) is
simple enough.

BTW, in the context of GNU/Linux, an alternative to `--seccomp` is to
require the `bwrap` tool (that's what I use in the `elpa-admin.el`
scripts to run makefile rules from ELPA packages).


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 3936 bytes --]


17 apr. 2021 kl. 21.42 skrev Philipp <p.stephani2@gmail.com>:

>> --- a/lisp/subr.el
> 
> Should this maybe be in a platform-specific file such as ns-fns.el (like dos-fns.el, w32-fns.el)?

Yes, and thank you for noticing. Now moved.

> I don't think we use the "darwin-" prefix anywhere else in Emacs.  Maybe use a "ns-" prefix.

As Alan correctly noted this has nothing with NS to do per se, so I'm staying with darwin- for now.

> Also I think such internal functions commonly have an "internal" piece somewhere in their name, e.g. "ns-enter-sandbox-internal".

Maybe, but there is already a platform-specific prefix and a clear note in the doc string that it's internal. Doesn't that suffice?

>> +    (darwin-sandbox-init
> 
> Can you also refer to the documents listed below, so that readers aren't wondering what this syntax means?

Thank you but that sounds unlikely since the PROFILE argument is described as SBPL in the function doc string.

>> +     (concat "(version 1)\n"
> 
> Since this uses Lisp syntax, maybe use (prin1-to-string `(...))?

I'd rather not, since it's not exactly Emacs Lisp syntax. I'd like to avoiding conflating the two as far as possible. However...

>> +                          (format "(allow file-read* (subpath %S))\n" dir))
> 
> Are you sure that the string quoting syntaxes are compatible?

It had me concerned when I wrote it too but it didn't seem possible to cause a false positive that way -- false negatives (access denial) at worst. In particular, names containing backslashes or double quotes work correctly.

>> +       doc: /* Enter a sandbox whose permitted access is curtailed by PROFILE.
>> +Already open descriptors can be used freely.
>> +PROFILE is a string in the macOS sandbox profile language,
>> +a set of rules in a Lisp-like syntax.
> 
> I'd also refer to the Chromium document here, otherwise C-h f for this function won't be very useful.

I wouldn't worry -- an Emacs developer who doesn't already know it is more likely to duckduckgo "macos sandbox profile language" and get an up-to-date reference.

>> +  if (sandbox_init_with_parameters (SSDATA (profile), 0, NULL, &err) != 0)
> 
> If you're using SSDATA, better check that the string doesn't contain embedded null bytes.

There is really no way that that could ever be a problem but I added a check just in case.

> Also does this need to be encoded somehow?  What happens if the string contains non-Unicode characters like raw bytes?

Then there will be false negatives (permissions not granted) or syntax errors. This is just a system call wrapper; the caller is responsible for using reasonable arguments.

>> +    error ("sandbox error: %s", err);
> 
> This looks like an error that clients could handle, so I'd signal a specific error symbol here.

That error just indicates a programming mistake. Nobody is going to handle it.

>> diff --git a/test/src/emacs-tests.el b/test/src/emacs-tests.el
> 
> This should probably be in subr-tests.el since it tests a function in subr.el.

Right again, moved to a new file.

> I like tests that are somewhat repetitive but more decoupled and easier to read better than tests with factored-out assertions.

There is merit to such a style but the more important concern in this case is to avoid false positives and negatives, and make the tests robust in that regard.

It is very easy to make a mistake that renders a test meaningless, especially when testing a mechanism that does not alter the value of a computation but merely allows it to proceed or causes it to fail. The test has now been rewritten in a kind of oracular-combinatorial style which is effective in these situations. Doing so also improved coverage.

Thank you very much for your review and comments! I'd like to push the resulting patch but perhaps it and the rest of the sandbox development should go into a separate branch?


[-- Attachment #2: 0001-Add-macOS-sandboxing-bug-45198.patch --]
[-- Type: application/octet-stream, Size: 9382 bytes --]

From c5006dc5260f547ff132fd42c580ea0e0fb227a7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sat, 17 Apr 2021 20:53:39 +0200
Subject: [PATCH] Add macOS sandboxing (bug#45198)

This is the corresponding low-level sandboxing facility corresponding
to the recently added Seccomp for Linux.  `darwin-sandbox-init` gives
direct access to the system sandboxing call; `darwin-sandbox-enter` is
a wrapper that takes a list of directories under which files can be
read.  These should be considered internal mechanisms for now.

* lisp/darwin-fns.el: New file.
* lisp/loadup.el: Load it.
* src/sysdep.c (Fdarwin_sandbox_init): New function.
* test/lisp/darwin-fns-tests.el: New file.
---
 lisp/darwin-fns.el            | 40 ++++++++++++++++
 lisp/loadup.el                |  2 +
 src/sysdep.c                  | 34 +++++++++++++
 test/lisp/darwin-fns-tests.el | 90 +++++++++++++++++++++++++++++++++++
 4 files changed, 166 insertions(+)
 create mode 100644 lisp/darwin-fns.el
 create mode 100644 test/lisp/darwin-fns-tests.el

diff --git a/lisp/darwin-fns.el b/lisp/darwin-fns.el
new file mode 100644
index 0000000000..e45aaa0c4e
--- /dev/null
+++ b/lisp/darwin-fns.el
@@ -0,0 +1,40 @@
+;;; darwin-fns.el --- Darwin-specific functions  -*- lexical-binding: t -*-
+
+;; Copyright (C) 2021 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(defun darwin-sandbox-enter (dirs)
+  "Enter a sandbox only permitting reading files under DIRS.
+DIRS is a list of directory names.  Most other operations such as
+writing files and network access are disallowed.
+Existing open descriptors can still be used freely.
+
+This is not a supported interface and is for internal use only."
+  (darwin-sandbox-init
+   (concat "(version 1)\n"
+           "(deny default)\n"
+           ;; Emacs seems to need /dev/null; allowing it does no harm.
+           "(allow file-read* (path \"/dev/null\"))\n"
+           (mapconcat (lambda (dir)
+                        (format "(allow file-read* (subpath %S))\n" dir))
+                      dirs ""))))
+
+(provide 'darwin-fns)
+
+;;; darwin-fns.el ends here
diff --git a/lisp/loadup.el b/lisp/loadup.el
index d6cfcd6fc8..a6a7251d4b 100644
--- a/lisp/loadup.el
+++ b/lisp/loadup.el
@@ -324,6 +324,8 @@
       (load "term/pc-win")
       (load "ls-lisp")
       (load "disp-table"))) ; needed to setup ibm-pc char set, see internal.el
+(if (eq system-type 'darwin)
+    (load "darwin-fns"))
 (if (featurep 'ns)
     (progn
       (load "term/common-win")
diff --git a/src/sysdep.c b/src/sysdep.c
index d940acc4e0..4ea1f0050a 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -4286,8 +4286,42 @@ str_collate (Lisp_Object s1, Lisp_Object s2,
 }
 #endif	/* WINDOWSNT */
 
+#ifdef DARWIN_OS
+
+/* This function prototype is not in the platform header files.
+   See https://reverse.put.as/wp-content/uploads/2011/09/Apple-Sandbox-Guide-v1.0.pdf
+   and https://chromium.googlesource.com/chromium/src/+/master/sandbox/mac/seatbelt_sandbox_design.md */
+int sandbox_init_with_parameters(const char *profile,
+                                 uint64_t flags,
+                                 const char *const parameters[],
+                                 char **errorbuf);
+
+DEFUN ("darwin-sandbox-init", Fdarwin_sandbox_init, Sdarwin_sandbox_init,
+       1, 1, 0,
+       doc: /* Enter a sandbox whose permitted access is curtailed by PROFILE.
+Already open descriptors can be used freely.
+PROFILE is a string in the macOS sandbox profile language,
+a set of rules in a Lisp-like syntax.
+
+This is not a supported interface and is for internal use only. */)
+  (Lisp_Object profile)
+{
+  CHECK_STRING (profile);
+  if (memchr (SSDATA (profile), '\0', SBYTES (profile)))
+    error ("NUL in sandbox profile");
+  char *err = NULL;
+  if (sandbox_init_with_parameters (SSDATA (profile), 0, NULL, &err) != 0)
+    error ("sandbox error: %s", err);
+  return Qnil;
+}
+
+#endif	/* DARWIN_OS */
+
 void
 syms_of_sysdep (void)
 {
   defsubr (&Sget_internal_run_time);
+#ifdef DARWIN_OS
+  defsubr (&Sdarwin_sandbox_init);
+#endif
 }
diff --git a/test/lisp/darwin-fns-tests.el b/test/lisp/darwin-fns-tests.el
new file mode 100644
index 0000000000..1e426407a1
--- /dev/null
+++ b/test/lisp/darwin-fns-tests.el
@@ -0,0 +1,90 @@
+;;; darwin-fns-tests.el --- tests for darwin-fns.el  -*- lexical-binding: t -*-
+
+;; Copyright (C) 2021  Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published
+;; by the Free Software Foundation, either version 3 of the License,
+;; or (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful, but
+;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+;; General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+(require 'ert)
+
+(defun darwin-fns-tests--run-emacs (expr1 expr2)
+  "Run Emacs in batch mode and evaluate EXPR1 and EXPR2.
+Return (EXIT-STATUS . OUTPUT), where OUTPUT is stderr and stdout."
+  (let ((emacs (expand-file-name invocation-name invocation-directory))
+        (process-environment nil))
+    (with-temp-buffer
+      (let ((res (call-process emacs nil t nil
+                               "--quick" "--batch"
+                               (format "--eval=%S" expr1)
+                               (format "--eval=%S" expr2))))
+        (cons res (buffer-string))))))
+
+(ert-deftest darwin-fns-sandbox ()
+  (skip-unless (eq system-type 'darwin))
+  ;; Test file reading and writing under various sandboxing conditions.
+  (let* ((some-text "abcdef")
+         (new-text "ghijkl")
+         (test-file (file-truename (make-temp-file "test")))
+         (file-dir (file-name-directory test-file)))
+    (unwind-protect
+        (dolist (mode '(read write))
+          (ert-info ((symbol-name mode) :prefix "mode: ")
+            (dolist (sandbox '(allow-all deny-all allow-read))
+              (ert-info ((symbol-name sandbox) :prefix "sandbox: ")
+                ;; Prepare initial file contents.
+                (with-temp-buffer
+                  (insert some-text)
+                  (write-file test-file))
+
+                (let* ((sandbox-form
+                        (pcase-exhaustive sandbox
+                          ('allow-all nil)
+                          ('deny-all '(darwin-sandbox-enter nil))
+                          ('allow-read `(darwin-sandbox-enter '(,file-dir)))))
+                       (action-form
+                        (pcase-exhaustive mode
+                          ('read `(progn (find-file-literally ,test-file)
+                                         (message "OK: %s" (buffer-string))))
+                          ('write `(with-temp-buffer
+                                     (insert ,new-text)
+                                     (write-file ,test-file)))))
+                       (allowed (or (eq sandbox 'allow-all)
+                                    (and (eq sandbox 'allow-read)
+                                         (eq mode 'read))))
+                       (res-out (darwin-fns-tests--run-emacs
+                                 sandbox-form action-form))
+                       (exit-status (car res-out))
+                       (output (cdr res-out))
+                       (file-contents
+                        (with-temp-buffer
+                          (insert-file-contents-literally test-file)
+                          (buffer-string))))
+                  (if allowed
+                      (should (equal exit-status 0))
+                    (should-not (equal exit-status 0)))
+                  (when (eq mode 'read)
+                    (if allowed
+                        (should (equal output (format "OK: %s\n" some-text)))
+                      (should-not (string-search some-text output))))
+                  (should (equal file-contents
+                                 (if (and (eq mode 'write) allowed)
+                                     new-text
+                                   some-text))))))))
+
+      ;; Clean up.
+      (ignore-errors (delete-file test-file)))))
+
+
+(provide 'darwin-fns-tests)
-- 
2.21.1 (Apple Git-122.3)

bug#45198: 28.0.50; Sandbox mode

[Philipp]

> Am 18.04.2021 um 16:25 schrieb Stefan Monnier <monnier@IRO.UMontreal.CA>:
> 
>>> The whole point of the sandboxing exercise is so as to be able to have
>>> flymake-mode in the hook without exposing yourself to
>>> these vulnerabilities.
>> 
>> So we are going to introduce all this non-trivial machinery into Emacs
>> just to solve the Flymake use case?  Is that reasonable from the
>> project management POV, in your eyes?
> 
> To the extent that this machinery will only be used by those rare places
> that need it (e.g. flymake), yes, as long as the low-level interface
> (e.g. the code that added the support for the `--seccomp` argument) is
> simple enough.
> 
> BTW, in the context of GNU/Linux, an alternative to `--seccomp` is to
> require the `bwrap` tool (that's what I use in the `elpa-admin.el`
> scripts to run makefile rules from ELPA packages).
> 

I wouldn't call it an alternative, they are rather complementary approaches, and I think we should require both for a sandbox that we consider "hard enough".  The --seccomp flag doesn't set up namespaces; that's rather subtle and best done out-of-process by a dedicated tool like bwrap.  On the other hand, setting up a seccomp filter out-of-process has the disadvantage that it needs to allow execve, thereby increasing the attack surface quite a bit.  The sandboxing I'd have in mind would be a combination of bwrap (with namespaces and a seccomp filter that allows execve) and Emacs's own seccomp filter (banning execve).

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 1201 bytes --]

So far the discussion has been focussed on platform-dependent low-level sandbox implementation. I took a stab at writing something that can be used by portable code.

It's basically versions of `call-process` and `make-process` specialised for running batch-mode Emacs in a sandbox. The attached patch is a straw man proposal but that should serve as a starting point for agreement on what the interface might look like.

It's only been "tested" on macOS, and there will of course be ERT tests as well before it's ready. Everything can be changed.

The idea is to have something that could be used from alpa-admin.el or similar, and for running background Elisp byte-compilation.

It uses `make-process` rather than the simpler `start-process` for running an asynchronous Emacs because the former seemed to give greater control. There is currently only one sandbox parameter: the list of directories to make available for reading. Maybe there should be a list of writable directories as well?

We could also consider higher-level primitives, for example something that takes a Lisp expression to evaluate and returns the Lisp result, taking care of the intermediate printing and reading.


[-- Attachment #2: 0001-platform-independent-sandbox-interface.patch --]
[-- Type: application/octet-stream, Size: 4174 bytes --]

From 1dfea4588286ec4177619bd5d20502803a98c4c0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Fri, 17 Sep 2021 09:30:53 +0200
Subject: [PATCH] platform-independent sandbox interface

---
 lisp/sandbox.el | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 lisp/sandbox.el

diff --git a/lisp/sandbox.el b/lisp/sandbox.el
new file mode 100644
index 0000000000..a8069d59c7
--- /dev/null
+++ b/lisp/sandbox.el
@@ -0,0 +1,77 @@
+;;; -*- lexical-binding: t -*-
+
+(defconst sandbox-mechanism
+  ;; FIXME: make it a defcustom? What about other systems?
+  (cond ((eq system-type 'darwin) 'darwin)
+        ((eq system-type 'gnu/linux) 'bwrap)))
+
+(defun sandbox-available-p ()
+  "Non-nil if a sandboxing mechanism is available."
+  ;; FIXME: We should check for availability of bwrap etc.
+  (not (null sandbox-mechanism)))
+
+(defun sandbox--program-args (sandbox-spec prog)
+  "Return (PROGRAM . ARGS) for running PROG according to SANDBOX-SPEC."
+  (let ((allow-read-dirs (plist-get sandbox-spec :allow-read-dirs)))
+    ;; FIXME: Would `:allow-write-dirs' make sense and be useful?
+    (pcase-exhaustive sandbox-mechanism
+      ('darwin
+       (list prog "--eval"
+             (prin1-to-string `(darwin-sandbox-enter ',allow-read-dirs))))
+      ('bwrap
+       ;; FIXME: with seccomp?
+       `("bwrap"
+         "--unshare-all"
+         "--dev" "/dev"
+         "--proc" "/proc"
+         "--tmpfs" "/tmp"
+         ,@(mapcan (lambda (dir) (let ((d (expand-file-name dir)))
+                                   (list "--ro-bind" d d)))
+                   allow-read-dirs)
+         ,prog)))))
+
+(defun sandbox--emacs-command (sandbox-spec args)
+  (let* ((emacs (expand-file-name invocation-name invocation-directory))
+         (program-args (sandbox--program-args sandbox-spec emacs)))
+    `(,@program-args "--batch" ,@args)))
+
+(defun sandbox-run-emacs (sandbox-spec destination args)
+  "Run sandboxed Emacs in batch mode, synchronously.
+SANDBOX-SPEC is a sandbox specification plist.  Currently defined key:
+ `:allow-read-dirs' -- the value is a list of directories that can
+                       be read from (but not written to).
+DESTINATION is as in `call-process'.
+ARGS is a list of command-line arguments passed to the sandboxed Emacs.
+Return value is as in `call-process'.
+
+Depending on the platform, the sandbox restrictions do not necessarily
+take effect until Emacs has been initialised and loaded the site and user
+init files.  If that is not desirable, suppress their use by adding the
+corresponding flags (eg \"-Q\") to ARGS."
+  (let ((command (sandbox--emacs-command sandbox-spec args)))
+    (apply #'call-process (car command) nil destination nil (cdr command))))
+
+(defun sandbox-start-emacs (sandbox-spec params args)
+  "Run sandboxed Emacs in batch mode, asynchronously.
+SANDBOX-SPEC is a sandbox specification plist.  Currently defined key:
+ `:allow-read-dirs' -- the value is a list of directories that can
+                       be read from (but not written to).
+ARGS is a list of command-line arguments passed to the sandboxed Emacs.
+PARAMS is a plist of parameters passed to `make-process'.  Do not
+  supply `:command'; it will be overridden by ARGS.
+Return value is as in `make-process'.
+
+Depending on the platform, the sandbox restrictions do not necessarily
+take effect until Emacs has been initialised and loaded the site and user
+init files.  If that is not desirable, suppress their use by adding the
+corresponding flags (eg \"-Q\") to ARGS."
+  (let* ((command (sandbox--emacs-command sandbox-spec args))
+         (params (copy-sequence params))
+         (params (plist-put params :command command)))
+    (unless (plist-member params :name)
+      (setq params (plist-put params :name "emacs")))
+    (unless (plist-member params :connection-type)
+      (setq params (plist-put params :connection-type 'pipe)))
+    (apply #'make-process params)))
+
+(provide 'sandbox)
-- 
2.21.1 (Apple Git-122.3)


[-- Attachment #3: Type: text/plain, Size: 2 bytes --]

bug#45198: 28.0.50; Sandbox mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> It's basically versions of `call-process` and `make-process` specialised for
> running batch-mode Emacs in a sandbox. The attached patch is a straw man
> proposal but that should serve as a starting point for agreement on what the
> interface might look like.

For `elpa-admin.el` we need a writable directory as well.
We also need the ability to run sub-processes.  Your `bwrap`
implementation for GNU/Linux should allow that, AFAICT, but I can't tell
if `darwin-sandbox-enter` also allows it.


        Stefan

bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 497 bytes --]

17 sep. 2021 kl. 15.20 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> For `elpa-admin.el` we need a writable directory as well.
> We also need the ability to run sub-processes.  Your `bwrap`
> implementation for GNU/Linux should allow that, AFAICT, but I can't tell
> if `darwin-sandbox-enter` also allows it.

Looks like it can be made to work.

Of course this whole exercise doesn't really touch the questions that really matter, such as whether it is practical for actual use.


[-- Attachment #2: 0001-Add-macOS-sandboxing-bug-45198.patch --]
[-- Type: application/octet-stream, Size: 10223 bytes --]

From 03233ad9abb0c18bdbd00eb2cad42db8a252cafe Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sat, 17 Apr 2021 20:53:39 +0200
Subject: [PATCH 1/2] Add macOS sandboxing (bug#45198)

This is the corresponding low-level sandboxing facility corresponding
to the recently added Seccomp for Linux.  `darwin-sandbox-init` gives
direct access to the system sandboxing call; `darwin--sandbox-enter`
is a wrapper that takes a plist specifying directories under which
files can be read, written or executed.  These should be considered
internal mechanisms for now.

* lisp/darwin-fns.el: New file.
* lisp/loadup.el: Load it.
* src/sysdep.c (Fdarwin_sandbox_init): New function.
* test/lisp/darwin-fns-tests.el: New file.
---
 lisp/darwin-fns.el            | 56 +++++++++++++++++++++
 lisp/loadup.el                |  2 +
 src/sysdep.c                  | 34 +++++++++++++
 test/lisp/darwin-fns-tests.el | 91 +++++++++++++++++++++++++++++++++++
 4 files changed, 183 insertions(+)
 create mode 100644 lisp/darwin-fns.el
 create mode 100644 test/lisp/darwin-fns-tests.el

diff --git a/lisp/darwin-fns.el b/lisp/darwin-fns.el
new file mode 100644
index 0000000000..feba9739b5
--- /dev/null
+++ b/lisp/darwin-fns.el
@@ -0,0 +1,56 @@
+;;; darwin-fns.el --- Darwin-specific functions  -*- lexical-binding: t -*-
+
+;; Copyright (C) 2021 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(defun darwin--sandbox-enter (spec)
+  "Enter a sandbox only permitting actions described by SPEC.
+SPEC is a plist allowing the keys:
+`:read-dirs'  -- value is a list of directories in which reading is allowed.
+`:write-dirs' -- value is a list of directories in which writing is allowed.
+`:exec-dirs'  -- value is a list of directories from which executables
+                 can be run as subprocesses.
+Most other operations such as network access are disallowed.
+Existing open descriptors can still be used freely.
+
+This is not a supported interface and is for internal use only."
+  (let ((read-dirs (plist-get spec :read-dirs))
+        (write-dirs (plist-get spec :write-dirs))
+        (exec-dirs (plist-get spec :exec-dirs)))
+    (darwin-sandbox-init
+     (concat
+      "(version 1)\n"
+      "(deny default)\n"
+      ;; Emacs seems to need /dev/null; allowing it does no harm.
+      "(allow file-read* (path \"/dev/null\"))\n"
+      (mapconcat (lambda (dir)
+                   (format "(allow file-read* (subpath %S))\n" dir))
+                 read-dirs "")
+      (mapconcat (lambda (dir)
+                   (format "(allow file-write* (subpath %S))\n" dir))
+                 write-dirs "")
+      (mapconcat (lambda (dir)
+                   (format "(allow process-exec (subpath %S))\n" dir))
+                 exec-dirs "")
+      (and exec-dirs
+           "(allow process-fork)\n")))))
+
+(provide 'darwin-fns)
+
+;;; darwin-fns.el ends here
diff --git a/lisp/loadup.el b/lisp/loadup.el
index 158c02ecea..163b639640 100644
--- a/lisp/loadup.el
+++ b/lisp/loadup.el
@@ -325,6 +325,8 @@
       (load "term/pc-win")
       (load "ls-lisp")
       (load "disp-table"))) ; needed to setup ibm-pc char set, see internal.el
+(if (eq system-type 'darwin)
+    (load "darwin-fns"))
 (if (featurep 'ns)
     (progn
       (load "term/common-win")
diff --git a/src/sysdep.c b/src/sysdep.c
index 8eaee22498..79a1fad4da 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -4458,8 +4458,42 @@ str_collate (Lisp_Object s1, Lisp_Object s2,
 }
 #endif	/* WINDOWSNT */
 
+#ifdef DARWIN_OS
+
+/* This function prototype is not in the platform header files.
+   See https://reverse.put.as/wp-content/uploads/2011/09/Apple-Sandbox-Guide-v1.0.pdf
+   and https://chromium.googlesource.com/chromium/src/+/master/sandbox/mac/seatbelt_sandbox_design.md */
+int sandbox_init_with_parameters(const char *profile,
+                                 uint64_t flags,
+                                 const char *const parameters[],
+                                 char **errorbuf);
+
+DEFUN ("darwin-sandbox-init", Fdarwin_sandbox_init, Sdarwin_sandbox_init,
+       1, 1, 0,
+       doc: /* Enter a sandbox whose permitted access is curtailed by PROFILE.
+Already open descriptors can be used freely.
+PROFILE is a string in the macOS sandbox profile language,
+a set of rules in a Lisp-like syntax.
+
+This is not a supported interface and is for internal use only. */)
+  (Lisp_Object profile)
+{
+  CHECK_STRING (profile);
+  if (memchr (SSDATA (profile), '\0', SBYTES (profile)))
+    error ("NUL in sandbox profile");
+  char *err = NULL;
+  if (sandbox_init_with_parameters (SSDATA (profile), 0, NULL, &err) != 0)
+    error ("sandbox error: %s", err);
+  return Qnil;
+}
+
+#endif	/* DARWIN_OS */
+
 void
 syms_of_sysdep (void)
 {
   defsubr (&Sget_internal_run_time);
+#ifdef DARWIN_OS
+  defsubr (&Sdarwin_sandbox_init);
+#endif
 }
diff --git a/test/lisp/darwin-fns-tests.el b/test/lisp/darwin-fns-tests.el
new file mode 100644
index 0000000000..fa0d58ac3d
--- /dev/null
+++ b/test/lisp/darwin-fns-tests.el
@@ -0,0 +1,91 @@
+;;; darwin-fns-tests.el --- tests for darwin-fns.el  -*- lexical-binding: t -*-
+
+;; Copyright (C) 2021  Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published
+;; by the Free Software Foundation, either version 3 of the License,
+;; or (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful, but
+;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+;; General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+(require 'ert)
+
+(defun darwin-fns-tests--run-emacs (expr1 expr2)
+  "Run Emacs in batch mode and evaluate EXPR1 and EXPR2.
+Return (EXIT-STATUS . OUTPUT), where OUTPUT is stderr and stdout."
+  (let ((emacs (expand-file-name invocation-name invocation-directory))
+        (process-environment nil))
+    (with-temp-buffer
+      (let ((res (call-process emacs nil t nil
+                               "--quick" "--batch"
+                               (format "--eval=%S" expr1)
+                               (format "--eval=%S" expr2))))
+        (cons res (buffer-string))))))
+
+(ert-deftest darwin-fns-sandbox ()
+  (skip-unless (eq system-type 'darwin))
+  ;; Test file reading and writing under various sandboxing conditions.
+  (let* ((some-text "abcdef")
+         (new-text "ghijkl")
+         (test-file (file-truename (make-temp-file "test")))
+         (file-dir (file-name-directory test-file)))
+    (unwind-protect
+        (dolist (mode '(read write))
+          (ert-info ((symbol-name mode) :prefix "mode: ")
+            (dolist (sandbox '(allow-all deny-all allow-read))
+              (ert-info ((symbol-name sandbox) :prefix "sandbox: ")
+                ;; Prepare initial file contents.
+                (with-temp-buffer
+                  (insert some-text)
+                  (write-file test-file))
+
+                (let* ((sandbox-form
+                        (pcase-exhaustive sandbox
+                          ('allow-all nil)
+                          ('deny-all '(darwin--sandbox-enter nil))
+                          ('allow-read `(darwin--sandbox-enter
+                                         '(:read-dirs (,file-dir))))))
+                       (action-form
+                        (pcase-exhaustive mode
+                          ('read `(progn (find-file-literally ,test-file)
+                                         (message "OK: %s" (buffer-string))))
+                          ('write `(with-temp-buffer
+                                     (insert ,new-text)
+                                     (write-file ,test-file)))))
+                       (allowed (or (eq sandbox 'allow-all)
+                                    (and (eq sandbox 'allow-read)
+                                         (eq mode 'read))))
+                       (res-out (darwin-fns-tests--run-emacs
+                                 sandbox-form action-form))
+                       (exit-status (car res-out))
+                       (output (cdr res-out))
+                       (file-contents
+                        (with-temp-buffer
+                          (insert-file-contents-literally test-file)
+                          (buffer-string))))
+                  (if allowed
+                      (should (equal exit-status 0))
+                    (should-not (equal exit-status 0)))
+                  (when (eq mode 'read)
+                    (if allowed
+                        (should (equal output (format "OK: %s\n" some-text)))
+                      (should-not (string-search some-text output))))
+                  (should (equal file-contents
+                                 (if (and (eq mode 'write) allowed)
+                                     new-text
+                                   some-text))))))))
+
+      ;; Clean up.
+      (ignore-errors (delete-file test-file)))))
+
+
+(provide 'darwin-fns-tests)
-- 
2.21.1 (Apple Git-122.3)


[-- Attachment #3: 0002-platform-independent-sandbox-interface.patch --]
[-- Type: application/octet-stream, Size: 4934 bytes --]

From aa7780d2a40cf1da60ae236e9468cea9c36a8350 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Fri, 17 Sep 2021 09:30:53 +0200
Subject: [PATCH 2/2] platform-independent sandbox interface

---
 lisp/sandbox.el | 91 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 lisp/sandbox.el

diff --git a/lisp/sandbox.el b/lisp/sandbox.el
new file mode 100644
index 0000000000..589d25615a
--- /dev/null
+++ b/lisp/sandbox.el
@@ -0,0 +1,91 @@
+;;; -*- lexical-binding: t -*-
+
+(require 'cl-lib)
+
+(defconst sandbox-mechanism
+  ;; FIXME: make it a defcustom? What about other systems?
+  (cond ((eq system-type 'darwin) 'darwin)
+        ((eq system-type 'gnu/linux) 'bwrap)))
+
+(defun sandbox-available-p ()
+  "Non-nil if a sandboxing mechanism is available."
+  ;; FIXME: We should check for availability of bwrap etc.
+  (not (null sandbox-mechanism)))
+
+(defun sandbox--program-args (sandbox-spec prog)
+  "Return (PROGRAM . ARGS) for running PROG according to SANDBOX-SPEC."
+  (pcase-exhaustive sandbox-mechanism
+    ('darwin
+     (list prog "--eval"
+           (prin1-to-string `(darwin--sandbox-enter ',sandbox-spec))))
+    ('bwrap
+     ;; FIXME: with seccomp?
+     (let* ((read-dirs (plist-get sandbox-spec :read-dirs))
+            (write-dirs (plist-get sandbox-spec :write-dirs))
+            (exec-dirs (plist-get sandbox-spec :exec-dirs))
+            (ro-dirs (cl-set-difference
+                      (cl-union read-dirs exec-dirs :test #'equal)
+                      write-dirs :test #'equal)))
+       `("bwrap"
+         "--unshare-all"
+         "--dev" "/dev"
+         "--proc" "/proc"
+         "--tmpfs" "/tmp"
+         ,@(mapcan (lambda (dir) (let ((d (expand-file-name dir)))
+                                   (list "--ro-bind" d d)))
+                   ro-dirs)
+         ,@(mapcan (lambda (dir) (let ((d (expand-file-name dir)))
+                                   (list "--bind" d d)))
+                   write-dirs)
+         ,prog)))))
+
+(defun sandbox--emacs-command (sandbox-spec args)
+  "Command and arguments for running Emacs with SANDBOX-SPEC and ARGS."
+  (let* ((emacs (expand-file-name invocation-name invocation-directory))
+         (program-args (sandbox--program-args sandbox-spec emacs)))
+    `(,@program-args "--batch" ,@args)))
+
+(defun sandbox-run-emacs (sandbox-spec destination args)
+  "Run sandboxed Emacs in batch mode, synchronously.
+SANDBOX-SPEC is a sandbox specification plist.  Currently defined key:
+ `:read-dirs'  -- the value is a list of directories that can be read from.
+ `:write-dirs' -- the value is a list of directories that can be written to.
+ `:exec-dirs'  -- the value is a list of directories from which
+                  executables can be run as subprocesses.
+DESTINATION is as in `call-process'.
+ARGS is a list of command-line arguments passed to the sandboxed Emacs.
+Return value is as in `call-process'.
+
+Depending on the platform, the sandbox restrictions do not necessarily
+take effect until Emacs has been initialised and loaded the site and user
+init files.  If that is not desirable, suppress their use by adding the
+corresponding flags (eg \"-Q\") to ARGS."
+  (let ((command (sandbox--emacs-command sandbox-spec args)))
+    (apply #'call-process (car command) nil destination nil (cdr command))))
+
+(defun sandbox-start-emacs (sandbox-spec params args)
+  "Run sandboxed Emacs in batch mode, asynchronously.
+SANDBOX-SPEC is a sandbox specification plist.  Currently defined key:
+ `:read-dirs'  -- the value is a list of directories that can be read from.
+ `:write-dirs' -- the value is a list of directories that can be written to.
+ `:exec-dirs'  -- the value is a list of directories from which
+                  executables can be run as subprocesses.
+ARGS is a list of command-line arguments passed to the sandboxed Emacs.
+PARAMS is a plist of parameters passed to `make-process'.  Do not
+  supply `:command'; it will be overridden by ARGS.
+Return value is as in `make-process'.
+
+Depending on the platform, the sandbox restrictions do not necessarily
+take effect until Emacs has been initialised and loaded the site and user
+init files.  If that is not desirable, suppress their use by adding the
+corresponding flags (eg \"-Q\") to ARGS."
+  (let* ((command (sandbox--emacs-command sandbox-spec args))
+         (params (copy-sequence params))
+         (params (plist-put params :command command)))
+    (unless (plist-member params :name)
+      (setq params (plist-put params :name "emacs")))
+    (unless (plist-member params :connection-type)
+      (setq params (plist-put params :connection-type 'pipe)))
+    (apply #'make-process params)))
+
+(provide 'sandbox)
-- 
2.21.1 (Apple Git-122.3)

bug#45198: 28.0.50; Sandbox mode

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> Of course this whole exercise doesn't really touch the questions that
> really matter, such as whether it is practical for actual use.

This was a year ago, but it looks like none of these patches were
applied?

I think having a sandbox mode would certainly be good in principle.

bug#45198: 28.0.50; Sandbox mode

[mattiase]

11 sep. 2022 kl. 13.28 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> This was a year ago, but it looks like none of these patches were
> applied?

Probably means they weren't very good to begin with.

> I think having a sandbox mode would certainly be good in principle.

Same here, but I know how perilous it is to design interfaces without a concrete and obviously useful application from the start so let's be careful.

bug#45198: 28.0.50; Sandbox mode

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

On Tue, Sep 13, 2022, 13:37 <mattiase@acm.org> wrote:

> 11 sep. 2022 kl. 13.28 skrev Lars Ingebrigtsen <larsi@gnus.org>:
>
> > This was a year ago, but it looks like none of these patches were
> > applied?
>
> Probably means they weren't very good to begin with.
>

Heh. That's a bit harsh, but also true more often than not.

>
> > I think having a sandbox mode would certainly be good in principle.
>
> Same here, but I know how perilous it is to design interfaces without a
> concrete and obviously useful application from the start so let's be
> careful.


I agree. Here's an obviously useful application in my humble opinion: to
turn on Elisp's Flymake checker by default.

To do that, we must ensure that this checker, which starts an emacs
inferior process to byte-compile Lisp code, is guaranteed not to cause
unintended side-effects.

This inferior Emacs macro-expands macro calls and thus and runs code:
there's no other way to compile Lisp code. It must thus not be allowed to
do "unsandboxy" things like writing to the file system or network. Probably
also not starting other processes. But probably it should be allowed to
cons lists and intern symbols inside its address space.

João

[-- Attachment #2: Type: text/html, Size: 2080 bytes --]

bug#45198: 28.0.50; Sandbox mode

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]

On Tue, Sep 13, 2022 at 1:53 PM João Távora <joaotavora@gmail.com> wrote:

>
>
> On Tue, Sep 13, 2022, 13:37 <mattiase@acm.org> wrote:
>
>> 11 sep. 2022 kl. 13.28 skrev Lars Ingebrigtsen <larsi@gnus.org>:
>>
>> > This was a year ago, but it looks like none of these patches were
>> > applied?
>>
>> Probably means they weren't very good to begin with.
>>
>
> Heh. That's a bit harsh, but also true more often than not.
>

Doh.  I just realised you were talking about your own patches...
FWIW I haven't looked at them, so just a generic, gratuitous
utterance.

João

[-- Attachment #2: Type: text/html, Size: 1382 bytes --]