Add Semgrep in CircleCI (#1910)

Related tiny-pilot/tinypilot-pro#1595

This PR runs [Semgrep](https://github.com/semgrep/semgrep), a Static
Application Security Testing (SAST) tool, on our code in CircleCI.

This PR also addresses Semgrep's [9 initial
findings](https://app.circleci.com/pipelines/github/tiny-pilot/tinypilot/4840/workflows/b0fb5361-fcb6-462e-bcbe-53dc42e16af9/jobs/36859?invite=true#step-104-1533_19).

Notes:
1. Seeing as Semgrep runs SAST on multiple languages (not just Python),
we run it via a new `dev-scripts/check-security` script.
2. We only use the free version of Semgrep (i.e., [local
scans](https://semgrep.dev/docs/getting-started/cli)). Otherwise,
Semgrep want us to login and pay for additional security rules.
3. Semgrep flagged `janus.js` as allowing any origin (i.e., `*`) to
listen for
[`postMessage`](https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage#targetorigin)
messages. Looking at the Janus code, `postMessage` is only used for an
(outdated) [Janus WebRTC Screensharing chrome
extension](https://chromewebstore.google.com/detail/janus-webrtc-screensharin/hapfgfdkleiggjjpfpenajgdnfckjpaj?hl=fil&gl=001).
So I've just removed the origin wildcard:
-
https://github.com/tiny-pilot/tinypilot/blob/0276b1358f3fc976a27aa386a31e1966212dff03/app/static/third-party/janus-gateway/1.3.2/janus.js#L73

<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1910"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>

Author: jdeanwallace

.circleci/config.yml

@@ -149,6 +149,23 @@ jobs:
       - run:
           name: Run build script
           command: ./dev-scripts/check-javascript
+  check_security:
+    executor: python
+    steps:
+      - checkout
+      - run:
+          name: Create virtual environment
+          command: python3 -m venv venv
+      - run:
+          name: Install requirements
+          command: |
+            . venv/bin/activate
+            pip install --requirement dev_requirements.txt
+      - run:
+          name: Run Static Application Security Testing (SAST) on files
+          command: |
+            . venv/bin/activate
+            ./dev-scripts/check-security
   run_e2e_tests:
     docker:
       # To run tests against the dev server, Playwright needs a CircleCI image
@@ -303,6 +320,7 @@ workflows:
       - decode_edid
       - check_python
       - check_javascript
+      - check_security
       - run_e2e_tests
       - build_debian_package:
           requires:

CONTRIBUTING.md

@@ -601,3 +601,29 @@ const confirmDeleteButton = document.querySelector("#delete .confirm-btn");
   - You are responsible for fixing formatting and tests to ensure that your code passes build checks in CI.
 
 I try to review all PRs within one business day. If you've been waiting longer than this, feel free to comment on the PR to verify that it's on my radar.
+
+## Quirks of this project
+
+### Patch security in `janus.js`
+
+Semgrep flagged `janus.js` as allowing any origin (i.e., `*`) to listen for [`postMessage`](https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage#targetorigin) messages: [`javascript.browser.security.wildcard-postmessage-configuration.wildcard-postmessage-configuration`](https://semgrep.dev/r?q=javascript.browser.security.wildcard-postmessage-configuration.wildcard-postmessage-configuration)
+
+Janus only uses `postMessage` for a seemingly outdated [Janus WebRTC Screensharing chrome extension](https://chromewebstore.google.com/detail/janus-webrtc-screensharin/hapfgfdkleiggjjpfpenajgdnfckjpaj?hl=fil&gl=001), which we don't use.
+
+To resolve this issue, we simply removed the wildcard origin:
+
+```diff
+--- app/static/third-party/janus-gateway/1.3.2/janus.js
++++ app/static/third-party/janus-gateway/1.3.2/janus.js
+@@ -70,7 +70,7 @@ var Janus = (function (factory) {
+ 				return callback(error);
+ 			}, 1000);
+ 			this.cache[pending] = callback;
+-			window.postMessage({ type: 'janusGetScreen', id: pending }, '*');
++			window.postMessage({ type: 'janusGetScreen', id: pending });
+ 		},
+ 		init: function () {
+ 			let cache = {};
+```
+
+Note that this patch should be reapplied whenever we update `janus.js`.

app/db/store.py

@@ -130,7 +130,9 @@ def create_or_open(db_path):
             transaction.executescript('BEGIN; ' + _MIGRATIONS[i])
             # SQlite doesn’t allow prepared statements for PRAGMA queries.
             # That’s okay here, since we know our query is safe.
-            transaction.execute(f'PRAGMA user_version={i+1}')
+            # pylint: disable=line-too-long
+            transaction.execute(  # nosemgrep: formatted-sql-query, sqlalchemy-execute-raw-query
+                f'PRAGMA user_version={i+1}')
         logger.info('Applied migration, counter is now at %d', i + 1)
 
     return connection

app/license_notice.py

@@ -317,8 +317,10 @@ def _get_project_metadata(project_name):
     return None
 
 
-def _make_plaintext_response(response_body):
-    response = flask.make_response(response_body, 200)
+def _make_plaintext_response(text):
+    # The content is rendered as plain text which mitigates the risk of XSS.
+    response = flask.make_response(  # nosemgrep: make-response-with-unknown-content
+        text, 200)
     response.mimetype = 'text/plain'
     return response
 

app/secret_key.py

@@ -75,7 +75,9 @@ def _create():
     Raises:
         IOError: If an error occured while accessing the secret key file.
     """
-    logger.info('Creating new flask secret key at %s', _SECRET_KEY_FILE)
+    # We're only logging the secret key file path, which isn't sensitive.
+    logger.info(  # nosemgrep: python-logger-credential-disclosure
+        'Creating new flask secret key at %s', _SECRET_KEY_FILE)
     secret_key = os.urandom(_SECRET_KEY_BYTE_LENGTH)
     with atomic_file.create(_SECRET_KEY_FILE,
                             chmod_mode=_SECRET_KEY_FILE_PERMS) as file:

app/secret_key_test.py

@@ -27,7 +27,9 @@ def test_get_or_create_will_recreate_if_file_has_invalid_perms(self):
         with tempfile.NamedTemporaryFile() as mock_secret_key_file:
             mock_secret_key_file.write(b'0' * 32)
             mock_secret_key_file.flush()
-            os.chmod(mock_secret_key_file.name, 0o700)
+            # We're testing that insecure file permissions aren't accepted.
+            os.chmod(  # nosemgrep: insecure-file-permissions
+                mock_secret_key_file.name, 0o700)
 
             with mock.patch.object(secret_key, '_SECRET_KEY_FILE',
                                    mock_secret_key_file.name):

app/static/third-party/janus-gateway/1.3.2/janus.js

@@ -70,7 +70,7 @@ var Janus = (function (factory) {
 				return callback(error);
 			}, 1000);
 			this.cache[pending] = callback;
-			window.postMessage({ type: 'janusGetScreen', id: pending }, '*');
+			window.postMessage({ type: 'janusGetScreen', id: pending });
 		},
 		init: function () {
 			let cache = {};

app/version.py

@@ -84,7 +84,8 @@ def latest_version():
     """
     try:
         # The URL is trusted because it's not based on user input.
-        with urllib.request.urlopen(  # noqa: S310
+        # pylint: disable=line-too-long
+        with urllib.request.urlopen(  # noqa: S310 # nosemgrep: dynamic-urllib-use-detected
                 f'{env.GATEKEEPER_BASE_URL}/community/available-update',
                 timeout=10) as response:
             response_bytes = response.read()

debian-pkg/opt/tinypilot-privileged/scripts/update

@@ -9,6 +9,8 @@ set -u
 # Exit on first error.
 set -e
 
+# The URL is trusted.
+# nosemgrep: curl-pipe-bash
 curl \
   --silent \
   --show-error \

dev-scripts/check-all

@@ -18,6 +18,7 @@ set -u
 ./dev-scripts/check-javascript
 ./dev-scripts/check-privilege-guard
 ./dev-scripts/check-python
+./dev-scripts/check-security
 ./dev-scripts/check-sql
 ./dev-scripts/check-style
 ./dev-scripts/check-trailing-whitespace