Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Synapse accepts to_device messages to hidden devices #9348

Closed
anoadragon453 opened this issue Feb 8, 2021 · 5 comments · Fixed by #10097
Closed

Synapse accepts to_device messages to hidden devices #9348

anoadragon453 opened this issue Feb 8, 2021 · 5 comments · Fixed by #10097
Labels
S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@anoadragon453
Copy link
Member

anoadragon453 commented Feb 8, 2021

As part of cross-signing, devices for the master, self-signing and user-signing cross-signing keys are created and stored in the devices table.

These devices should not be getting to_device messages sent to them, as they will never be read by any client. It turns out that it is possible to have to_device messages aimed at them in the device_inbox table through the following:

  1. Clients can directly send to_device messages to one of your cross-signing public keys. These happen to be the device IDs of these hidden devices. Synapse doesn't condition on the device being hidden, and so will accept them. It should be doing so here:

rows = self.db_pool.simple_select_many_txn(
txn,
table="devices",
keyvalues={"user_id": user_id},
column="device_id",
iterable=devices,
retcols=("device_id",),
)

  1. Clients often send to_device messages to all devices of a user, by specifying * as the target. Synapse will then create a to_device message for all devices of a user, including hidden ones:

# Handle wildcard device_ids.
devices = self.db_pool.simple_select_onecol_txn(
txn,
table="devices",
keyvalues={"user_id": user_id},
retcol="device_id",
)
message_json = json_encoder.encode(messages_by_device["*"])
for device_id in devices:
# Add the message for all devices for this user on this
# server.
messages_json_for_user[device_id] = message_json

These messages were never getting read, and thus piling up in the database forever. In my testing they didn't seem to be using that much space however. Upon removing them for all users on my homeserver and vacuuming the table, the size of device_inbox only went from 3420MB to 3410MB. Edit: I don't think this is accurate. After deleting over half the table in a different cleanup and REINDEX+VACUUMing I also only dropped after 10MB.

Initially presumed as a client-side issue, before we discovered Synapse was at fault: element-hq/element-web#15638

Related: #3599

@anoadragon453 anoadragon453 added S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. labels Feb 8, 2021
@anoadragon453 anoadragon453 changed the title Synapse accept to_device messages to hidden devices Synapse accepts to_device messages to hidden devices Feb 8, 2021
@jryans
Copy link
Contributor

jryans commented Feb 8, 2021

Related to #3656

@anoadragon453
Copy link
Member Author

I'd like to see a solution which:

  • Stops adding messages for hidden devices when the user specifies *
  • Silently ignore messages directed at hidden devices (just as we do today for non-existent ones)
  • Removes existing messages that are directed at hidden devices to cut down on database size. I ended up using:
delete from device_inbox where device_id in (select device_id from devices where hidden = true);

@DMRobertson
Copy link
Contributor

Should this be closed? AFAICS #11199 will delete the stuff we don't want on Synapse start up, but it won't address the first two bullets in @anoadragon453's #9348 (comment).

@anoadragon453
Copy link
Member Author

@DMRobertson #10097 should address the first two points, and has been merged.

I'm not sure if it's worth keeping this open for #11199, unless we're specifically tracking this issue. The third bullet point was mostly mentioned as a nice-to-have.

@DMRobertson
Copy link
Contributor

Ahh, sorry. I confused #10097 and #11199. I hadn't spotted the former; I thought the latter was just merged. Agreed entirely.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants