Passing -1 to json_tokener_parse_ex is possibly unsafe

[schwehr]

json_tokener_parse_ex(tok, str, -1); appears to be unsafe. e.g. in json_tokener_parse_verbose().

There are accesses to str without length checks in the function.

              if ((len == -1 || len > (tok->char_offset + 2)) &&
                  // str[0] != '0' &&  // implied by json_hex_chars, above.
                  (str[1] == '\\') &&
                  (str[2] == 'u'))

So a len of -1 and a str of "" may crash. I'm not 100% sure of this as I'm working with an older copy of json-c.

crash-4f47d9a74b38cc49fdf3c27602fe6a8e68d8352b.txt

This triggered an ASAN heap-buffer-overflow in gdal fixed in this commit:

https://trac.osgeo.org/gdal/changeset/38107

[ploxiln]

len == -1 means that str must be null terminated (otherwise the library is being mis-used). So, as the comment suggests, the previous line if (c && strchr(json_hex_chars, c)) { ensures that str[0] is not null, so str[1] must be valid. If str[1] is \, then it is not null, thus str[2] must be valid. So str[1] and str[2] are not accessed if they might be invalid - as soon as a null terminator appears, access stops.

Side note, the comment has a bit of a bug:

// str[0] != '0'

should be

// str[0] != '\0'

[ploxiln]

The testcase should be verified with the latest json-c. I'm just saying that I don't think the bug is in that particular place. (Also, the "fix" in gdal is just a work-around.)

[ploxiln]

I tested your json sample "crash-4f47d9a74b38cc49fdf3c27602fe6a8e68d8352b.txt" (which I renamed "malformed.json") with the following c program:

#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>

#include "json_tokener.h"

int main()
{
	char jsonstr[256];
	int fd = open("malformed.json", O_RDONLY);
	if (fd < 0) {
		perror("open failed");
		return 1;
	}
	ssize_t r = read(fd, jsonstr, sizeof(jsonstr) - 1);
	if (r <= 0) {
		perror("read failed");
		return 1;
	}
	printf("json file size: %zd\n", r);
	jsonstr[r] = '\0';
	close(fd);

	struct json_object *obj;
	struct json_tokener *tok = json_tokener_new();
	obj = json_tokener_parse_ex(tok, jsonstr, -1);

	enum json_tokener_error tokerr = json_tokener_get_error(tok);
	printf("json obj = %p ; tokener error = %s\n", obj, json_tokener_error_desc(tokerr));
	json_tokener_free(tok);
	return 0;
}

It was Valgrind clean and ASAN clean. I tested with both current master and json-c-0.12.1.

I did the (more tricky) ASAN test roughly as follows, on ubuntu-16.04 x86_64:

cd json-c
./configure CFLAGS=-fsanitize=address LDFLAGS=-fsanitize=address
make
gcc -g -O1 -fsanitize=address -Wall -Wextra -o test_malformed test_malformed.c -L.libs -ljson-c
ASAN_OPTIONS=verbosity=1 LD_LIBRARY_PATH=.libs ./test_malformed

output:

==20110==Parsed ASAN_OPTIONS: verbosity=1
==20110==AddressSanitizer: failed to intercept '__isoc99_printf'
... (skipping random stuff)
==20110==T0: stack [0x7ffdcd362000,0x7ffdcdb62000) size 0x800000; local=0x7ffdcdb60aa4
==20110==AddressSanitizer Init done
json file size: 42
json obj = (nil) ; tokener error = unexpected end of data
==20111==Attached to thread 20110.
==20111==Detached from thread 20110.

[schwehr]

GDAL uses and old copy of json-c and it was crashing by passing "" and -1. The crash is only useful via gdal

[hawicz]

Since you say it's only a problem with old versions of json-c I'm closing this bug.

[schwehr]

I think it's reasonable to close this. GDAL is now immune to this as it always specifies the size now and anyone who wants to follow up can find this bug entry.