### Summary
An exploitable heap overflow vulnerability exists in the gdk_pixbuf__jpeg_image_load_increment functionality of Gdk-Pixbuf 2.36.6. 
A specially crafted jpeg file can cause a heap overflow resulting in remote code execution. An attacker can send a file or url to trigger this vulnerability. 


### Tested Versions

Gdk-Pixbuf 2.36.6 commit: aba8d88798dfc2f3856ea0ddda14b06174bbb2bc
libjpeg-turbo 1.5.2   

### Product URLs

[https://developer.gnome.org/gdk-pixbuf/](https://developer.gnome.org/gdk-pixbuf/)


### CVSSv3 Score

8.8 - CVSS:3.0/AV:N/AC:L/PR:N/UI:R/S:U/C:H/I:H/A:H


### CWE

CWE-131: Incorrect Calculation of Buffer Size

### Details

	Gdk-Pixbuf is a toolkit for image loading and pixel buffer manipulation used in various type of desktop applications: image viewers(GNOME thumbnailer), web browser (Chromium, Firefox),media players (VLC), (...).
A vulnerability exists in the JPEG parser: it's based on wrong calculation size for an output buffer in the `gdk_pixbuf__jpeg_image_load_increment` function which later causes a heap overflow
during file content conversion inside libjpeg `null_convert` function. Because the file necessary to trigger this vulnerability is quite small we can present its entire content here:

	Offset      0  1  2  3  4  5  6  7   8  9  A  B  C  D  E  F

	00000000   FF D8 FF DB 00 43 01 28  2D 8C A5 F8 F8 F8 F8 F8   ����.C.(-�������
	00000010   F8 F8 F8 F8 F8 F8 F8 F8  F8 F8 F8 F8 F8 F8 F8 F8   ����������������
	00000020   F8 F8 64 F8 F8 F8 F8 F8  F8 11 11 11 11 11 11 11   ��d������.......
	00000030   11 11 11 11 11 11 11 11  11 11 11 11 11 11 11 11   ................
	00000040   11 11 11 11 11 11 11 FF  CA 00 23 08 00 01 00 2F   .......��.#..../
	00000050   09 01 11 01 02 11 01 03  11 01 FF 32 11 11 11 11   ..........�2....
	00000060   11 11 11 11 11 11 11 11  11 11 11 11 12 22 41 FF   ............."A�
	00000070   DA 00 0C 03 01 10 02 11  03 00 00 00 03 01 00 02   �...............
	00000080   11 FF 80 00                                        .��. 

We can notice that there are a couple JPEG markers but we will focus on the one directly related with vulnerability. That marker is 0xFFCA which starts at offset : 0x47 and has a size of 0x23 bytes.
Below are important values read from this section (marker):

	cinfo {
	...
	data_precision = 0x8,
	image_width = 0x2f,
	image_height = 0x1,
	num_components = 0x9,
	...
	output_width = 0x2f,
	output_height = 0x1,
	out_color_components = 0x9,
	output_components = 0x9, 	
	}

	-------------  line 255 jdmarker.c -------------------
	INPUT_BYTE(cinfo, cinfo->data_precision, return FALSE);
	INPUT_2BYTES(cinfo, cinfo->image_height, return FALSE);
	INPUT_2BYTES(cinfo, cinfo->image_width, return FALSE);
	INPUT_BYTE(cinfo, cinfo->num_components, return FALSE);
	---------------------------------------------------------

To observe how the vulnerability manifests itself, we will use the `pixuf-read` test application and our malicious JPEG as input file. Executed under `valgrind` control we see the following output:

	==5058== Invalid write of size 1
	==5058== at 0x75278C7: null_convert (jdcolor.c:408)
	==5058== by 0x753F506: sep_upsample (jdsample.c:98)
	==5058== by 0x7530660: process_data_simple_main (jdmainct.c:311)
	==5058== by 0x75234A5: jpeg_read_scanlines (jdapistd.c:282)
	==5058== by 0x72F64AA: gdk_pixbuf__jpeg_image_load_lines (io-jpeg.c:901)
	==5058== by 0x72F6FB2: gdk_pixbuf__jpeg_image_load_increment (io-jpeg.c:1201)
	==5058== by 0x4E4B44C: gdk_pixbuf_loader_load_module (gdk-pixbuf-loader.c:443)
	==5058== by 0x4E4BE23: gdk_pixbuf_loader_close (gdk-pixbuf-loader.c:811)
	==5058== by 0x400AA0: test_loader (pixbuf-read.c:35)
	==5058== by 0x400BD4: main (pixbuf-read.c:75)
	==5058== Address 0x6f3cbd0 is 0 bytes after a block of size 144 alloc'd
	==5058== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
	==5058== by 0x4E4104A: gdk_pixbuf_new (gdk-pixbuf.c:464)
	==5058== by 0x72F6B93: gdk_pixbuf__jpeg_image_load_increment (io-jpeg.c:1093)
	==5058== by 0x4E4B44C: gdk_pixbuf_loader_load_module (gdk-pixbuf-loader.c:443)
	==5058== by 0x4E4BE23: gdk_pixbuf_loader_close (gdk-pixbuf-loader.c:811)
	==5058== by 0x400AA0: test_loader (pixbuf-read.c:35)
	==5058== by 0x400BD4: main (pixbuf-read.c:75)
	==5058==


So, generally there is a buffer allocated inside `gdk_pixbuf__jpeg_image_load_increment` and later overflowed in `null_convert`.
Let's take a closer look at where this allocation takes place:

	Line 1093			context->pixbuf = gdk_pixbuf_new (GDK_COLORSPACE_RGB, 
	Line 1094							  cinfo->output_components == 4 ? TRUE : FALSE,
	Line 1095							  8, 
	Line 1096							  cinfo->output_width,
	Line 1097							  cinfo->output_height);


The important variable here is `cinfo->output_components`. Its value is equal to `0x9` so as a result of the comparison, second argument passed to `gdk_pixbuf_new` will be a `False` boolean
flag. Going inside this function we see:

	Line 440		gdk_pixbuf_new (GdkColorspace colorspace, 
	Line 441						gboolean      has_alpha,
	Line 442						int           bits_per_sample,
	Line 443						int           width,
	Line 444						int           height)
	Line 445		{
	Line 446			guchar *buf;
	Line 447			unsigned int channels;
	Line 448			unsigned int rowstride;
	(...)
	Line 455		channels = has_alpha ? 4 : 3;
	Line 456
	Line 457		/* Overflow? */
	Line 458		if (width > (G_MAXUINT - 3) / channels)
	Line 459			return NULL;
	Line 460
	Line 461		/* Always align rows to 32-bit boundaries */
	Line 462		rowstride = (width * channels + 3) & ~3;
	Line 463
	Line 464		buf = g_try_malloc_n (height, rowstride);
	
We see that the `channels` variable which is used in the calculation of the `rowstride` value at `line 462`.
The size of the allocated buffer in our case equals 0x90.

Now going straight to the function where the heap overflow appears we see these lines:

	jdcolor.c libjpeg-turbo
	--------------------------------
	Line 363		METHODDEF(void)
	Line 364		null_convert (j_decompress_ptr cinfo,
	Line 365					  JSAMPIMAGE input_buf, JDIMENSION input_row,
	Line 366					  JSAMPARRAY output_buf, int num_rows)
	(...)
	Line 370  register int num_components = cinfo->num_components;
	(...)	
	Line 374  if (num_components == 3) {
	(...)	  
	Line 387  } else if (num_components == 4) {	
	(...)	
	Line 402  } else {
	Line 403		while (--num_rows >= 0) {
	Line 404		  for (ci = 0; ci < num_components; ci++) {
	Line 405			inptr = input_buf[ci][input_row];
	Line 406			outptr = *output_buf;
	Line 407			for (col = 0; col < num_cols; col++) {
	Line 408			  outptr[ci] = inptr[col];
	Line 409			  outptr += num_components;
	Line 410			}
	Line 411		  }
	Line 412		  output_buf++;
	Line 413		  input_row++;
	Line 414		}	

Based on the `num_components` value, a different loop is used to copy data from the input buffer to the output one. We land of course in `if` branch starting at `Line 403` because our 
`num_components` equals 0x9. Then we see the following:

Line 403: the `while` loop condition is based on `num_rows` which equals 0x1 
Line 404: the `for` is controlled by `num_components` which equals 0x9 and finally 
Line 407: tihs for loop is controlled by `num_cols` which equals 0x2f.

It is clear now why the overflow occurs. The Gdk-pixbuf developers assume only two scenarios of `number of components`: 3 or 4 and based on those two potential values, allocated the buffer.
Here we land in scenario where `num_components` equals 0x9 that will cause an out of buffer write at `line 408` in will result in heap corruption.
	
### Crash Information 

	Program received signal SIGSEGV, Segmentation fault.
	[----------------------------------registers-----------------------------------]
	RAX: 0x80
	RBX: 0x7ffff4702000 --> 0x0
	RCX: 0x7fffffffda40 --> 0x7ffff4701f70 --> 0xffffffffffffff80
	RDX: 0x7ffff4702000 --> 0x0
	RSI: 0x7ffff46fc758 --> 0x7ffff46fc840 --> 0x7ffff47b4f80 --> 0x8080808080808080
	RDI: 0x7ffff4d70c10 --> 0x7ffff4d70e88 --> 0x7ffff4a7cbb0 (: push rbp)
	RBP: 0x7fffffffd910 --> 0x7fffffffd970 --> 0x7fffffffd9c0 --> 0x7fffffffda00 --> 0x7fffffffda80 (0x00007fffffffdb60)
	RSP: 0x7fffffffd8e8 --> 0x0
	RIP: 0x7ffff48128c7 (mov BYTE PTR [rdx],al)
	R8 : 0x1
	R9 : 0x0
	R10: 0x5b ('[')
	R11: 0x7ffff483d24c (: push rbp)
	R12: 0x10
	R13: 0x9 ('\t')
	R14: 0x7ffff47b4f80 --> 0x8080808080808080
	R15: 0x0
	EFLAGS: 0x10206 (carry PARITY adjust zero sign trap INTERRUPT direction overflow)
	[-------------------------------------code-------------------------------------]
	0x7ffff48128be: mov eax,r12d
	0x7ffff48128c1: add rax,r14
	0x7ffff48128c4: movzx eax,BYTE PTR [rax]
	=> 0x7ffff48128c7: mov BYTE PTR [rdx],al
	0x7ffff48128c9: movsxd rax,r13d
	0x7ffff48128cc: add rbx,rax
	0x7ffff48128cf: add r12d,0x1
	0x7ffff48128d3: cmp r12d,DWORD PTR [rbp-0x2c]
	[------------------------------------stack-------------------------------------]
	0000| 0x7fffffffd8e8 --> 0x0
	0008| 0x7fffffffd8f0 --> 0x400950 (: xor ebp,ebp)
	0016| 0x7fffffffd8f8 --> 0x7fffffffdd70 --> 0x2
	0024| 0x7fffffffd900 --> 0x0
	0032| 0x7fffffffd908 --> 0x0
	0040| 0x7fffffffd910 --> 0x7fffffffd970 --> 0x7fffffffd9c0 --> 0x7fffffffda00 --> 0x7fffffffda80 (0x00007fffffffdb60)
	0048| 0x7fffffffd918 --> 0x7ffff482a507 (mov rax,QWORD PTR [rbp-0x50])
	0056| 0x7fffffffd920 --> 0x7fffffffd9f4 --> 0xe399da0000000000
	[------------------------------------------------------------------------------]
	Legend: code, data, rodata, value
	Stopped reason: SIGSEGV
	0x00007ffff48128c7 in null_convert (cinfo=0x7ffff4d70c10, input_buf=0x7ffff46fc758, input_row=0x0, output_buf=0x7fffffffda40, num_rows=0x0) at jdcolor.c:408
	408 outptr[ci] = inptr[col];
	gdb-peda$ bt
	#0 0x00007ffff48128c7 in null_convert (cinfo=0x7ffff4d70c10, input_buf=0x7ffff46fc758, input_row=0x0, output_buf=0x7fffffffda40, num_rows=0x0) at jdcolor.c:408
	#1 0x00007ffff482a507 in sep_upsample (cinfo=0x7ffff4d70c10, input_buf=0x7ffff46fe3f0, in_row_group_ctr=0x7ffff46fe444, in_row_groups_avail=0x8, output_buf=0x7fffffffda40, out_row_ctr=0x7fffffffd9f4, out_rows_avail=0x1) at jdsample.c:98
	#2 0x00007ffff481b661 in process_data_simple_main (cinfo=0x7ffff4d70c10, output_buf=0x7fffffffda40, out_row_ctr=0x7fffffffd9f4, out_rows_avail=0x1) at jdmainct.c:311
	#3 0x00007ffff480e4a6 in jpeg_read_scanlines (cinfo=0x7ffff4d70c10, scanlines=0x7fffffffda40, max_lines=0x1) at jdapistd.c:282
	#4 0x00007ffff4a7e4ab in gdk_pixbuf__jpeg_image_load_lines (context=0x7ffff4d70bd0, error=0x7fffffffdbd8) at io-jpeg.c:901
	#5 0x00007ffff4a7efb3 in gdk_pixbuf__jpeg_image_load_increment (data=0x7ffff4d70bd0, buf=0x7ffff5273fd4 "\377\330\377", , size=0x34a, error=0x7fffffffdbd8) at io-jpeg.c:1201
	#6 0x00007ffff79ad44d in gdk_pixbuf_loader_load_module (loader=0x7ffff546be00, image_type=0x0, error=0x7fffffffdbd8) at gdk-pixbuf-loader.c:443
	#7 0x00007ffff79ade24 in gdk_pixbuf_loader_close (loader=0x7ffff546be00, error=0x7fffffffdc80) at gdk-pixbuf-loader.c:811
	#8 0x0000000000400aa1 in test_loader (bytes=0x7ffff53e9cb5 "\377\330\377", , len=0x34a, err=0x7fffffffdc80) at pixbuf-read.c:35
	#9 0x0000000000400bd5 in main (argc=0x2, argv=0x7fffffffdd78) at pixbuf-read.c:75
	#10 0x00007ffff70c5830 in __libc_start_main (main=0x400ad8 , argc=0x2, argv=0x7fffffffdd78, init=, fini=, rtld_fini=, stack_end=0x7fffffffdd68) at ../csu/libc-start.c:291
	#11 0x0000000000400979 in _start ()
	gdb-peda$

### Credit 

Discovered by Marcin 'Icewall' Noga of Cisco Talos.
http://talosintelligence.com/vulnerability-reports/

Created attachment 355451 [details]
crashing file, password "crash"

Created attachment 355452 [details] [review]
potential patch. It's untested, however.

This is a quickly developed and untested patch.
I assumed that "3" or "4" are the only values gdk-pb can handle, because I see those values being used.

Review of attachment 355452 [details] [review]:

Yes, that looks correct. Does this avoid the crash ?

Review of attachment 355452 [details] [review]:

> JPEG loader: explicitly check output components for "3" or
 "4"

Please follow the commit subjects already used (so "jpeg: " is preferred).

> which analysed a buffer overwrite from libjpeg-turbo in a buffer allocated by
> This ultimately leads to libjpg-turbo assuming a size of the buffer we

libjpeg-turbo or libjpg-turbo?

::: gdk-pixbuf/io-jpeg.c
@@ +1091,3 @@
 			jpeg_calc_output_dimensions (cinfo);
+
+			gboolean has_alpha;

We try to declare at the top of blocks, not in the middle of it.

@@ +1097,3 @@
+			    has_alpha = TRUE;
+			} else {
+					g_set_error_literal (error,

Nitpick, but the indentation is out of whack here and/or in the blocks above.

@@ +1100,3 @@
+                                         GDK_PIXBUF_ERROR,
+                                         GDK_PIXBUF_ERROR_CORRUPT_IMAGE,
+                                         _("Components neither 3 nor 4."));

Should be:
"Unsupported number of color components (%d)"
similarly to the:
"Unsupported JPEG color space (%s)"
error.

Note that we usually also want a test case added to the test suite for each potential crasher.

(In reply to Bastien Nocera from comment #5)
> Review of attachment 355452 [details] [review] [review]:
> 
> > JPEG loader: explicitly check output components for "3" or
>  "4"
> 
> Please follow the commit subjects already used (so "jpeg: " is preferred).
> 
right.

> > which analysed a buffer overwrite from libjpeg-turbo in a buffer allocated by
> > This ultimately leads to libjpg-turbo assuming a size of the buffer we
> 
> libjpeg-turbo or libjpg-turbo?
> 
heh. good catch, thanks.

> ::: gdk-pixbuf/io-jpeg.c
> @@ +1091,3 @@
>  			jpeg_calc_output_dimensions (cinfo);
> +
> +			gboolean has_alpha;
> 
> We try to declare at the top of blocks, not in the middle of it.
>
fair enough. I thought we're past C89.
 
> @@ +1097,3 @@
> +			    has_alpha = TRUE;
> +			} else {
> +					g_set_error_literal (error,
> 
> Nitpick, but the indentation is out of whack here and/or in the blocks above.
hm. yeah, tabs and space mixes. Not good.

> 
> @@ +1100,3 @@
> +                                         GDK_PIXBUF_ERROR,
> +                                         GDK_PIXBUF_ERROR_CORRUPT_IMAGE,
> +                                         _("Components neither 3 nor 4."));
> 
> Should be:
> "Unsupported number of color components (%d)"
> similarly to the:
> "Unsupported JPEG color space (%s)"
> error.
yeah, cool.

Created attachment 355483 [details] [review]
potential patch. It's untested, however.

Created attachment 355484 [details] [review]
added the crashing file

Review of attachment 355483 [details] [review]:

I'll fix those up. Is there an embargo on this vulnerability?

::: gdk-pixbuf/io-jpeg.c
@@ +1093,3 @@
+
+			if (cinfo->output_components == 3) {
+			    has_alpha = FALSE;

that's 4 spaces.

@@ +1095,3 @@
+			    has_alpha = FALSE;
+			} else if (cinfo->output_components == 4) {
+			    has_alpha = TRUE;

that's also 4 spaces.

@@ +1097,3 @@
+			    has_alpha = TRUE;
+			} else {
+					g_set_error (error,

This has one tab too much.

(In reply to Bastien Nocera from comment #10)
> Review of attachment 355483 [details] [review] [review]:
> 
> I'll fix those up.
Cool. Thanks.

> Is there an embargo on this vulnerability?
We haven't set any.  And I don't think we necessarily need any.
The earlier we get the fix out, the better.

Created attachment 355504 [details] [review]
tests: Split off failing test into 2 separate tests

One with the tiniest of buffers, one with the biggest one possible.
This catches some crashes that weren't caught with the original "1 byte"
buffers test.

This makes it possible to reproduce the crasher in

Created attachment 355505 [details] [review]
jpeg: Throw error when number of color components is unsupported

Explicitly check "3" or "4" output color components.

gdk-pixbuf assumed that the value of output_components to be either
3 or 4, but not an invalid value (9) or an unsupported value (1).

The way the buffer size was deduced was using a naive "== 4" check,
with a 1, 3 or 9 color component picture getting the same buffer size,
a size just sufficient for 3 color components, causing invalid writes
later when libjpeg-turbo was decoding the image.

CVE-2017-2862

Sent by from Marcin 'Icewall' Noga of Cisco Talos

Attachment 355504 [details] pushed as 4ffc78b - tests: Split off failing test into 2 separate tests
Attachment 355505 [details] pushed as c2a40a9 - jpeg: Throw error when number of color components is unsupported

Make public

Ugh, this broke handling of grayscale JPEGs. Ah, I see that Bug 785171 has been filed against that.