Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/19915.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Don't validate signatures with unknown algorithms for master keys, and allow updates to signatures.
36 changes: 21 additions & 15 deletions synapse/handlers/e2e_keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -1272,21 +1272,27 @@ def _check_master_key_signature(
master_key_signature_list = []
sigs = signed_master_key["signatures"]
for signing_key_id, signature in sigs[user_id].items():
_, signing_device_id = signing_key_id.split(":", 1)
if (
signing_device_id not in devices
or signing_key_id not in devices[signing_device_id]["keys"]
):
# signed by an unknown device, or the
# device does not have the key
raise SynapseError(400, "Invalid signature", Codes.INVALID_SIGNATURE)

# get the key and check the signature
pubkey = devices[signing_device_id]["keys"][signing_key_id]
verify_key = decode_verify_key_bytes(signing_key_id, decode_base64(pubkey))
_check_device_signature(
user_id, verify_key, signed_master_key, stored_master_key
)
algorithm, signing_device_id = signing_key_id.split(":", 1)
# we only check the signature for known algorithms
if algorithm == "ed25519":
if (
signing_device_id not in devices
or signing_key_id not in devices[signing_device_id]["keys"]
):
# signed by an unknown device, or the
# device does not have the key
raise SynapseError(
400, "Invalid signature", Codes.INVALID_SIGNATURE
)

# get the key and check the signature
pubkey = devices[signing_device_id]["keys"][signing_key_id]
verify_key = decode_verify_key_bytes(
signing_key_id, decode_base64(pubkey)
)
_check_device_signature(
user_id, verify_key, signed_master_key, stored_master_key
)

master_key_signature_list.append(
SignatureListItem(signing_key_id, user_id, master_key_id, signature)
Expand Down
24 changes: 14 additions & 10 deletions synapse/storage/databases/main/end_to_end_keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,15 @@ def __init__(
columns=("user_id", "device_id", "algorithm", "ts_added_ms"),
)

self.db_pool.updates.register_background_index_update(
update_name="e2e_cross_signing_signatures_add_key_id_to_index",
index_name="e2e_cross_signing_signatures3_idx",
table="e2e_cross_signing_signatures",
columns=("user_id", "target_user_id", "target_device_id", "key_id"),
unique=True,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what's the proper way to handle the case where we end up with duplicate values on those columns. I think that we shouldn't have any, but we should probably try to be safe.

replaces_index="e2e_cross_signing_signatures2_idx",
)


class EndToEndKeyWorkerStore(EndToEndKeyBackgroundStore, CacheInvalidationWorkerStore):
def __init__(
Expand Down Expand Up @@ -1821,26 +1830,21 @@ def _store_e2e_cross_signing_signatures(
txn: LoggingTransaction,
signatures: "Iterable[SignatureListItem]",
) -> None:
self.db_pool.simple_insert_many_txn(
self.db_pool.simple_upsert_many_txn(
txn,
"e2e_cross_signing_signatures",
keys=(
"user_id",
"key_id",
"target_user_id",
"target_device_id",
"signature",
),
values=[
key_names=("user_id", "key_id", "target_user_id", "target_device_id"),
key_values=[
(
user_id,
item.signing_key_id,
item.target_user_id,
item.target_device_id,
item.signature,
)
for item in signatures
],
value_names=("signature",),
value_values=[(item.signature,) for item in signatures],
)

to_invalidate = [
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
--
-- This file is licensed under the Affero General Public License (AGPL) version 3.
--
-- Copyright (C) 2026 New Vector, Ltd
--
-- This program is free software: you can redistribute it and/or modify
-- it under the terms of the GNU Affero General Public License as
-- published by the Free Software Foundation, either version 3 of the
-- License, or (at your option) any later version.
--
-- See the GNU Affero General Public License for more details:
-- <https://www.gnu.org/licenses/agpl-3.0.html>.

-- Adds the `key_id` to the e2e_cross_signing_signatures index, since the
-- ("user_id", "key_id", "target_user_id", "target_device_id") should be
-- unique.
INSERT INTO background_updates (ordering, update_name, progress_json) VALUES
(9205, 'e2e_cross_signing_signatures_add_key_id_to_index', '{}');
98 changes: 97 additions & 1 deletion tests/handlers/test_e2e_keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
#
#
import time
from typing import Iterable
from typing import Any, Iterable
from unittest import mock

from parameterized import parameterized
Expand Down Expand Up @@ -817,6 +817,102 @@ def test_reupload_signatures(self) -> None:
self.assertDictEqual(devices["device_keys"][local_user]["abc"], device_key_1)
self.assertDictEqual(devices["device_keys"][local_user]["def"], device_key_2)

def test_update_signature_master_key(self) -> None:
"""should be able to update a signature on the Master signing key with an unknown algorithm"""
local_user = "@boris:" + self.hs.hostname
master_key: dict[str, Any] = {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type annotation seems overly broad to me, so suggestions welcome. Without it, mypy complains when I try to assign to master_key["signatures"][local_user]["unknown:abcdefg"] below. (On the other hand, this is just a test, so I'm not overly concerned about getting the types just right.)

# private key: HvQBbU+hc2Zr+JP1sE0XwBe1pfZZEYtJNPJLZJtS+F8
"user_id": local_user,
"usage": ["master"],
"keys": {
"ed25519:EmkqvokUn8p+vQAGZitOk4PWjp7Ukp3txV2TbMPEiBQ": "EmkqvokUn8p+vQAGZitOk4PWjp7Ukp3txV2TbMPEiBQ"
},
"signatures": {local_user: {"unknown:abcdefg": "abcdefg"}},
}
self_signing_key = {
# private key: 2lonYOM6xYKdEsO+6KrC766xBcHnYnim1x/4LFGF8B0
"user_id": local_user,
"usage": ["self_signing"],
"keys": {
"ed25519:nqOvzeuGWT/sRx3h7+MHoInYj3Uk2LD/unI9kDYcHwk": "nqOvzeuGWT/sRx3h7+MHoInYj3Uk2LD/unI9kDYcHwk"
},
}
master_signing_key = key.decode_signing_key_base64(
"ed25519",
"EmkqvokUn8p+vQAGZitOk4PWjp7Ukp3txV2TbMPEiBQ",
"HvQBbU+hc2Zr+JP1sE0XwBe1pfZZEYtJNPJLZJtS+F8",
)
sign.sign_json(self_signing_key, local_user, master_signing_key)
self.get_success(
self.handler.upload_signing_keys_for_user(
local_user,
{"master_key": master_key, "self_signing_key": self_signing_key},
)
)

device_key: JsonDict = {
"user_id": local_user,
"device_id": "abc",
"algorithms": [
"m.olm.curve25519-aes-sha2",
RoomEncryptionAlgorithms.MEGOLM_V1_AES_SHA2,
],
"keys": {
"ed25519:abc": "base64+ed25519+key",
"curve25519:abc": "base64+curve25519+key",
},
"signatures": {local_user: {"ed25519:abc": "base64+signature"}},
}
self.get_success(
self.handler.upload_keys_for_user(
local_user, "abc", {"device_keys": device_key}
)
)

# update the signature and upload it
master_key["signatures"][local_user]["unknown:abcdefg"] = "ABCDEFG"
self.get_success(
self.handler.upload_signatures_for_device_keys(
local_user,
{
local_user: {
"EmkqvokUn8p+vQAGZitOk4PWjp7Ukp3txV2TbMPEiBQ": master_key
}
},
)
)

devices = self.get_success(
self.handler.query_devices(
{"device_keys": {local_user: []}}, 0, local_user, "device123"
)
)
if "unsigned" in devices["master_keys"][local_user]:
del devices["master_keys"][local_user]["unsigned"]
self.assertDictEqual(devices["master_keys"][local_user], master_key)

# update the signature and upload it
master_key["signatures"][local_user]["unknown:abcdefg"] = "AbCdEfG"
self.get_success(
self.handler.upload_signatures_for_device_keys(
local_user,
{
local_user: {
"EmkqvokUn8p+vQAGZitOk4PWjp7Ukp3txV2TbMPEiBQ": master_key
}
},
)
)

devices = self.get_success(
self.handler.query_devices(
{"device_keys": {local_user: []}}, 0, local_user, "device123"
)
)
if "unsigned" in devices["master_keys"][local_user]:
del devices["master_keys"][local_user]["unsigned"]
self.assertDictEqual(devices["master_keys"][local_user], master_key)

def test_self_signing_key_doesnt_show_up_as_device(self) -> None:
"""signing keys should be hidden when fetching a user's devices"""
local_user = "@boris:" + self.hs.hostname
Expand Down
Loading