Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 1 May 2024 15:08:45 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
	Andrew Dunstan <andrew@dunslane.net>
Subject: Re: [PATCH] json_lex_string: don't overread on bad UTF8
Message-ID: <ZjHcbcFcZ7pku2VH@paquier.xyz>
References: <CAOYmi+ncM7pwLS3AnKCSmoqqtpjvA8wmCdoBtKA3ZrB2hZG6zA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="UO8EvQN97bzRo6RY"
Content-Disposition: inline
In-Reply-To: <CAOYmi+ncM7pwLS3AnKCSmoqqtpjvA8wmCdoBtKA3ZrB2hZG6zA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZjHcbcFcZ7pku2VH%40paquier.xyz>
Precedence: bulk


--UO8EvQN97bzRo6RY
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 30, 2024 at 10:39:04AM -0700, Jacob Champion wrote:
> When json_lex_string() hits certain types of invalid input, it calls
> pg_encoding_mblen_bounded(), which assumes that its input is
> null-terminated and calls strnlen(). But the JSON lexer is constructed
> with an explicit string length, and we don't ensure that the string is
> null-terminated in all cases, so we can walk off the end of the
> buffer. This isn't really relevant on the server side, where you'd
> have to get a superuser to help you break string encodings, but for
> client-side usage on untrusted input (such as my OAuth patch) it would
> be more important.

Not sure to like much the fact that this advances token_terminator
first.  Wouldn't it be better to calculate pg_encoding_mblen() first,
then save token_terminator?  I feel a bit uneasy about saving a value
in token_terminator past the end of the string.  That a nit in this
context, still..

> Attached is a draft patch that explicitly checks against the
> end-of-string pointer and clamps the token_terminator to it. Note that
> this removes the only caller of pg_encoding_mblen_bounded() and I'm
> not sure what we should do with that function. It seems like a
> reasonable API, just not here.

Hmm.  Keeping it around as currently designed means that it could
cause more harm than anything in the long term, even in the stable
branches if new code uses it.  There is a risk of seeing this new code
incorrectly using it again, even if its top comment tells that we rely
on the string being nul-terminated.  A safer alternative would be to
redesign it so as the length of the string is provided in input,
removing the dependency of strlen in its internals, perhaps.  Anyway,
without any callers of it, I'd be tempted to wipe it from HEAD and
call it a day.

> The new test needs to record two versions of the error message, one
> for invalid token and one for invalid escape sequence. This is
> because, for smaller chunk sizes, the partial-token logic in the
> incremental JSON parser skips the affected code entirely when it can't
> find an ending double-quote.

Ah, that makes sense.  That looks OK here.  A comment around the test
would be adapted to document that, I guess. =20

> Tangentially: Should we maybe rethink pieces of the json_lex_string
> error handling? For example, do we really want to echo an incomplete
> multibyte sequence once we know it's bad? It also looks like there are
> places where the FAIL_AT_CHAR_END macro is called after the `s`
> pointer has already advanced past the code point of interest. I'm not
> sure if that's intentional.

Advancing the tracking pointer 's' before reporting an error related
the end of the string is a bad practive, IMO, and we should avoid
that.  json_lex_string() does not offer a warm feeling regarding that
with escape characters, at least :/
--
Michael

--UO8EvQN97bzRo6RY
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmYx3G0ACgkQnvQgOdby
QH3HihAAkMj5DzAmIRl4LV1yPCyXTzOl6xGBdMEqrnP4xo1VgnRdSYnRYqq2HVpV
v0qduqIeJstKK2Cn+jBzdCD1xder+RSkhIEqLf4JplMdi0vx3szIgLRdg45vzcaV
pMn23AXPEz6Rlw1epQ4b3H34isW3R8ornoF/05Bn85cNDgjRbuM01TN4kLHASBL1
FTARf4ZDmVqDwdMGQqg7ZRw8hWVesevRK0TPockznkXBEkvV2efkFB04R2yZAY2v
4jEY+o5AAjDq8bW5D9FDdCsywxkifLUMB+HkYap4921mGktTL6nOJ0EozXBgdZF8
IEWocTjmifFJ+FeID2kq+A4AesXlhKkaRk2tjShYupQFcof36W2vMljuR6xWdPLU
bR3h7iFmmJEgNu5N+LAEKTqFjUzcwSEo0DJ4uclPUs+auaeplWa930+KXvYnlIqH
AbPAJMtgObsmv8I2v4nrlEFgLbvT2W1iKQ/swne+jDGnpc6+L2PwZiMuV159ALsN
eGl25OFCyQlNWsBBUdkCGPiXZAQgMnhfh7TQB8HCJX8SuqHe7cpR1sgSvaExAjBc
3djL7Lwr2hqzoNCUGR/0Og0+5bxESNvX/ncjSZtD/mWvWOdhPw9kHmd5VXL2yPv1
ijvi60BPwiOwxcmtj9kB879JR6b+Z5PGt9mNlfFMA5K9JMO/qTE=
=OtQ1
-----END PGP SIGNATURE-----

--UO8EvQN97bzRo6RY--