Validate the body of requests to /keys/upload

changelog.d/17097.misc

@@ -0,0 +1 @@
+Extend validation of uploaded device keys.

synapse/handlers/e2e_keys.py

@@ -57,7 +57,6 @@
 
 logger = logging.getLogger(__name__)
 
-
 ONE_TIME_KEY_UPLOAD = "one_time_key_upload_lock"
 
 
@@ -847,14 +846,22 @@ async def upload_keys_for_user(
         """
         time_now = self.clock.time_msec()
 
-        # TODO: Validate the JSON to make sure it has the right keys.
         device_keys = keys.get("device_keys", None)
         if device_keys:
+            log_kv(
+                {
+                    "message": "Updating device_keys for user.",
+                    "user_id": user_id,
+                    "device_id": device_id,
+                }
+            )
             await self.upload_device_keys_for_user(
                 user_id=user_id,
                 device_id=device_id,
                 keys={"device_keys": device_keys},
             )
+        else:
+            log_kv({"message": "Did not update device_keys", "reason": "not a dict"})
 
         one_time_keys = keys.get("one_time_keys", None)
         if one_time_keys:
@@ -872,8 +879,9 @@ async def upload_keys_for_user(
             log_kv(
                 {"message": "Did not update one_time_keys", "reason": "no keys given"}
             )
+
         fallback_keys = keys.get("fallback_keys")
-        if fallback_keys and isinstance(fallback_keys, dict):
+        if fallback_keys:
             log_kv(
                 {
                     "message": "Updating fallback_keys for device.",
@@ -882,8 +890,6 @@ async def upload_keys_for_user(
                 }
             )
             await self.store.set_e2e_fallback_keys(user_id, device_id, fallback_keys)
-        elif fallback_keys:
-            log_kv({"message": "Did not update fallback_keys", "reason": "not a dict"})
         else:
             log_kv(
                 {"message": "Did not update fallback_keys", "reason": "no keys given"}

synapse/rest/client/keys.py

@@ -23,10 +23,19 @@
 import logging
 import re
 from collections import Counter
-from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple
+from http import HTTPStatus
+from typing import TYPE_CHECKING, Any, Dict, List, Mapping, Optional, Tuple, Union
 
+from typing_extensions import Self
+
+from synapse._pydantic_compat import (
+    StrictBool,
+    StrictStr,
+    validator,
+)
 from synapse.api.auth.mas import MasDelegatedAuth
 from synapse.api.errors import (
+    Codes,
     InteractiveAuthIncompleteError,
     InvalidAPICallError,
     SynapseError,
@@ -37,11 +46,13 @@
     parse_integer,
     parse_json_object_from_request,
     parse_string,
+    validate_json_object,
 )
 from synapse.http.site import SynapseRequest
 from synapse.logging.opentracing import log_kv, set_tag
 from synapse.rest.client._base import client_patterns, interactive_auth_handler
 from synapse.types import JsonDict, StreamToken
+from synapse.types.rest import RequestBodyModel
 from synapse.util.cancellation import cancellable
 
 if TYPE_CHECKING:
@@ -59,7 +70,6 @@ class KeyUploadServlet(RestServlet):
         "device_keys": {
             "user_id": "<user_id>",
             "device_id": "<device_id>",
-            "valid_until_ts": <millisecond_timestamp>,
             "algorithms": [
                 "m.olm.curve25519-aes-sha2",
             ]
@@ -111,12 +121,123 @@ def __init__(self, hs: "HomeServer"):
         self._clock = hs.get_clock()
         self._store = hs.get_datastores().main
 
+    class KeyUploadRequestBody(RequestBodyModel):
+        """
+        The body of a `POST /_matrix/client/v3/keys/upload` request.
+
+        Based on https://spec.matrix.org/v1.16/client-server-api/#post_matrixclientv3keysupload.
+        """
+
+        class DeviceKeys(RequestBodyModel):
+            algorithms: List[StrictStr]
+            """The encryption algorithms supported by this device."""
+
+            device_id: StrictStr
+            """The ID of the device these keys belong to. Must match the device ID used when logging in."""
+
+            keys: Mapping[StrictStr, StrictStr]
+            """
+            Public identity keys. The names of the properties should be in the
+            format `<algorithm>:<device_id>`. The keys themselves should be encoded as
+            specified by the key algorithm.
+            """
+
+            signatures: Mapping[StrictStr, Mapping[StrictStr, StrictStr]]
+            """Signatures for the device key object. A map from user ID, to a map from "<algorithm>:<device_id>" to the signature."""
+
+            user_id: StrictStr
+            """The ID of the user the device belongs to. Must match the user ID used when logging in."""
+
+        class KeyObject(RequestBodyModel):
+            key: StrictStr
+            """The key, encoded using unpadded base64."""
+
+            fallback: Optional[StrictBool] = False
+            """Whether this is a fallback key. Only used when handling fallback keys."""
+
+            signatures: Mapping[StrictStr, Mapping[StrictStr, StrictStr]]
+            """Signature for the device. Mapped from user ID to another map of key signing identifier to the signature itself.
+
+            See the following for more detail: https://spec.matrix.org/v1.16/appendices/#signing-details
+            """
+
+        device_keys: Optional[DeviceKeys] = None
+        """Identity keys for the device. May be absent if no new identity keys are required."""
+
+        fallback_keys: Optional[Mapping[StrictStr, Union[StrictStr, KeyObject]]]
+        """
+        The public key which should be used if the device's one-time keys are
+        exhausted. The fallback key is not deleted once used, but should be
+        replaced when additional one-time keys are being uploaded. The server
+        will notify the client of the fallback key being used through `/sync`.
+
+        There can only be at most one key per algorithm uploaded, and the server
+        will only persist one key per algorithm.
+
+        When uploading a signed key, an additional fallback: true key should be
+        included to denote that the key is a fallback key.
+
+        May be absent if a new fallback key is not required.
+        """
+
+        @validator("fallback_keys", pre=True)
+        def validate_fallback_keys(cls: Self, v: Any) -> Any:
+            if v is None:
+                return v
+            if not isinstance(v, dict):
+                raise TypeError("fallback_keys must be a mapping")
+
+            for k in v.keys():
+                if not len(k.split(":")) == 2:
+                    raise SynapseError(
+                        code=HTTPStatus.BAD_REQUEST,
+                        errcode=Codes.BAD_JSON,
+                        msg=f"Invalid fallback_keys key {k!r}. "
+                        'Expected "<algorithm>:<device_id>".',
+                    )
+            return v
+
+        one_time_keys: Optional[Mapping[StrictStr, Union[StrictStr, KeyObject]]] = None
+        """
+        One-time public keys for "pre-key" messages. The names of the properties
+        should be in the format `<algorithm>:<key_id>`.
+
+        The format of the key is determined by the key algorithm, see:
+        https://spec.matrix.org/v1.16/client-server-api/#key-algorithms.
+        """
+
+        @validator("one_time_keys", pre=True)
+        def validate_one_time_keys(cls: Self, v: Any) -> Any:
+            if v is None:
+                return v
+            if not isinstance(v, dict):
+                raise TypeError("one_time_keys must be a mapping")
+
+            for k, _ in v.items():
+                if not len(k.split(":")) == 2:
+                    raise SynapseError(
+                        code=HTTPStatus.BAD_REQUEST,
+                        errcode=Codes.BAD_JSON,
+                        msg=f"Invalid one_time_keys key {k!r}. "
+                        'Expected "<algorithm>:<device_id>".',
+                    )
+            return v
+
     async def on_POST(
         self, request: SynapseRequest, device_id: Optional[str]
     ) -> Tuple[int, JsonDict]:
         requester = await self.auth.get_user_by_req(request, allow_guest=True)
         user_id = requester.user.to_string()
+
+        # Parse the request body. Validate separately, as the handler expects a
+        # plain dict, rather than any parsed object.
+        #
+        # Note: It would be nice to work with a parsed object, but the handler
+        # needs to encode portions of the request body as canonical JSON before
+        # storing the result in the DB. There's little point in converted to a
+        # parsed object and then back to a dict.
         body = parse_json_object_from_request(request)
+        validate_json_object(body, self.KeyUploadRequestBody)
 
         if device_id is not None:
             # Providing the device_id should only be done for setting keys
@@ -149,8 +270,31 @@ async def on_POST(
                 400, "To upload keys, you must pass device_id when authenticating"
             )
 
+        if "device_keys" in body:
+            # Validate the provided `user_id` and `device_id` fields in
+            # `device_keys` match that of the requesting user. We can't do
+            # this directly in the pydantic model as we don't have access
+            # to the requester yet.
+            #
+            # TODO: We could use ValidationInfo when we switch to Pydantic v2.
+            # https://docs.pydantic.dev/latest/concepts/validators/#validation-info
+            if body["device_keys"]["user_id"] != user_id:
+                raise SynapseError(
+                    code=HTTPStatus.BAD_REQUEST,
+                    errcode=Codes.BAD_JSON,
+                    msg="Provided `user_id` in `device_keys` does not match that of the authenticated user",
+                )
+            if body["device_keys"]["device_id"] != device_id:
+                raise SynapseError(
+                    code=HTTPStatus.BAD_REQUEST,
+                    errcode=Codes.BAD_JSON,
+                    msg="Provided `device_id` in `device_keys` does not match that of the authenticated user device",
+                )
+
         result = await self.e2e_keys_handler.upload_keys_for_user(
-            user_id=user_id, device_id=device_id, keys=body
+            user_id=user_id,
+            device_id=device_id,
+            keys=body,
         )
 
         return 200, result

tests/rest/client/test_keys.py

@@ -40,6 +40,127 @@
 from tests.utils import HAS_AUTHLIB
 
 
+class KeyUploadTestCase(unittest.HomeserverTestCase):
+    servlets = [
+        keys.register_servlets,
+        admin.register_servlets_for_client_rest_resource,
+        login.register_servlets,
+    ]
+
+    def test_upload_keys_fails_on_invalid_structure(self) -> None:
+        """Check that we validate the structure of keys upon upload.
+
+        Regression test for https://github.com/element-hq/synapse/pull/17097
+        """
+        self.register_user("alice", "wonderland")
+        alice_token = self.login("alice", "wonderland")
+
+        channel = self.make_request(
+            "POST",
+            "/_matrix/client/v3/keys/upload",
+            {
+                # Error: device_keys must be a dict
+                "device_keys": ["some", "stuff", "weewoo"]
+            },
+            alice_token,
+        )
+        self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result)
+        self.assertEqual(
+            channel.json_body["errcode"],
+            Codes.BAD_JSON,
+            channel.result,
+        )
+
+        channel = self.make_request(
+            "POST",
+            "/_matrix/client/v3/keys/upload",
+            {
+                # Error: properties of fallback_keys must be in the form `<algorithm>:<device_id>`
+                "fallback_keys": {"invalid_key": "signature_base64"}
+            },
+            alice_token,
+        )
+        self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result)
+        self.assertEqual(
+            channel.json_body["errcode"],
+            Codes.BAD_JSON,
+            channel.result,
+        )
+
+        channel = self.make_request(
+            "POST",
+            "/_matrix/client/v3/keys/upload",
+            {
+                # Same as above, but for one_time_keys
+                "one_time_keys": {"invalid_key": "signature_base64"}
+            },
+            alice_token,
+        )
+        self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result)
+        self.assertEqual(
+            channel.json_body["errcode"],
+            Codes.BAD_JSON,
+            channel.result,
+        )
+
+    def test_upload_keys_fails_on_invalid_user_id_or_device_id(self) -> None:
+        """
+        Validate that the requesting user is uploading their own keys and nobody
+        else's.
+        """
+        device_id = "DEVICE_ID"
+        alice_user_id = self.register_user("alice", "wonderland")
+        alice_token = self.login("alice", "wonderland", device_id=device_id)
+
+        channel = self.make_request(
+            "POST",
+            "/_matrix/client/v3/keys/upload",
+            {
+                "device_keys": {
+                    # Included `user_id` does not match requesting user.
+                    "user_id": "@unknown_user:test",
+                    "device_id": device_id,
+                    "algorithms": ["m.olm.curve25519-aes-sha2"],
+                    "keys": {
+                        f"ed25519:{device_id}": "publickey",
+                    },
+                    "signatures": {},
+                }
+            },
+            alice_token,
+        )
+        self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result)
+        self.assertEqual(
+            channel.json_body["errcode"],
+            Codes.BAD_JSON,
+            channel.result,
+        )
+
+        channel = self.make_request(
+            "POST",
+            "/_matrix/client/v3/keys/upload",
+            {
+                "device_keys": {
+                    "user_id": alice_user_id,
+                    # Included `device_id` does not match requesting user's.
+                    "device_id": "UNKNOWN_DEVICE_ID",
+                    "algorithms": ["m.olm.curve25519-aes-sha2"],
+                    "keys": {
+                        f"ed25519:{device_id}": "publickey",
+                    },
+                    "signatures": {},
+                }
+            },
+            alice_token,
+        )
+        self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result)
+        self.assertEqual(
+            channel.json_body["errcode"],
+            Codes.BAD_JSON,
+            channel.result,
+        )
+
+
 class KeyQueryTestCase(unittest.HomeserverTestCase):
     servlets = [
         keys.register_servlets,