From 5639a400e633a48c8564a61a035485933229d4f5 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Fri, 20 Nov 2020 14:21:14 +0100 Subject: Limit STDOUT to prevent OOM events in container Recently, we discovered that for some code inputs, snekbox would get into an OOM event on the container level, seemingly bypassing the memory restrictions laid on code execution by NSJail. After investigating the issue, we identified the culprit to be the STDOUT pipe we use to get output back from NSJail: As output is piped out of the jailed process, it will be gathered outside of the NSJail in the main container process instead. This meant that our initial attempts of limiting the allowed filesize within the NSJail failed, as the OOM happened outside of the jailed environment. To mitigate the issue, I've written a loop that consumes the STDOUT pipe in chunks of 100 characters. Once the size of the accrued output reaches a certain limit (currently set to 1 MB), we send a SIGTERM signal to NSJail to terminate itself. The output up to that point will be relayed back to the caller. A minimal code snippet to trigger the event and the mitigation: ```py while True: print(" ") ``` I've included a test for this vulnerability in `tests/test_nsjail.py`. --- tests/test_nsjail.py | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'tests') diff --git a/tests/test_nsjail.py b/tests/test_nsjail.py index 0b755b2..852be4b 100644 --- a/tests/test_nsjail.py +++ b/tests/test_nsjail.py @@ -174,3 +174,12 @@ class NsJailTests(unittest.TestCase): msg="stdout does not come before stderr" ) self.assertEqual(result.stderr, None) + + def test_stdout_flood_results_in_graceful_sigterm(self): + stdout_flood = dedent(""" + while True: + print('abcdefghij') + """).strip() + + result = self.nsjail.python3(stdout_flood) + self.assertEqual(result.returncode, 143) -- cgit v1.2.3 From 18ab777eef3045421f4df3b127079833a8c3cbe5 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Fri, 20 Nov 2020 22:29:47 +0100 Subject: Use SIGKILL instead of SIGTERM to terminate NsJail This new behavior matches how other limiters terminate the subprocess, resulting in a more consistency in the front-end for the end users as well. --- snekbox/nsjail.py | 6 +++--- tests/test_nsjail.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/snekbox/nsjail.py b/snekbox/nsjail.py index a1695a5..6fc0343 100644 --- a/snekbox/nsjail.py +++ b/snekbox/nsjail.py @@ -112,7 +112,7 @@ class NsJail: The aim of this function is to limit the size of the output received from NsJail to prevent container from claiming too much memory. If the output received from STDOUT goes over the OUTPUT_MAX limit, the NsJail subprocess - is asked to terminate with a SIGTERM. + is asked to terminate with a SIGKILL. Once the subprocess has exited, either naturally or because it was terminated, the output up to that point is returned as a string. @@ -127,8 +127,8 @@ class NsJail: output.append(chars) if output_size > OUTPUT_MAX: - # Ask NsJail to terminate as we've gone over the output limit. - nsjail.terminate() + # Terminate the NsJail subprocess with SIGKILL. + nsjail.kill() break # Ensure that we wait for the NsJail subprocess to terminate. diff --git a/tests/test_nsjail.py b/tests/test_nsjail.py index 852be4b..4a040e7 100644 --- a/tests/test_nsjail.py +++ b/tests/test_nsjail.py @@ -182,4 +182,4 @@ class NsJailTests(unittest.TestCase): """).strip() result = self.nsjail.python3(stdout_flood) - self.assertEqual(result.returncode, 143) + self.assertEqual(result.returncode, -9) -- cgit v1.2.3 From 162e669032f377e0ba3751ed5a924c84789696d8 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Fri, 20 Nov 2020 23:46:19 +0100 Subject: Convert negative exit codes into standard form When you send a signal `N` to a subprocess using Popen, it will return `-N` as its exit code. As the rest of the code returns signal exit codes as `128 + N`, we convert those negative exit codes into the standard form used by the rest of the code. --- snekbox/nsjail.py | 22 ++++++++++++++-------- tests/test_nsjail.py | 2 +- 2 files changed, 15 insertions(+), 9 deletions(-) (limited to 'tests') diff --git a/snekbox/nsjail.py b/snekbox/nsjail.py index 6fc0343..8edf546 100644 --- a/snekbox/nsjail.py +++ b/snekbox/nsjail.py @@ -7,7 +7,7 @@ import textwrap from pathlib import Path from subprocess import CompletedProcess from tempfile import NamedTemporaryFile -from typing import Iterable +from typing import Iterable, Tuple from snekbox import DEBUG @@ -105,7 +105,7 @@ class NsJail: log.error(msg) @staticmethod - def _consume_stdout(nsjail: subprocess.Popen) -> str: + def _consume_stdout(nsjail: subprocess.Popen) -> Tuple[int, str]: """ Consume STDOUT, stopping when the output limit is reached or NsJail has exited. @@ -115,7 +115,7 @@ class NsJail: is asked to terminate with a SIGKILL. Once the subprocess has exited, either naturally or because it was terminated, - the output up to that point is returned as a string. + we return the exit code as an int and the output as a single string. """ output_size = 0 output = [] @@ -128,13 +128,19 @@ class NsJail: if output_size > OUTPUT_MAX: # Terminate the NsJail subprocess with SIGKILL. + log.info(f"Output exceeded the output limit, sending SIGKILL to NsJail.") nsjail.kill() break # Ensure that we wait for the NsJail subprocess to terminate. nsjail.wait() - return "".join(output) + # When you send signal `N` to a subprocess to terminate it using Popen, it + # will return `-N` as its exit code. As we normally get `N + 128` back, we + # convert negative exit codes to the `N + 128` form. + returncode = -nsjail.returncode + 128 if nsjail.returncode < 0 else nsjail.returncode + + return returncode, "".join(output) def python3(self, code: str) -> CompletedProcess: """Execute Python 3 code in an isolated environment and return the completed process.""" @@ -168,14 +174,14 @@ class NsJail: except ValueError: return CompletedProcess(args, None, "ValueError: embedded null byte", None) - output = self._consume_stdout(nsjail) + returncode, output = self._consume_stdout(nsjail) log_lines = nsj_log.read().decode("utf-8").splitlines() - if not log_lines and nsjail.returncode == 255: + if not log_lines and returncode == 255: # NsJail probably failed to parse arguments so log output will still be in stdout log_lines = output.splitlines() self._parse_log(log_lines) - log.info(f"nsjail return code: {nsjail.returncode}") - return CompletedProcess(args, nsjail.returncode, output, None) + log.info(f"nsjail return code: {returncode}") + return CompletedProcess(args, returncode, output, None) diff --git a/tests/test_nsjail.py b/tests/test_nsjail.py index 4a040e7..691eb5b 100644 --- a/tests/test_nsjail.py +++ b/tests/test_nsjail.py @@ -182,4 +182,4 @@ class NsJailTests(unittest.TestCase): """).strip() result = self.nsjail.python3(stdout_flood) - self.assertEqual(result.returncode, -9) + self.assertEqual(result.returncode, 137) -- cgit v1.2.3 From 1afd4ec766d10ddea8108fa439c793a06a63c78b Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Sat, 21 Nov 2020 00:10:01 +0100 Subject: Add test for stdout output truncation I've added a test that checks if output exceeding the limit is correctly truncated. To make the test more robust, I've defined a constant for the read chunk size. --- snekbox/nsjail.py | 3 ++- tests/test_nsjail.py | 20 +++++++++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/snekbox/nsjail.py b/snekbox/nsjail.py index 8edf546..4dea3cb 100644 --- a/snekbox/nsjail.py +++ b/snekbox/nsjail.py @@ -29,6 +29,7 @@ MEM_MAX = 52428800 # Limit of stdout bytes we consume before terminating nsjail OUTPUT_MAX = 1_000_000 # 1 MB +READ_CHUNK_SIZE = 10_000 # chars class NsJail: @@ -122,7 +123,7 @@ class NsJail: # We'll consume STDOUT as long as the NsJail subprocess is running. while nsjail.poll() is None: - chars = nsjail.stdout.read(10_000) + chars = nsjail.stdout.read(READ_CHUNK_SIZE) output_size += sys.getsizeof(chars) output.append(chars) diff --git a/tests/test_nsjail.py b/tests/test_nsjail.py index 691eb5b..f4e678c 100644 --- a/tests/test_nsjail.py +++ b/tests/test_nsjail.py @@ -1,8 +1,11 @@ +import io import logging +import sys import unittest +import unittest.mock from textwrap import dedent -from snekbox.nsjail import MEM_MAX, NsJail +from snekbox.nsjail import MEM_MAX, NsJail, READ_CHUNK_SIZE, OUTPUT_MAX class NsJailTests(unittest.TestCase): @@ -183,3 +186,18 @@ class NsJailTests(unittest.TestCase): result = self.nsjail.python3(stdout_flood) self.assertEqual(result.returncode, 137) + + def test_large_output_is_truncated(self): + chunk = "a" * READ_CHUNK_SIZE + expected_chunks = OUTPUT_MAX // sys.getsizeof(chunk) + 1 + + nsjail_subprocess = unittest.mock.Mock() + + # Go 10 chunks over to make sure we exceed the limit + nsjail_subprocess.stdout = io.StringIO((expected_chunks + 10) * chunk) + nsjail_subprocess.returncode = -9 + nsjail_subprocess.poll.return_value = None + + returncode, output = self.nsjail._consume_stdout(nsjail_subprocess) + self.assertEqual(returncode, 137) + self.assertEqual(output, chunk * expected_chunks) -- cgit v1.2.3 From faa8fe73d8c197df79774c67211f144238717f58 Mon Sep 17 00:00:00 2001 From: Sebastiaan Zeeff Date: Sat, 21 Nov 2020 00:19:42 +0100 Subject: Move exit code conversion to main function --- snekbox/nsjail.py | 22 +++++++++++----------- tests/test_nsjail.py | 6 ++---- 2 files changed, 13 insertions(+), 15 deletions(-) (limited to 'tests') diff --git a/snekbox/nsjail.py b/snekbox/nsjail.py index 4dea3cb..f3b2be4 100644 --- a/snekbox/nsjail.py +++ b/snekbox/nsjail.py @@ -7,7 +7,7 @@ import textwrap from pathlib import Path from subprocess import CompletedProcess from tempfile import NamedTemporaryFile -from typing import Iterable, Tuple +from typing import Iterable from snekbox import DEBUG @@ -106,7 +106,7 @@ class NsJail: log.error(msg) @staticmethod - def _consume_stdout(nsjail: subprocess.Popen) -> Tuple[int, str]: + def _consume_stdout(nsjail: subprocess.Popen) -> str: """ Consume STDOUT, stopping when the output limit is reached or NsJail has exited. @@ -116,7 +116,7 @@ class NsJail: is asked to terminate with a SIGKILL. Once the subprocess has exited, either naturally or because it was terminated, - we return the exit code as an int and the output as a single string. + we rturn the output as a single string. """ output_size = 0 output = [] @@ -129,19 +129,14 @@ class NsJail: if output_size > OUTPUT_MAX: # Terminate the NsJail subprocess with SIGKILL. - log.info(f"Output exceeded the output limit, sending SIGKILL to NsJail.") + log.info("Output exceeded the output limit, sending SIGKILL to NsJail.") nsjail.kill() break # Ensure that we wait for the NsJail subprocess to terminate. nsjail.wait() - # When you send signal `N` to a subprocess to terminate it using Popen, it - # will return `-N` as its exit code. As we normally get `N + 128` back, we - # convert negative exit codes to the `N + 128` form. - returncode = -nsjail.returncode + 128 if nsjail.returncode < 0 else nsjail.returncode - - return returncode, "".join(output) + return "".join(output) def python3(self, code: str) -> CompletedProcess: """Execute Python 3 code in an isolated environment and return the completed process.""" @@ -175,7 +170,12 @@ class NsJail: except ValueError: return CompletedProcess(args, None, "ValueError: embedded null byte", None) - returncode, output = self._consume_stdout(nsjail) + output = self._consume_stdout(nsjail) + + # When you send signal `N` to a subprocess to terminate it using Popen, it + # will return `-N` as its exit code. As we normally get `N + 128` back, we + # convert negative exit codes to the `N + 128` form. + returncode = -nsjail.returncode + 128 if nsjail.returncode < 0 else nsjail.returncode log_lines = nsj_log.read().decode("utf-8").splitlines() if not log_lines and returncode == 255: diff --git a/tests/test_nsjail.py b/tests/test_nsjail.py index f4e678c..40c27f9 100644 --- a/tests/test_nsjail.py +++ b/tests/test_nsjail.py @@ -5,7 +5,7 @@ import unittest import unittest.mock from textwrap import dedent -from snekbox.nsjail import MEM_MAX, NsJail, READ_CHUNK_SIZE, OUTPUT_MAX +from snekbox.nsjail import MEM_MAX, NsJail, OUTPUT_MAX, READ_CHUNK_SIZE class NsJailTests(unittest.TestCase): @@ -195,9 +195,7 @@ class NsJailTests(unittest.TestCase): # Go 10 chunks over to make sure we exceed the limit nsjail_subprocess.stdout = io.StringIO((expected_chunks + 10) * chunk) - nsjail_subprocess.returncode = -9 nsjail_subprocess.poll.return_value = None - returncode, output = self.nsjail._consume_stdout(nsjail_subprocess) - self.assertEqual(returncode, 137) + output = self.nsjail._consume_stdout(nsjail_subprocess) self.assertEqual(output, chunk * expected_chunks) -- cgit v1.2.3