Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Configurable maximum number of events requested by /sync and /messages #2221

Merged
merged 3 commits into from May 15, 2017

Conversation

Projects
None yet
3 participants
Contributor

psaavedra commented May 13, 2017 edited

Fixes: #2220

Some test done during this Saturday confirmed me a new attact vector for Matrix using the /sync (API). The vulnerability is on Matrix don't set an upper limit for the max number of events to request for a requested room, this allow the attacker generates huge SQL queries in the server which can degradate the service and lead a DDoS.

Set the limit on the returned events in the timeline in the get and sync operations. The default value is -1, means no upper limit.

For example, using filter_timeline_limit: 5000:

POST /_matrix/client/r0/user/user:id/filter
{
room: {
    timeline: {
      limit: 1000000000000000000
    }
}
}
GET /_matrix/client/r0/user/user:id/filter/filter:id

{
room: {
    timeline: {
      limit: 5000
    }
}
}

The server cuts down the room.timeline.limit.

Signed-off-by: Pablo Saavedra (psaavedra@igalia.com)

Configurable maximum number of events requested by /sync and /messages (
#2220)

Set the limit on the returned events in the timeline in the get and sync
operations. The default value is -1, means no upper limit.

For example, using `filter_timeline_limit: 5000`:

POST /_matrix/client/r0/user/user:id/filter
{
room: {
    timeline: {
      limit: 1000000000000000000
    }
}
}

GET /_matrix/client/r0/user/user:id/filter/filter:id

{
room: {
    timeline: {
      limit: 5000
    }
}
}

The server cuts down the room.timeline.limit.

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Owner

erikjohnston commented May 15, 2017

@matrixbot ok to test

Contributor

psaavedra commented May 15, 2017

Fixing errors. ...

Fixed implementation errors
* Added HS as property in SyncRestServlet
* Fixed set_timeline_upper_limit function implementat¡ion
synapse/rest/client/v2_alpha/_base.py
+ return # no upper limits
+ if 'room' in filter_json \
+ and 'timeline' in filter_json['room'] \
+ and 'limit' in filter_json['room']['timeline']:
@erikjohnston

erikjohnston May 15, 2017

Owner

Style nit: We prefer to never use \ style new line continuations and instead use brackets.

e.g. something like:

if (
    'room' in filter_json
    and 'timeline' in filter_json['room']
    and 'limit' in filter_json['room']['timeline']
):
    ...

(This could also be written as:

timeline = filter_json.get('room', {}).get('timeline', {})
if 'limit' in timeline:
    ...

but I'm not sure if that's actually better in this case)

synapse/rest/client/v2_alpha/filter.py
@@ -85,6 +86,9 @@ def on_POST(self, request, user_id):
raise AuthError(403, "Can only create filters for local users")
content = parse_json_object_from_request(request)
+ set_timeline_upper_limit(content,
+ self.hs.config.filter_timeline_limit)
@erikjohnston

erikjohnston May 15, 2017

Owner

Style nit: For multi line stuff we prefer the following style:

set_timeline_upper_limit(
    content,
    self.hs.config.filter_timeline_limit,
)
Owner

erikjohnston commented May 15, 2017

Other than some style nits this looks good! If you could quickly fix those up then I'm happy to merge this

(Note to self: see if we can get pyflakes to complain about those things)

Contributor

psaavedra commented May 15, 2017

Owner

erikjohnston commented May 15, 2017

Thanks for this! Could you just quickly sign off as per CONTRIBUTING.rst please? Just as a comment/email here is fine. (Sorry for not spotting this before)

Contributor

psaavedra commented May 15, 2017

Signed-off-by: Pablo Saavedra psaavedra@igalia.com

Contributor

psaavedra commented May 15, 2017

Owner

erikjohnston commented May 15, 2017

Thanks!

@erikjohnston erikjohnston merged commit 2c9475b into matrix-org:develop May 15, 2017

4 checks passed

Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Merged PR) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

psaavedra added a commit to psaavedra/synapse that referenced this pull request May 19, 2017

Merge tag 'v0.21.0' into v0.21.0_no_federate_by_default
Changes in synapse v0.21.0 (2017-05-18)
=======================================

No changes since v0.21.0-rc3

Changes in synapse v0.21.0-rc3 (2017-05-17)
===========================================

Features:

* Add per user rate-limiting overrides (PR #2208)
* Add config option to limit maximum number of events requested by ``/sync``
  and ``/messages`` (PR #2221) Thanks to @psaavedra!

Changes:

* Various small performance fixes (PR #2201, #2202, #2224, #2226, #2227, #2228,
  #2229)
* Update username availability checker API (PR #2209, #2213)
* When purging, don't de-delta state groups we're about to delete (PR #2214)
* Documentation to check synapse version (PR #2215) Thanks to @hamber-dick!
* Add an index to event_search to speed up purge history API (PR #2218)

Bug fixes:

* Fix API to allow clients to upload one-time-keys with new sigs (PR #2206)

Changes in synapse v0.21.0-rc2 (2017-05-08)
===========================================

Changes:

* Always mark remotes as up if we receive a signed request from them (PR #2190)

Bug fixes:

* Fix bug where users got pushed for rooms they had muted (PR #2200)

Changes in synapse v0.21.0-rc1 (2017-05-08)
===========================================

Features:

* Add username availability checker API (PR #2183)
* Add read marker API (PR #2120)

Changes:

* Enable guest access for the 3pl/3pid APIs (PR #1986)
* Add setting to support TURN for guests (PR #2011)
* Various performance improvements (PR #2075, #2076, #2080, #2083, #2108,
  #2158, #2176, #2185)
* Make synctl a bit more user friendly (PR #2078, #2127) Thanks @APwhitehat!
* Replace HTTP replication with TCP replication (PR #2082, #2097, #2098,
  #2099, #2103, #2014, #2016, #2115, #2116, #2117)
* Support authenticated SMTP (PR #2102) Thanks @DanielDent!
* Add a counter metric for successfully-sent transactions (PR #2121)
* Propagate errors sensibly from proxied IS requests (PR #2147)
* Add more granular event send metrics (PR #2178)

Bug fixes:

* Fix nuke-room script to work with current schema (PR #1927) Thanks
  @zuckschwerdt!
* Fix db port script to not assume postgres tables are in the public schema
  (PR #2024) Thanks @jerrykan!
* Fix getting latest device IP for user with no devices (PR #2118)
* Fix rejection of invites to unreachable servers (PR #2145)
* Fix code for reporting old verify keys in synapse (PR #2156)
* Fix invite state to always include all events (PR #2163)
* Fix bug where synapse would always fetch state for any missing event (PR #2170)
* Fix a leak with timed out HTTP connections (PR #2180)
* Fix bug where we didn't time out HTTP requests to ASes  (PR #2192)

Docs:

* Clarify doc for SQLite to PostgreSQL port (PR #1961) Thanks @benhylau!
* Fix typo in synctl help (PR #2107) Thanks @HarHarLinks!
* ``web_client_location`` documentation fix (PR #2131) Thanks @matthewjwolff!
* Update README.rst with FreeBSD changes (PR #2132) Thanks @feld!
* Clarify setting up metrics (PR #2149) Thanks @encks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment