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
0001-oauth-Limit-JSON-parsing-depth-in-the-client.patch
Description: Binary data
