Re: [PATCH nft] json: work around fuzzer-induced assert crashes

[Phil Sutter <phil@nwl.cc>]

Hi,

On Tue, Jun 03, 2025 at 08:14:26PM +0200, Phil Sutter wrote:
> On Mon, Jun 02, 2025 at 02:12:49PM +0200, Florian Westphal wrote:
> > fuzzer can cause assert failures due to json_pack() returning a NULL
> > value and therefore triggering the assert(out) in __json_pack macro.
> > 
> > All instances I saw are due to invalid UTF-8 strings, i.e., table/chain
> > names with non-text characters in them.
> 
> So these odd strings are supported everywhere else and we only fail to
> format them into JSON? According to the spec[1] this should even support
> "\uXXXX"-style escapes. Not sure if it helps us, but to me this sounds
> like a bug in libjansson. Or are these really binary sequences somehow
> entered into nftables wich jansson's utf8_check_string() identifies as
> invalid?
> 
> > Work around this for now, replace the assert with a plaintext error
> > message and return NULL instead of abort().
> 
> The old code was active with DEBUG builds, only. If undefined, it would
> just call json_pack() itself. Did you test a non-DEBUG build, too? I
> wonder if json.c swallows the NULL return or we see at least an error
> message.

So for testing purposes, I built a json_pack() wrapper which
occasionally returns NULL (and does not assert()). This causes almost
all py testsuite tests to fail, obviously due to JSON dump differences.

Piping the non-empty ruleset-after.json files into json_pp shows no
errors, i.e. if we get any JSON output it is valid at least.

Looking at testout.log of those tests with rc-failed-dump status, I
don't see error messages related to JSON output. So catching the NULL
return is definitely necessary, also with non-DEBUG builds. Hence:

Acked-by: Phil Sutter <phil@nwl.cc>

Thanks, Phil