element-hq / element-hq/synapse

Improve API/event validation in synapse

Open
#8,445 0 comments 0 reactions 0 assignees View on GitHub
A-Validation T-Task
Dominant language
Python
Stars
4.6k
Forks
600
Avg merge
5d 22h
Merged PRs (30d)
51

Description

This issue has been migrated from [#8445](https://github.com/matrix-org/synapse/issues/8445).

---

# Background

We've recently encountered a number of bugs in which malformed (or at least, unexpected) data has caused Synapse or clients to misbehave in some way. These bugs stem from the fact that, faced with a given datastructure, you cannot rely on it having the expected format. For example:
- What happens if the `displayname` in an `m.room.member` event is not a string (#8220, #8340)?
- What happens if an event object is missing its `origin` field (#8319)?

Such bugs are disruptive, and in extreme cases could progress beyond "denial of service" to "security threat", and it might be possible to avoid them by validating data at the point of entry to Synapse. We've recently discussed this in some depth within the core Synapse team; this issue serves to record some of our thoughts on the topic, including promising areas for further development.

# Introduction

There are actually multiple reasons why it would be useful to improve validation of data within Synapse. These include:

* As above, avoiding potential bugs
* Simplifying code by removing `isinstance` checks everywhere
* Forcing clients to follow the specification rather than using "whatever works in Synapse", thus improving compatibility with other server implementations which *do* enforce the spec (or conversely: not forcing other implementations to grandfather in Synapse's misfeatures for compatibility with clients)
* Making synapse return 400 errors rather than "Internal Server Errors" if clients pass invalid parameters would:
* Give better feedback to client developers: the 500 error gives the developer little information about their error.)
* Allow server administrators to distinguish between "my server is having problems" and "somebody passed invalid parameters to my server"
* Allow load-balancers to distinguish between "this process is having problems" and "somebody passed invalid parameters". Notably, Cloudflare will stop sending requests to an origin which returns too many 500 errors.

There are multiple related, but different, things we mean when we talk about validation. At a high level, these can be broken down into:

* validating API request parameters (both CS and SS API); and
* validating events (and other such data types, like EDUs etc).

Let's consider these in turn.

# API validation

Given that we already have JSON schema specifications for our APIs, this is theoretically relatively straightforward (see, for example https://github.com/matrix-org/synapse/commit/c39941cd6e3727d5571d01bc6f72d7d439f41d13, which is a proof-of-concept applying this to a single endpoint), though it's certain to uncover a large number of places where clients are relying on non-spec-compliant behaviour. We also have to be wary, especially on the SS API, to ensure backwards compatibility.

Doing this would certainly reduce the number of false 500 errors, which as above brings a number of advantages. It might also reduce the occurrences of bugs due to bad events (e.g. non-string display names) since updated Synapses will correctly reject them on the CS API; however, it will absolutely *not* fix those bugs, since such malformed data could still be received from buggy or malicious servers over federation.

# Event validation

Validating events, particularly those received over federation, is quite hard as:
1. you need to decide what to do with events that fail validation,
2. retaining backwards compatibility means it’s hard to reject/drop/decline to handle events that fail strict validation.

For example, an event with malformed `m.relates_to` data can’t just be dropped as, according to the authentication rules of all room versions to date, it is a valid event, even though its payload (that Synapse still interprets) is invalid, since annotations were added *after* the current room versions were specced.

The main problem we have currently is that events contain a mix of properties which *are* validated on receipt (`auth_events`, `prev_events`, etc) alongside a bunch of untrusted data that we cannot assume nor assert the types of. Then, when we come to access event data, we need to remember to add checks for any untrusted data. While room versions allow us to add stricter schemas, relying on that approach will always be a case of playing catchup as we’ll want to use new features in older room versions where possible. (See also [MSC2801](https://github.com/matrix-org/matrix-doc/pull/2801) on this subject.)

Ideally, therefore, we’d try and add some tooling to make it easier to statically assert (or at least tell easily in the code) whether the fields you’re accessing on events (and other data types) have been validated or not. Hopefully it is possibly to do something with mypy here, however it will require some experimentation here.

# Conclusions

In summary there are a few paths going forward:
1. validate schemas of APIs;
2. validate a basic schema for events (and other data types), potentially using room versions to allow us to enforce stricter schemas while retaining backwards compatibilty; and
3. experiment with mypy and other tooling (as well as code changes) to try and highlight the difference between validated and unvalidated data in events, and to enforce correct validating when using untrusted data.

Both 1 and 2 would be good things to do, and may reduce the number of occurrences of bugs, but won’t actually fix the class of bugs we’re seeing due to unexpected formats of various keys in events. However, while paths 1 and 2 are something that we know how to do, path 3 has a lot more unknowns attached to it, but ultimately is the only option that will fully prevent the class of bugs we’re seeing.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.