WKB: Avoid buffer overflow

This only happens when not running under PG context, as
lwerror continues execution and that means that even after
detecting there isn't enough bytes still try to read from the buffer

Closes #4535
Closes #495


git-svn-id: http://svn.osgeo.org/postgis/trunk@17904 b70326c6-7e19-0410-871a-916f4a2858ee

NEWS

@@ -10,6 +10,7 @@ Additional performance enhancements if running GEOS 3.8+
   - #4534, Fix leak in lwcurvepoly_from_wkb_state (Raúl Marín)
   - #4536, Fix leak in lwcollection_from_wkb_state (Raúl Marín)
   - #4537, Fix leak in WKT collection parser (Raúl Marín)
+  - #4535, WKB: Avoid buffer overflow (Raúl Marín)
 
 PostGIS 3.0.0rc1
 2019/10/08

liblwgeom/cunit/cu_in_wkb.c

@@ -228,7 +228,7 @@ static void test_wkb_in_malformed(void)
 }
 
 static void
-test_wkb_leak(void)
+test_wkb_fuzz(void)
 {
 	/* OSS-FUZZ https://trac.osgeo.org/postgis/ticket/4534 */
 	uint8_t wkb[36] = {000, 000, 000, 000, 015, 000, 000, 000, 003, 000, 200, 000, 000, 010, 000, 000, 000, 000,
@@ -256,6 +256,11 @@ test_wkb_leak(void)
 	    001, 001, 001, 001, 001, 001, 001, 001, 001, 001, 001, 001, 001, 001, 001, 001, 001, 001, 001};
 	g = lwgeom_from_wkb(wkb2, 319, LW_PARSER_CHECK_NONE);
 	lwgeom_free(g);
+
+	/* OSS-FUZZ: https://trac.osgeo.org/postgis/ticket/4535 */
+	uint8_t wkb3[9] = {0x01, 0x03, 0x00, 0x00, 0x10, 0x8d, 0x55, 0xf3, 0xff};
+	g = lwgeom_from_wkb(wkb3, 9, LW_PARSER_CHECK_NONE);
+	lwgeom_free(g);
 }
 
 /*
@@ -278,5 +283,5 @@ void wkb_in_suite_setup(void)
 	PG_ADD_TEST(suite, test_wkb_in_multicurve);
 	PG_ADD_TEST(suite, test_wkb_in_multisurface);
 	PG_ADD_TEST(suite, test_wkb_in_malformed);
-	PG_ADD_TEST(suite, test_wkb_leak);
+	PG_ADD_TEST(suite, test_wkb_fuzz);
 }

liblwgeom/lwin_wkb.c

@@ -36,14 +36,15 @@
 typedef struct
 {
 	const uint8_t *wkb; /* Points to start of WKB */
-	size_t wkb_size; /* Expected size of WKB */
-	int swap_bytes; /* Do an endian flip? */
-	int check; /* Simple validity checks on geometries */
-	uint32_t lwtype; /* Current type we are handling */
 	int32_t srid;    /* Current SRID we are handling */
-	int has_z; /* Z? */
-	int has_m; /* M? */
-	int has_srid; /* SRID? */
+	size_t wkb_size; /* Expected size of WKB */
+	int8_t swap_bytes;  /* Do an endian flip? */
+	int8_t check;       /* Simple validity checks on geometries */
+	int8_t lwtype;      /* Current type we are handling */
+	int8_t has_z;       /* Z? */
+	int8_t has_m;       /* M? */
+	int8_t has_srid;    /* SRID? */
+	int8_t error;       /* An error was found (not enough bytes to read) */
 	const uint8_t *pos; /* Current parse position */
 } wkb_parse_state;
 
@@ -128,7 +129,7 @@ static inline void wkb_parse_state_check(wkb_parse_state *s, size_t next)
 	if( (s->pos + next) > (s->wkb + s->wkb_size) )
 	{
 		lwerror("WKB structure does not match expected size!");
-		return;
+		s->error = LW_TRUE;
 	}
 }
 
@@ -255,6 +256,8 @@ static char byte_from_wkb_state(wkb_parse_state *s)
 	LWDEBUG(4, "Entered function");
 
 	wkb_parse_state_check(s, WKB_BYTE_SIZE);
+	if (s->error)
+		return 0;
 	LWDEBUG(4, "Passed state check");
 
 	char_value = s->pos[0];
@@ -273,6 +276,8 @@ static uint32_t integer_from_wkb_state(wkb_parse_state *s)
 	uint32_t i = 0;
 
 	wkb_parse_state_check(s, WKB_INT_SIZE);
+	if (s->error)
+		return 0;
 
 	memcpy(&i, s->pos, WKB_INT_SIZE);
 
@@ -302,8 +307,6 @@ static double double_from_wkb_state(wkb_parse_state *s)
 {
 	double d = 0;
 
-	wkb_parse_state_check(s, WKB_DOUBLE_SIZE);
-
 	memcpy(&d, s->pos, WKB_DOUBLE_SIZE);
 
 	/* Swap? Copy into a stack-allocated integer. */
@@ -340,6 +343,8 @@ static POINTARRAY* ptarray_from_wkb_state(wkb_parse_state *s)
 
 	/* Calculate the size of this point array. */
 	npoints = integer_from_wkb_state(s);
+	if (s->error)
+		return NULL;
 	if (npoints > maxpoints)
 	{
 		lwerror("Pointarray length (%d) is too large");
@@ -358,6 +363,8 @@ static POINTARRAY* ptarray_from_wkb_state(wkb_parse_state *s)
 
 	/* Does the data we want to read exist? */
 	wkb_parse_state_check(s, pa_size);
+	if (s->error)
+		return NULL;
 
 	/* If we're in a native endianness, we can just copy the data directly! */
 	if( ! s->swap_bytes )
@@ -405,6 +412,8 @@ static LWPOINT* lwpoint_from_wkb_state(wkb_parse_state *s)
 
 	/* Does the data we want to read exist? */
 	wkb_parse_state_check(s, pa_size);
+	if (s->error)
+		return NULL;
 
 	/* If we're in a native endianness, we can just copy the data directly! */
 	if( ! s->swap_bytes )
@@ -449,10 +458,13 @@ static LWPOINT* lwpoint_from_wkb_state(wkb_parse_state *s)
 static LWLINE* lwline_from_wkb_state(wkb_parse_state *s)
 {
 	POINTARRAY *pa = ptarray_from_wkb_state(s);
+	if (s->error)
+		return NULL;
 
 	if( pa == NULL || pa->npoints == 0 )
 	{
-		ptarray_free(pa);
+		if (pa)
+			ptarray_free(pa);
 		return lwline_construct_empty(s->srid, s->has_z, s->has_m);
 	}
 
@@ -477,9 +489,15 @@ static LWLINE* lwline_from_wkb_state(wkb_parse_state *s)
 static LWCIRCSTRING* lwcircstring_from_wkb_state(wkb_parse_state *s)
 {
 	POINTARRAY *pa = ptarray_from_wkb_state(s);
+	if (s->error)
+		return NULL;
 
 	if( pa == NULL || pa->npoints == 0 )
+	{
+		if (pa)
+			ptarray_free(pa);
 		return lwcircstring_construct_empty(s->srid, s->has_z, s->has_m);
+	}
 
 	if( s->check & LW_PARSER_CHECK_MINPOINTS && pa->npoints < 3 )
 	{
@@ -507,6 +525,8 @@ static LWCIRCSTRING* lwcircstring_from_wkb_state(wkb_parse_state *s)
 static LWPOLY* lwpoly_from_wkb_state(wkb_parse_state *s)
 {
 	uint32_t nrings = integer_from_wkb_state(s);
+	if (s->error)
+		return NULL;
 	uint32_t i = 0;
 	LWPOLY *poly = lwpoly_construct_empty(s->srid, s->has_z, s->has_m);
 
@@ -519,12 +539,13 @@ static LWPOLY* lwpoly_from_wkb_state(wkb_parse_state *s)
 	for( i = 0; i < nrings; i++ )
 	{
 		POINTARRAY *pa = ptarray_from_wkb_state(s);
-		if( pa == NULL )
-			continue;
+		if (pa == NULL)
+			return NULL;
 
 		/* Check for at least four points. */
-		if( s->check & LW_PARSER_CHECK_MINPOINTS && pa->npoints < 4 )
+		if (s->check & LW_PARSER_CHECK_MINPOINTS && pa->npoints < 4)
 		{
+			lwpoly_free(poly);
 			LWDEBUGF(2, "%s must have at least four points in each ring", lwtype_name(s->lwtype));
 			lwerror("%s must have at least four points in each ring", lwtype_name(s->lwtype));
 			return NULL;
@@ -533,6 +554,7 @@ static LWPOLY* lwpoly_from_wkb_state(wkb_parse_state *s)
 		/* Check that first and last points are the same. */
 		if( s->check & LW_PARSER_CHECK_CLOSURE && ! ptarray_is_closed_2d(pa) )
 		{
+			lwpoly_free(poly);
 			LWDEBUGF(2, "%s must have closed rings", lwtype_name(s->lwtype));
 			lwerror("%s must have closed rings", lwtype_name(s->lwtype));
 			return NULL;
@@ -541,8 +563,10 @@ static LWPOLY* lwpoly_from_wkb_state(wkb_parse_state *s)
 		/* Add ring to polygon */
 		if ( lwpoly_add_ring(poly, pa) == LW_FAILURE )
 		{
+			lwpoly_free(poly);
 			LWDEBUG(2, "Unable to add ring to polygon");
 			lwerror("Unable to add ring to polygon");
+			return NULL;
 		}
 
 	}
@@ -560,6 +584,8 @@ static LWPOLY* lwpoly_from_wkb_state(wkb_parse_state *s)
 static LWTRIANGLE* lwtriangle_from_wkb_state(wkb_parse_state *s)
 {
 	uint32_t nrings = integer_from_wkb_state(s);
+	if (s->error)
+		return NULL;
 	LWTRIANGLE *tri = lwtriangle_construct_empty(s->srid, s->has_z, s->has_m);
 	POINTARRAY *pa = NULL;
 
@@ -606,6 +632,8 @@ static LWTRIANGLE* lwtriangle_from_wkb_state(wkb_parse_state *s)
 static LWCURVEPOLY* lwcurvepoly_from_wkb_state(wkb_parse_state *s)
 {
 	uint32_t ngeoms = integer_from_wkb_state(s);
+	if (s->error)
+		return NULL;
 	LWCURVEPOLY *cp = lwcurvepoly_construct_empty(s->srid, s->has_z, s->has_m);
 	LWGEOM *geom = NULL;
 	uint32_t i;
@@ -641,6 +669,8 @@ static LWCURVEPOLY* lwcurvepoly_from_wkb_state(wkb_parse_state *s)
 static LWCOLLECTION* lwcollection_from_wkb_state(wkb_parse_state *s)
 {
 	uint32_t ngeoms = integer_from_wkb_state(s);
+	if (s->error)
+		return NULL;
 	LWCOLLECTION *col = lwcollection_construct_empty(s->lwtype, s->srid, s->has_z, s->has_m);
 	LWGEOM *geom = NULL;
 	uint32_t i;
@@ -687,6 +717,8 @@ LWGEOM* lwgeom_from_wkb_state(wkb_parse_state *s)
 
 	/* Fail when handed incorrect starting byte */
 	wkb_little_endian = byte_from_wkb_state(s);
+	if (s->error)
+		return NULL;
 	if( wkb_little_endian != 1 && wkb_little_endian != 0 )
 	{
 		LWDEBUG(4,"Leaving due to bad first byte!");
@@ -706,13 +738,17 @@ LWGEOM* lwgeom_from_wkb_state(wkb_parse_state *s)
 
 	/* Read the type number */
 	wkb_type = integer_from_wkb_state(s);
+	if (s->error)
+		return NULL;
 	LWDEBUGF(4,"Got WKB type number: 0x%X", wkb_type);
 	lwtype_from_wkb_state(s, wkb_type);
 
 	/* Read the SRID, if necessary */
 	if( s->has_srid )
 	{
 		s->srid = clamp_srid(integer_from_wkb_state(s));
+		if (s->error)
+			return NULL;
 		/* TODO: warn on explicit UNKNOWN srid ? */
 		LWDEBUGF(4,"Got SRID: %u", s->srid);
 	}
@@ -785,8 +821,12 @@ LWGEOM* lwgeom_from_wkb(const uint8_t *wkb, const size_t wkb_size, const char ch
 	s.has_z = LW_FALSE;
 	s.has_m = LW_FALSE;
 	s.has_srid = LW_FALSE;
+	s.error = LW_FALSE;
 	s.pos = wkb;
 
+	if (!wkb || !wkb_size)
+		return NULL;
+
 	return lwgeom_from_wkb_state(&s);
 }