element-hq / element-web

A glossy Matrix collaboration client for the web.
https://element.io
GNU Affero General Public License v3.0
11.05k stars 1.97k forks source link

/query command does no input validation and can write invalid m.direct events #27630

Open Benjamin-L opened 3 months ago

Benjamin-L commented 3 months ago

Steps to reproduce

  1. Run a /query command with an argument that is not a valid userid. The case I used for my example was /query @somebody:matrix-1.daffodil.home here's my initial message contents

Note that it is easy to do this if you assume that /query works like the IRC /query command, and /query @user:origin a message will start a DM with @user:origin and send the message "a message".

Outcome

What did you expect?

I get an error message if the argument is not a valid userid. Ideally, it would also be legal to pass additional arguments for the first message contents, like IRC, but this is not fundamental to the bug.

What happened instead?

Element opens an empty DM room, and writes following to the global m.direct account data event:

{
  "content": {
    "@somebody:matrix-1.daffodil.home here's my initial message contents": [
      "!flT5eL4yxgCgm4rOKh:matrix-1.daffodil.home"
    ]
  },
  "type": "m.direct"
}

This is not a valid m.direct event according to the spec, because the type of m.direct contents should be {[User ID]: [string]}. This triggers a server bug in grapevine, where we are accepting arbitrary json for account data events but assuming that m.direct events in the db will be valid. This server bug should be fixed regardless of the element bug.

Operating system

NixOS (linux)

Browser information

Firefox 126.0

URL for webapp

develop.element.io

Application version

Element version: e3aee58a6d68-react-cea0b7c37ecf-js-25a7c9e14065 Crypto version: Rust SDK 0.7.1 (80a151e), Vodozemac 0.6.0

Homeserver

Grapevine 6fb9abed6218b3ced6c14cedfaca5c4094e88bc8

Will you send logs?

Yes

Xiretza commented 3 months ago

This permanently corrupts account data and is only fixable through dev tools, I don't think S-Minor is justified.

I'm fairly sure this is the reason why all my m.direct associations got lost at one point - some other client probably tripped over the invalid data stored by Element and cleared the account data. That was a fun afternoon of copy-pasting /converttodm.