From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43858) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a0uoF-0003Ct-H2 for qemu-devel@nongnu.org; Mon, 23 Nov 2015 12:21:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a0uoB-00032C-DL for qemu-devel@nongnu.org; Mon, 23 Nov 2015 12:21:47 -0500 Received: from oxygen.pond.sub.org ([144.76.244.19]:42098) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a0uoB-00031c-7h for qemu-devel@nongnu.org; Mon, 23 Nov 2015 12:21:43 -0500 Received: from blackfin.pond.sub.org (p5B3289AC.dip0.t-ipconnect.de [91.50.137.172]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by oxygen.pond.sub.org (Postfix) with ESMTPSA id 3CCCD201D1 for ; Mon, 23 Nov 2015 18:21:38 +0100 (CET) From: Markus Armbruster References: <1446122683-2355-1-git-send-email-armbru@redhat.com> <1446122683-2355-3-git-send-email-armbru@redhat.com> <5632488D.8040109@redhat.com> Date: Mon, 23 Nov 2015 18:21:36 +0100 Message-ID: <87vb8siqcf.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 2/4] json-streamer: Don't crash when input exceeds nesting limit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, lcapitulino@redhat.com Eric Blake writes: > On 10/29/2015 06:44 AM, Markus Armbruster wrote: >> We limit nesting depth and input size to defend against input >> triggering excessive heap or stack memory use (commit 29c75dd >> json-streamer: limit the maximum recursion depth and maximum token >> count). However, when the nesting limit is exceeded, >> parser_context_peek_token()'s assertion fails. >> >> Broken in commit 65c0f1e "json-parser: don't replicate tokens at each >> level of recursion". >> >> To reproduce stuff 1025 open braces or brackets into QMP. >> >> Fix by taking the error exit instead of the normal one. >> >> Reported-by: Eric Blake >> Signed-off-by: Markus Armbruster >> --- >> qobject/json-streamer.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Reviewed-by: Eric Blake > > However, a couple comments about the context: > >> if (type == JSON_ERROR) { >> goto out_emit_bad; >> } else if (parser->brace_count < 0 || >> parser->bracket_count < 0 || >> (parser->brace_count == 0 && >> parser->bracket_count == 0)) { >> goto out_emit; > > Should we go to out_emit_bad for brace_count/bracket_count < 0, and save > out_emit only for the case where brace == bracket == 0? Can we even > trigger negative counts (probably by attempting unpaired "{]]", but will > that trigger earlier errors?) Fair questions. Unpaired closing brace or bracket makes the count go negative. A simple test input is "}". As far as I can tell, nothing breaks: we emit an error, and the parser recovers. Perhaps the error exit would be more appropriate anyway, but I can't tell. Let's leave it alone as long as it works. >> } else if (parser->token_size > MAX_TOKEN_SIZE || >> parser->bracket_count > MAX_NESTING || >> parser->brace_count > MAX_NESTING) { >> /* Security consideration, we limit total memory allocated per object >> * and the maximum recursion depth that a message can force. >> */ >> goto out_emit; >> } >> >> return; >> >>out_emit_bad: >> /* clear out token list and tell the parser to emit and error > > Typo: s/and error/an error/ Fixing, thanks.