bug#45198: 28.0.50; Sandbox mode

[Mattias Engdegård]

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?

0001-Add-macOS-sandboxing-bug-45198.patch
Description: Binary data