From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40442 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PyBZ6-0001f1-EV for qemu-devel@nongnu.org; Fri, 11 Mar 2011 18:16:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PyBZ3-0008HG-39 for qemu-devel@nongnu.org; Fri, 11 Mar 2011 18:16:10 -0500 Received: from e7.ny.us.ibm.com ([32.97.182.137]:45972) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PyBZ3-0008H9-0s for qemu-devel@nongnu.org; Fri, 11 Mar 2011 18:16:09 -0500 Received: from d01dlp02.pok.ibm.com (d01dlp02.pok.ibm.com [9.56.224.85]) by e7.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p2BMtL1o024515 for ; Fri, 11 Mar 2011 17:55:21 -0500 Received: from d01relay06.pok.ibm.com (d01relay06.pok.ibm.com [9.56.227.116]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 34D796E8036 for ; Fri, 11 Mar 2011 18:16:08 -0500 (EST) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay06.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p2BNG60X1720322 for ; Fri, 11 Mar 2011 18:16:06 -0500 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p2BNG6LW002422 for ; Fri, 11 Mar 2011 20:16:06 -0300 Message-ID: <4D7AAD33.1000804@linux.vnet.ibm.com> Date: Fri, 11 Mar 2011 17:16:03 -0600 From: Michael Roth MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 10/11] json-streamer: limit the maximum recursion depth and maximum token count References: <1299877249-13433-1-git-send-email-aliguori@us.ibm.com> <1299877249-13433-11-git-send-email-aliguori@us.ibm.com> In-Reply-To: <1299877249-13433-11-git-send-email-aliguori@us.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Paolo Bonzini , Luiz Capitulino , qemu-devel@nongnu.org, Michael D Roth , Markus Armbruster On 03/11/2011 03:00 PM, Anthony Liguori wrote: > The JSON parse we use is a recursive decent parser which means that recursion > is used to backtrack. To avoid stack overflows from malformed input, we need > to limit recursion depth. > > Fortunately, we know the maximum recursion depth in the message parsing phase > so we can very easily check for unreasonably deep recursion. If we find that > emit the current token stream and let the parser handle it. The idea here is > that hopefully the stream will recover or at least error out gracefully. > > We also limit the maximum token count. We don't limit the number of tokens, but > rather the total memory used for tokens at any given point in time. > > Signed-off-by: Anthony Liguori > > diff --git a/json-streamer.c b/json-streamer.c > index f7e7a68..c7f43b0 100644 > --- a/json-streamer.c > +++ b/json-streamer.c > @@ -18,6 +18,9 @@ > #include "json-lexer.h" > #include "json-streamer.h" > > +#define MAX_TOKEN_SIZE (64ULL<< 20) > +#define MAX_NESTING (1ULL<< 10) > + > static void json_message_process_token(JSONLexer *lexer, QString *token, JSONTokenType type, int x, int y) > { > JSONMessageParser *parser = container_of(lexer, JSONMessageParser, lexer); > @@ -49,6 +52,8 @@ static void json_message_process_token(JSONLexer *lexer, QString *token, JSONTok > qdict_put(dict, "x", qint_from_int(x)); > qdict_put(dict, "y", qint_from_int(y)); > > + parser->token_size += token->length; > + > qlist_append(parser->tokens, dict); > > if (parser->brace_count == 0&& > @@ -56,6 +61,19 @@ static void json_message_process_token(JSONLexer *lexer, QString *token, JSONTok > parser->emit(parser, parser->tokens); > QDECREF(parser->tokens); > parser->tokens = qlist_new(); > + parser->token_size = 0; > + } 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. > + */ > + parser->brace_count = 0; > + parser->bracket_count = 0; > + parser->emit(parser, parser->tokens); Should we even bother passing this to the parser? That's a lot stuff to churn on that we know isn't going to result in anything useful. If there was a proper object earlier in the stream it would've been emitted when brace/bracket count reached 0. I think it might be nicer to do parser->emit(parser, NULL), and fix up json_parser_parse_err() to check for this and simply return NULL back to qmp_dispatch_err() or whoever is calling. I think brace_count < 0 || bracket_count < 0 should get similar treatment. > + QDECREF(parser->tokens); > + parser->tokens = qlist_new(); > + parser->token_size = 0; > } > } > > @@ -66,6 +84,7 @@ void json_message_parser_init(JSONMessageParser *parser, > parser->brace_count = 0; > parser->bracket_count = 0; > parser->tokens = qlist_new(); > + parser->token_size = 0; > > json_lexer_init(&parser->lexer, json_message_process_token); > } > diff --git a/json-streamer.h b/json-streamer.h > index 09f3bd7..f09bc4d 100644 > --- a/json-streamer.h > +++ b/json-streamer.h > @@ -24,6 +24,7 @@ typedef struct JSONMessageParser > int brace_count; > int bracket_count; > QList *tokens; > + uint64_t token_size; > } JSONMessageParser; > > void json_message_parser_init(JSONMessageParser *parser,