fix: Require an explicit opt in to unsafety; defer decision to call time (#246)

Codejail currently makes a decision at module load time of whether it
should run all code safely or unsafely, and defaults to unsafely. This
causes several problems:

- Any misconfiguration of codejail (such as a missing Django setting or
  middleware) results in the application becoming immediately and entirely
  vulnerable to anyone who can submit code.
- Codejail's behavior changes depending on when the `codejail.safe_exec`
  module is loaded during application initialization. This causes unstable
  behavior and is confusing for developers.

This change switches the `ALWAYS_BE_UNSAFE` check to occur only at the time
that `safe_exec` is actually called, rather than at module load time.

The check for whether codejail is configured for Python is also moved to
call time, but no longer automatically switches codejail to unsafe mode.
Instead, it raises an exception to notify the user of their error.

Author: timmc-edx

README.rst

@@ -6,18 +6,11 @@ designed primarily for Python execution, but can be used for other languages as
 well.
 
 Security is enforced with AppArmor.  If your operating system doesn't support
-AppArmor, then CodeJail won't protect the execution.
+AppArmor, or if the AppArmor profile is not defined and configured correctly,
+then CodeJail will not protect the execution.
 
 CodeJail is designed to be configurable, and will auto-configure itself for
-Python execution if you install it properly.  The configuration is designed to
-be flexible: it can run in safe mode or unsafe mode.  This helps support large
-development groups where only some of the developers are involved enough with
-secure execution to configure AppArmor on their development machines.
-
-If CodeJail is not configured for safe execution, it will execution Python
-using the same API, but will not guard against malicious code.  This allows the
-same code to be used on safe-configured or non-safe-configured developer's
-machines.
+Python execution if you install it properly.
 
 A CodeJail sandbox consists of several pieces:
 
@@ -64,10 +57,12 @@ Installation
 ------------
 
 These instructions detail how to configure your operating system so that
-CodeJail can execute Python code safely.  You can run CodeJail without these
-steps, and you will have an unsafe CodeJail.  This is fine for developers'
-machines who are unconcerned with security, and simplifies the integration of
-CodeJail into your project.
+CodeJail can execute Python code safely. However, it is also possible to set
+``codejail.safe_exec.ALWAYS_BE_UNSAFE = True`` and execute submitted Python
+directly on the machine, with no security whatsoever. This may be fine for
+developers' machines who are unconcerned with security, and allows testing
+an integration with CodeJail's API. It must not be used if any input is coming
+from untrusted sources, however. **Do not use this option in production systems.**
 
 To secure Python execution, you'll be creating a new virtualenv.  This means
 you'll have two: the main virtualenv for your project, and the new one for
@@ -166,8 +161,8 @@ the rights to modify the files in its site-packages directory.
 Tests
 -----
 
-In order to target the sandboxed Python environment(s) you have created on your
-system, you must set the following environment variables for testing::
+To run tests, you must perform the standard installation steps. Then
+you must set the following environment variables::
 
     $ export CODEJAIL_TEST_USER=<owner of sandbox (usually 'sandbox')>
     $ export CODEJAIL_TEST_VENV=<SANDENV>
@@ -176,10 +171,7 @@ Run the tests with the Makefile::
 
     $ make tests
 
-If CodeJail is running unsafely, many of the tests will be automatically
-skipped, or will fail, depending on whether CodeJail thinks it should be in
-safe mode or not.
-
+Several proxy tests are skipped if proxy mode is not configured.
 
 Design
 ------
@@ -214,7 +206,7 @@ the subprocess as JSON.
 Limitations
 -----------
 
-* If codejail or AppArmor is not configured properly, codejail will default to
+* If codejail or AppArmor is not configured properly, codejail may default to
   running code insecurely (no sandboxing). It is not secure by default.
   Projects integrating codejail should consider including a runtime test suite
   that checks for proper confinement at startup before untrusted inputs are

codejail/jail_code.py

@@ -68,7 +68,7 @@ def is_configured(command):
 )
 
 if running_in_virtualenv:
-    # On jenkins
+    # In test environment
     sandbox_user = os.getenv('CODEJAIL_TEST_USER')
     sandbox_env = os.getenv('CODEJAIL_TEST_VENV')
     if sandbox_env and sandbox_user:

codejail/safe_exec.py

@@ -23,7 +23,13 @@
 
 # Set this to True to log all the code and globals being executed.
 LOG_ALL_CODE = False
-# Set this to True to use the unsafe code, so that you can debug it.
+
+# Set this to True to run submitted code with no confinement and no sandbox.
+#
+# WARNING: This is deeply dangerous; anyone who can submit code can take
+# over the computer immediately and entirely.
+#
+# The only purpose of this setting is for local debugging.
 ALWAYS_BE_UNSAFE = False
 
 
@@ -80,8 +86,22 @@ def safe_exec(
     the code raises an exception, this function will raise `SafeExecException`
     with the stderr of the sandbox process, which usually includes the original
     exception message and traceback.
-
     """
+    if ALWAYS_BE_UNSAFE:
+        not_safe_exec(
+            code,
+            globals_dict,
+            files=files,
+            python_path=python_path,
+            limit_overrides_context=limit_overrides_context,
+            slug=slug,
+            extra_files=extra_files,
+        )
+        return
+
+    if not jail_code.is_configured('python'):
+        raise RuntimeError("safe_exec has not been configured for Python")
+
     the_code = []
 
     files = list(files or ())
@@ -257,6 +277,11 @@ def not_safe_exec(
     Note that `limit_overrides_context` is ignored here, because resource limits
     are not applied.
     """
+    # Because it would be bad if this function were used in production,
+    # let's log a warning when it is used.  Developers can live with
+    # one more log line.
+    log.warning("DANGER: Executing code with `not_safe_exec` for %s", slug)
+
     g_dict = json_safe(globals_dict)
 
     with temp_directory() as tmpdir:
@@ -286,22 +311,3 @@ def not_safe_exec(
                 sys.path = original_path
 
     globals_dict.update(json_safe(g_dict))
-
-
-# If the developer wants us to be unsafe (ALWAYS_BE_UNSAFE), or if there isn't
-# a configured jail for Python, then we'll be UNSAFE.
-UNSAFE = ALWAYS_BE_UNSAFE or not jail_code.is_configured("python")
-
-if UNSAFE:   # pragma: no cover
-    # Make safe_exec actually call not_safe_exec, but log that we're doing so.
-
-    def safe_exec(*args, **kwargs):                 # pylint: disable=E0102
-        """An actually-unsafe safe_exec, that warns it's being used."""
-
-        # Because it would be bad if this function were used in production,
-        # let's log a warning when it is used.  Developers can live with
-        # one more log line.
-        slug = kwargs.get('slug', None)
-        log.warning("Using codejail/safe_exec.py:not_safe_exec for %s", slug)
-
-        return not_safe_exec(*args, **kwargs)

codejail/tests/test_safe_exec.py

@@ -4,7 +4,10 @@
 import textwrap
 import zipfile
 from io import BytesIO
-from unittest import SkipTest, TestCase
+from unittest import TestCase
+from unittest.mock import patch
+
+import pytest
 
 from codejail import safe_exec
 from codejail.jail_code import set_limit
@@ -176,17 +179,36 @@ class TestSafeExec(SafeExecTests, TestCase):
     def safe_exec(self, *args, **kwargs):
         safe_exec.safe_exec(*args, **kwargs)
 
+    @patch('codejail.jail_code.is_configured', return_value=False)
+    def test_configuration(self, mock_is_configured):
+        """
+        When not configured for Python, raise an exception.
+        """
+        with pytest.raises(RuntimeError, match=r'safe_exec has not been configured for Python'):
+            self.safe_exec('out = 1 + 2', {})
+
+        mock_is_configured.assert_called_once_with('python')
+
+    @patch('codejail.safe_exec.not_safe_exec')
+    @patch('codejail.jail_code.is_configured', return_value=True)
+    def test_opt_unsafe(self, mock_is_configured, mock_not_safe_exec):
+        """
+        When ALWAYS_BE_UNSAFE enabled, go immediately to not_safe_exec.
+        """
+        with patch.object(safe_exec, 'ALWAYS_BE_UNSAFE', new=True):
+            self.safe_exec('out = 1 + 2', {})
+
+        mock_is_configured.assert_not_called()
+        mock_not_safe_exec.assert_called_once_with(
+            'out = 1 + 2', {},
+            files=None, python_path=None, limit_overrides_context=None, slug=None, extra_files=None,
+        )
+
 
 class TestNotSafeExec(SafeExecTests, TestCase):
     """Run SafeExecTests, with not_safe_exec."""
 
     __test__ = True
 
-    def setUp(self):
-        # If safe_exec is actually an alias to not_safe_exec, then there's no
-        # point running these tests.
-        if safe_exec.UNSAFE:                    # pragma: no cover
-            raise SkipTest
-
     def safe_exec(self, *args, **kwargs):
         safe_exec.not_safe_exec(*args, **kwargs)