sunnylink: block remote modification of SSH key parameters (#1591)

* feat: add blocked parameter names

* add unit test to validate

* test: use cached method

* move it out

---------

Co-authored-by: Jason Wen <haibin.wen3@gmail.com>

sunnypilot/sunnylink/athena/sunnylinkd.py

@@ -42,6 +42,12 @@ METADATA_PATH = os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(__f
 
 params = Params()
 
+# Parameters that should never be remotely modified for security reasons
+BLOCKED_PARAMS = {
+  "GithubUsername",  # Could grant SSH access
+  "GithubSshKeys",   # Direct SSH key injection
+}
+
 
 def handle_long_poll(ws: WebSocket, exit_event: threading.Event | None) -> None:
   cloudlog.info("sunnylinkd.handle_long_poll started")
@@ -248,6 +254,11 @@ def getParams(params_keys: list[str], compression: bool = False) -> str | dict[s
 @dispatcher.add_method
 def saveParams(params_to_update: dict[str, str], compression: bool = False) -> None:
   for key, value in params_to_update.items():
+    # disallow modifications to blocked parameters
+    if key in BLOCKED_PARAMS:
+      cloudlog.warning(f"sunnylinkd.saveParams.blocked: Attempted to modify blocked parameter '{key}'")
+      continue
+
     try:
       save_param_from_base64_encoded_string(key, value, compression)
     except Exception as e:

sunnypilot/sunnylink/athena/tests/test_sunnylinkd.py

@@ -0,0 +1,59 @@
+"""
+Copyright (c) 2021-, Haibin Wen, sunnypilot, and a number of other contributors.
+
+This file is part of sunnypilot and is licensed under the MIT License.
+See the LICENSE.md file in the root directory for more details.
+"""
+from openpilot.sunnypilot.sunnylink.athena import sunnylinkd
+
+
+class TestSunnylinkdMethods:
+  def setup_method(self):
+    self.saved_params = []
+
+    self.original_save = sunnylinkd.save_param_from_base64_encoded_string
+
+    def mock_save_param(key, value, compression=False):
+      self.saved_params.append((key, value, compression))
+
+    sunnylinkd.save_param_from_base64_encoded_string = mock_save_param
+
+  def teardown_method(self):
+    sunnylinkd.save_param_from_base64_encoded_string = self.original_save
+
+  def test_saveParams_blocked(self):
+    blocked_params = {
+      "GithubUsername": "attacker",
+      "GithubSshKeys": "ssh-rsa attacker_key",
+    }
+
+    sunnylinkd.saveParams(blocked_params)
+
+    assert len(self.saved_params) == 0
+
+  def test_saveParams_allowed(self):
+    allowed_params = {
+      "SpeedLimitOffset": "5",
+      "MyCustomParam": "123"
+    }
+
+    sunnylinkd.saveParams(allowed_params)
+
+    # verify content
+    assert len(self.saved_params) == 2
+    keys_saved = [p[0] for p in self.saved_params]
+    assert "SpeedLimitOffset" in keys_saved
+    assert "MyCustomParam" in keys_saved
+
+  def test_saveParams_mixed(self):
+    mixed_params = {
+      "GithubUsername": "attacker",
+      "SpeedLimitOffset": "10"
+    }
+
+    sunnylinkd.saveParams(mixed_params)
+
+    # should save allowed one
+    assert len(self.saved_params) == 1
+    assert self.saved_params[0][0] == "SpeedLimitOffset"
+    assert self.saved_params[0][1] == "10"