Limit body size for all requests

[jmattheis]

We should limit the body size for all requests. It's currently possible to send big messages to /message or big yaml configs to /plugin/{id}/config.

I guess a good default could be something like 10MB, but it should be configurable.

Email Report

Summary

Gotify Server's plugin configuration update feature accepts
attacker-controlled YAML bodies at POST /plugin/{id}/config.
Confirmed in v2.9.1, when registration=true and at least one
installed plugin supports Configurer, an unauthenticated remote
attacker can self-register and submit an oversized configuration body to
trigger severe memory pressure and persistent configuration storage
growth.

Details

UpdateConfig verifies plugin ownership and Configurer support, but
then immediately calls io.ReadAll(ctx.Request.Body) with no size
limit. The full request body is buffered before YAML parsing or
plugin-side validation, so any reachable caller can force memory
allocation proportional to the submitted body size. After validation,
the handler stores the original bytes in conf.Config, which turns the
same endpoint into a persistence amplifier rather than only a transient
parse-time sink.

conf, err := c.DB.GetPluginConfByID(id)
...
if aborted := supportOrAbort(ctx, instance, compat.Configurer); aborted { return }
newconfBytes, err := io.ReadAll(ctx.Request.Body)
...
if err := yaml.Unmarshal(newconfBytes, newConf); err != nil { ... }
if err := instance.ValidateAndSetConfig(newConf); err != nil { ... }
conf.Config = newconfBytes

When registration=true, POST /user is reachable under
authentication.Optional() and allows unauthenticated non-admin
account creation. User creation triggers fireUserAdded, which
initializes per-user instances for all installed plugins, and
RequireClient accepts Basic Auth directly. The attacker can then call
GET /plugin to enumerate a plugin instance whose capabilities
include configurer, fetch the current YAML from
GET /plugin/{id}/config, and reuse that id against
POST /plugin/{id}/config.

Because the handler stores raw YAML bytes rather than a normalized
representation, the attacker can append large YAML comment blocks to an
otherwise valid configuration. yaml.Unmarshal ignores comments, but
the oversized original document is still persisted as
PluginConf.Config. I verified the unbounded body read and raw-byte
persistence in api/plugin.go, and the self-registration plus Basic
Auth reachability in api/user.go, router/router.go,
auth/authentication.go, and plugin/manager.go. I did not identify
a fixed version from the current materials.

PoC

1. Ensure the target runs Gotify Server v2.9.1 with
   registration=true and at least one installed plugin whose
   /plugin entry lists configurer in capabilities.

2. Create a normal account:

POST /user
Content-Type: application/json

{"name":"pocuser","pass":"PocPassw0rd!"}

3. Use Basic Auth for that account to call GET /plugin, select an
   entry whose capabilities array contains configurer, then fetch
   its current YAML from GET /plugin/{id}/config.

4. Append a large number of YAML comment lines to the returned document
   and submit it back:

POST /plugin/{id}/config
Authorization: Basic <base64(pocuser:PocPassw0rd!)>
Content-Type: application/x-yaml

<original valid YAML>
# padding 000001
# padding 000002
...

5. Observe high memory pressure during the request. If the YAML remains
   otherwise valid, a subsequent GET /plugin/{id}/config returns the
   enlarged document, confirming that the oversized raw body was
   persisted.

Impact

Observed impact is a conditional unauthenticated network denial of
service in deployments that allow self-registration and have at least
one installed Configurer plugin: a remote attacker can consume
process memory with a single oversized request and can repeatedly
enlarge persisted plugin configuration data. Where registration is
disabled, the same bug remains reachable to a low-privilege
authenticated user. I did not confirm confidentiality or integrity
impact from current evidence.

[eternal-flame-AD]

I didn't quite get what this report is supposed to to express, a rather straightforward way to achieve the same "persistent" memory pressure (which isn't really persistent, it's actually transient at time-of-query) is simply register 1000 clients with very long names and then hit the /client endpoint . Adding a body limit would also not fix this vector because you can drip feed data in separate requests.

We can certainly add a limit for everything but I would find most OSS software accepting tradeoffs (or at least neglecting and not suffering from attacks) on this level of attack (filling database with junk data at 1:1 amplification ratio and hope the server memory is exhausted when reading it back) is not worth the risk reward ratio for an attacker.

[eternal-flame-AD]

Did the reporter actually have a viable PoC where they have a viable methodology achive the stated "consume process memory with a single oversized request and can repeatedly enlarge persisted plugin configuration data" outcome?

I don't see an endpoint they can repeatedly "fatten up" the configuration data, just the /config endpoint they can overwrite it with (and that is also subject to approval by plugin code). There is also the theoretical limit of 1GB as these data need to be stored in the database and most databases have a 1GB limit (I looked at the set config API handler it could be written better though, I will open a PR for that).

[pyuysig]

Thanks, this seems headed in the right direction for #950.

For my report, the main thing I care about is enforcing the body limit before the request is fully read/processed, so oversized requests to /message and /plugin/{id}/config get rejected early.

I see app/client-count limits as a separate concern. Having configurable size limits would be useful:)

[eternal-flame-AD]

I see app/client-count limits as a separate concern.

It's actually exactly the same concern (excessive large data pushed through API can lead to a large amount of data being in memory) and in fact more aligned with your report because it actually allows you to accumulate data over multiple requests.

I don't want to make a checklist mentally fix. I want to be honest with you that this is a problem I have always "defaulted out of scope" in my own mental threat model of gotify, simply because we never claimed or implied it does resource quota management, and it's very transparent that endpoints like /client allow unconstrained growth.

I'm not saying my implicit understanding is the arbiter of truth in Gotify security, I'm saying if it is incorrect then we need a realist threat model where before the patch the risk is open and after the patch the risk is clearly mitigated, not just "this checklist said add limit here" and totally ignoring there are 10 other Gotify-specific things without a limit.

The honest way forward is either

• acknowledge server resource exhaustion by a malicious authenticated user as in scope and fix this problem completely (which means a real quota system and cover all unpaginated resourced) or
• declare them out of scope and ask users to factor these residual threat in their integration plan (which is trivial for the body size limit problem, many projects especially PHP frameworks expect users to put the body size limit on the reverse proxy or WAF layer), but we also have to be honest and acknowledge zero quota on low privilege resources is a clear DOS residual risk that is nontrivial to mitigate by end user.