Horton

Thread

Commits

  1. oauth: Limit JSON parsing depth in the client

  1. [PATCH] oauth: Prevent stack overflow by limiting JSON parse depth

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-05-07T18:13:24Z

    Hi all,

I forgot to put a recursion limit in the new OAuth parsers; the
server-side depth checks don't apply to the client, and it's not using
the incremental parser to move the burden from the stack to the heap.
Luckily, we track the nesting level already, so a fix (attached) can
be pretty small.

(Eventually I'd like to set a limit via the client-side JSONAPI, or
maybe port the server's stack checks? That's 19 material.)

Enforcing a limit is relatively easy, but choosing the limit is
somewhat harder, because OAuth will continue to evolve and add
extensions that we're supposed to gracefully ignore. The most typical
depth I've seen in practice is 2 (an object containing arrays of
strings). I took a look through the IANA registry [1], and I think the
maximum nesting depth we can get today is if someone plops an entire
JSON Web Key Set into their discovery document. That could bring the
depth up to 6 with some of the newer key types.

Hopefully the extension depth doesn't continue to grow unbounded, but
it's easy for spec writers to find ways to nest new stuff, so I've
chosen a max depth of 16. Probably overkill. Hopefully overkill?

For the SASL parser, which is only speaking OAUTHBEARER to a Postgres
server/proxy, I think we can be way more strict. We expect a nesting
depth of only 1 there. I've put in a limit of 8, to try to avoid a
backport in the case that some nested extension is published and
suddenly gains popularity, but I could easily be talked downwards.

And if we ever have to revisit these limits, I think that would be
reason enough to switch from the recursive descent parser over to the
incremental parser, anyway.

= Postmortem =

As an aside: I was curious why libfuzzer didn't catch this, since I've
been using it as a safety net. The answer was a depressingly simple
"you ignored a printed warning":

    INFO: -max_len is not provided; libFuzzer will not generate inputs
larger than 4096 bytes

It looks like this default limit is variable, based on the corpus you
already have, and 4K is not enough to run my machine out of stack. But
setting it explicitly to our response limit of 256K allows libfuzzer
to find this issue very quickly, and I will do that going forward.

(If anyone has run libfuzzer before and has a "favorite
configuration", I'd love to hear it!)

Thanks,
--Jacob

[1] https://www.iana.org/assignments/oauth-parameters/oauth-parameters.xhtml#authorization-server-metadata

  2. Re: [PATCH] oauth: Prevent stack overflow by limiting JSON parse depth

    Aleksander Alekseev <aleksander@timescale.com> — 2025-05-08T12:22:35Z

    Hi Jacob,

> I forgot to put a recursion limit in the new OAuth parsers; the
> server-side depth checks don't apply to the client, and it's not using
> the incremental parser to move the burden from the stack to the heap.
> Luckily, we track the nesting level already, so a fix (attached) can
> be pretty small.
>
> [...]

Thanks for the patch. It looks good to me. It's well documented and
covered with tests. I can confirm that the tests pass. Also they fail
if I decrease the $nesting_limit value to 15.

--
Best regards,
Aleksander Alekseev

  3. Re: [PATCH] oauth: Prevent stack overflow by limiting JSON parse depth

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-05-08T15:45:04Z

    On Thu, May 8, 2025 at 5:22 AM Aleksander Alekseev
<aleksander@timescale.com> wrote:
> Thanks for the patch. It looks good to me. It's well documented and
> covered with tests. I can confirm that the tests pass. Also they fail
> if I decrease the $nesting_limit value to 15.

Thanks for the review!

--Jacob