CVE-2018-7998: Concurrent Execution using Shared Resource with Improper Synchronization ('Race Condition') (CWE-362)
value: In libvips before 8.6.3, a NULL function pointer dereference vulnerability was found in the vips_region_generate function in region.c, which allows remote attackers to cause a denial of service or possibly have unspecified other impact via a crafted image file. This occurs because of a race condition involving a failed delayed load and other worker threads.
CWE description: The product contains a code sequence that can run concurrently with other code, and the code sequence requires temporary, exclusive access to a shared resource, but a timing window exists in which the shared resource can be modified by another code sequence that is operating concurrently.
CVSS v3 base score: 7.5
Published: 2018-03-09T19:29Z
Repository: https://github.com/jcupitt/libvips
Language: Other
Files changed: ChangeLog, libvips/foreign/foreign.c, libvips/include/vips/foreign.h
Commit date: 2018-03-05 14:42:09 +0000
Commit message: fix a crash with delayed load

If a delayed load failed, it could leave the pipeline only half-set up.
Sebsequent threads could then segv.

Set a load-has-failed flag and test before generate.

See https://github.com/jcupitt/libvips/issues/893

Diff: diff --git a/ChangeLog b/ChangeLog
index 68f64654..08aaab8c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -2,10 +2,11 @@
 - use pkg-config to find libjpeg, if we can
 - better clean of output image in vips_image_write() fixes a crash 
   writing twice to memory
 - better rounding behaviour in convolution means we hit the vector path more
   often
+- fix a crash if a delayed load failed [gsharpsh00ter]
 
 5/1/18 started 8.6.2
 - vips_sink_screen() keeps a ref to the input image ... stops a rare race
 - fix a minor accidental ABI break in 8.6.0 -> 8.6.1 [remicollet]
 - fix read of plane-separate TIFFs with large strips [remicollet]
diff --git a/libvips/foreign/foreign.c b/libvips/foreign/foreign.c
index 35ad2be5..fb03fd74 100644
--- a/libvips/foreign/foreign.c
+++ b/libvips/foreign/foreign.c
@@ -16,10 +16,12 @@
  * 	- remove max-alpha stuff, this is now automatic
  * 12/6/17
  * 	- transform cmyk->rgb if there's an embedded profile
  * 16/6/17
  * 	- add page_height
+ * 5/3/18
+ * 	- block _start if one start fails, see #893
  */
 
 /*
 
     This file is part of VIPS.
@@ -794,10 +796,15 @@ static void *
 vips_foreign_load_start( VipsImage *out, void *a, void *b )
 {
 	VipsForeignLoad *load = VIPS_FOREIGN_LOAD( b );
 	VipsForeignLoadClass *class = VIPS_FOREIGN_LOAD_GET_CLASS( load );
 
+	/* If this start has failed before in another thread, we can fail now.
+	 */
+	if( load->error )
+		return( NULL );
+
 	if( !load->real ) {
 		if( !(load->real = vips_foreign_load_temp( load )) )
 			return( NULL );
 
 #ifdef DEBUG
@@ -817,23 +824,29 @@ vips_foreign_load_start( VipsImage *out, void *a, void *b )
 		 * vips_foreign_load_invalidate() below.
 		 */
 		g_object_set_qdata( G_OBJECT( load->real ), 
 			vips__foreign_load_operation, load ); 
 
-		if( class->load( load ) ||
-			vips_image_pio_input( load->real ) ) 
-			return( NULL );
-
-		/* ->header() read the header into @out, load has read the
+		/* Load the image and check the result.
+		 *
+		 * ->header() read the header into @out, load has read the
 		 * image into @real. They must match exactly in size, bands,
 		 * format and coding for the copy to work.  
 		 *
 		 * Some versions of ImageMagick give different results between
 		 * Ping and Load for some formats, for example.
+		 *
+		 * If the load fails, we need to stop
 		 */
-		if( !vips_foreign_load_iscompat( load->real, out ) )
+		if( class->load( load ) ||
+			vips_image_pio_input( load->real ) || 
+			vips_foreign_load_iscompat( load->real, out ) ) {
+			vips_operation_invalidate( VIPS_OPERATION( load ) ); 
+			load->error = TRUE;
+
 			return( NULL );
+		}
 
 		/* We have to tell vips that out depends on real. We've set
 		 * the demand hint below, but not given an input there.
 		 */
 		vips_image_pipelinev( load->out, load->out->dhint, 
diff --git a/libvips/include/vips/foreign.h b/libvips/include/vips/foreign.h
index 47e285e3..dcbf21c8 100644
--- a/libvips/include/vips/foreign.h
+++ b/libvips/include/vips/foreign.h
@@ -156,10 +156,15 @@ typedef struct _VipsForeignLoad {
 
 	/* Deprecated: the memory option used to be called disc and default
 	 * TRUE.
 	 */
 	gboolean disc;
+
+	/* Set if a start function fails. We want to prevent the other starts
+	 * from also triggering the load.
+	 */
+	gboolean error;
 } VipsForeignLoad;
 
 typedef struct _VipsForeignLoadClass {
 	VipsForeignClass parent_class;
 	/*< public >*/

Vulnerable code: 		if( class->load( load ) ||
			vips_image_pio_input( load->real ) ) 
			return( NULL );

		/* ->header() read the header into @out, load has read the
		if( !vips_foreign_load_iscompat( load->real, out ) )
Fixed code: - fix a crash if a delayed load failed [gsharpsh00ter]
 * 5/3/18
 * 	- block _start if one start fails, see #893
	/* If this start has failed before in another thread, we can fail now.
	 */
	if( load->error )
		return( NULL );

		/* Load the image and check the result.
		 *
		 * ->header() read the header into @out, load has read the
		 *
		 * If the load fails, we need to stop
		if( class->load( load ) ||
			vips_image_pio_input( load->real ) || 
			vips_foreign_load_iscompat( load->real, out ) ) {
			vips_operation_invalidate( VIPS_OPERATION( load ) ); 
			load->error = TRUE;

		}

	/* Set if a start function fails. We want to prevent the other starts
	 * from also triggering the load.
	 */
	gboolean error;