https://cs.chromium.org/chromium/src/third_party/BUILD.gn?q=libjpeg_turbo+package:%5Echromium$+file:%5C.gn&l=17&dr=C looks like libjpeg_turbo should be the default.

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux%2Flinux_chromium_chromeos_ozone_rel_ng%2F350102%2F%2B%2Frecipes%2Fsteps%2Fgenerate_build_files__with_patch_%2F0%2Fstdout
dcheck_always_on = true
ffmpeg_branding = "ChromeOS"
goma_dir = "/b/c/goma_client"
is_component_build = false
is_debug = false
proprietary_codecs = true
strip_absolute_paths_from_debug_symbols = true
symbol_level = 1
target_os = "chromeos"
use_goma = true
use_ozone = true

looks like the default should be taken (?)

It's because of this snippet:


  if (is_chromeos) {
    # Robust JPEG decoding for the login screen.
    sources += [
      "chromeos/jpeg_codec_robust_slow.cc",
      "chromeos/jpeg_codec_robust_slow.h",
    ]
    deps += [ "//third_party/libjpeg" ]
  }

in ui/gfx/codec/BUILD.gn. This was done in https://codereview.chromium.org/569623002

Is it really more secure if this one file on one platform uses a different jpeg library than all the rest of chrome?

Also, codec depends on both libjpeg and libjpeg_turbo, which means that the aliasing in https://cs.chromium.org/chromium/src/third_party/libjpeg/jpeglibmangler.h?dr=CS&l=49 / https://cs.chromium.org/chromium/src/third_party/libjpeg/jpeglibmangler.h?type=cs&q=jpeg_CreateCompress&l=49 relies on compiler search paths details to work at all (but I think it does work).

If we need to keep the second jpeg library around, who owns updating it? It contains undefined behavior that we've fixed in the turbo variant.

The reason for this was that the wallpaper of any user can be displayed on the login screen for Chrome OS -- so using a malicious JPEG image could be an easy way for cross-user exploitation, something we very much try to prevent on Chrome OS. So we went with a more robust JPEG codec fro the login screen to try to mitigate this.

Can we fix IJG libjpeg?

>  So we went with a more robust JPEG codec fro the login screen to try to mitigate this.

Right, I'm wondering if it's actually more robust given that we use libjpeg_turbo virtually anywhere else.

> Can we fix IJG libjpeg?

Sure, who'd own doing that?

IJG libjpeg has definitely had significantly fewer bugs than libjpeg-turbo. That's what we were going for. The question here is "are there more places where we shouldn't use libjpeg-turbo" and not "should we get rid of it at a crucial place of cross-user exploitation."

I can probably fix libjpeg, or find someone in Chrome OS security to do it.

Looks like this particular problem is already fixed in IJG libjpeg v9b. We're at 6b, from 2009. It sounds like 7 and newer are no longer ABI-compatible (https://en.wikipedia.org/wiki/Libjpeg#History) which might be why we haven't updated, and it also sounds like 8 and 9 have stuff we don't want.

So it sounds like libjpeg is a dead end. Why is it considered more robust?

That's a significantly harder problem =(.

The reason to choose libjpeg was just the sheer difference in reported bugs, number of bugs found during fuzzing, and the like. If it's no longer a drop-in replacement, we might have to move back to libjpeg-turbo, which would make us less confident in the ability to prevent cross-user exploitation, but we might have to live with it.

What's the stuff in 8 and 9 we don't like?

Everything that's new and not security / warning fixes. https://lists.fedoraproject.org/pipermail/devel/2013-January/176400.html is from the original libjpeg author, saying that libjpeg 8 is a dead end, http://libjpeg-turbo.org/About/Jpeg-9 sounds like someone is pretty much trying to fork the jpeg format, etc. The Wikipedia page links to more, similar articles.

Fair. Can we temporarily patch libjpeg before we consider moving to libjpeg-turbo?

Yes, that should be easy.

Here's the v9b equivalent code for the code that's being warned on:

/*
 * Figure F.12: extend sign bit.
 * On some machines, a shift and sub will be faster than a table lookup.
 */

#ifdef AVOID_TABLES

#define BIT_MASK(nbits)   ((1<<(nbits))-1)
#define HUFF_EXTEND(x,s)  ((x) < (1<<((s)-1)) ? (x) - ((1<<(s))-1) : (x))

#else

#define BIT_MASK(nbits)   bmask[nbits]
#define HUFF_EXTEND(x,s)  ((x) <= bmask[(s) - 1] ? (x) - bmask[s] : (x))

static const int bmask[16] =	/* bmask[n] is mask for n rightmost bits */
  { 0, 0x0001, 0x0003, 0x0007, 0x000F, 0x001F, 0x003F, 0x007F, 0x00FF,
    0x01FF, 0x03FF, 0x07FF, 0x0FFF, 0x1FFF, 0x3FFF, 0x7FFF };

#endif /* AVOID_TABLES */

But it means we basically have our own libjpeg fork. We probably don't have fixes for
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-6629
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2012-2845
(not sure if those are serious, or we might've patched them already, this is from 2 seconds of googling for "cve libjpeg").

jorgelo: ping, this is now the only thing keeping a dependency on libjpeg (pdfium removed the one it had in the last few weeks). If you want to keep the libjpeg dep, please merge that patch, else let's use libjpeg-turbo everywhere.

Will patch.

Apologies about the silence here. Should I be able to repro this by fiddling with https://codereview.chromium.org/2774223005/ to also apply that CL for Chrome-on-Chrome-OS?

Right, just delete the green lines in https://codereview.chromium.org/2774223005/diff/60001/build/config/compiler/BUILD.gn

Have a working Chrome build, will hopefully fix this week.

Patch at https://chromium-review.googlesource.com/c/544198/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f0b84b500c5be5f74652b68ad01b39a9d82259ed

commit f0b84b500c5be5f74652b68ad01b39a9d82259ed
Author: Jorge Lucangeli Obes <jorgelo@google.com>
Date: Thu Jun 22 20:53:41 2017

Fix -Wshift-negative-value errors.

BUG= 706832 
TEST=Remove -Wno-shift-negative-values from Chrome OS build.
TEST=Build Chrome for Chrome OS, no warnings/errors.

Change-Id: Icb4f50452d87a8b2dc680752a2bb965f31c33639
Reviewed-on: https://chromium-review.googlesource.com/544198
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Jorge Lucangeli Obes <jorgelo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481657}
[modify] https://crrev.com/f0b84b500c5be5f74652b68ad01b39a9d82259ed/third_party/libjpeg/README.chromium
[add] https://crrev.com/f0b84b500c5be5f74652b68ad01b39a9d82259ed/third_party/libjpeg/google.shift-negative-values.patch
[modify] https://crrev.com/f0b84b500c5be5f74652b68ad01b39a9d82259ed/third_party/libjpeg/jdphuff.c

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24fe1e506d53c202a7903adc3c186acc3fe01273

commit 24fe1e506d53c202a7903adc3c186acc3fe01273
Author: Jorge Lucangeli Obes <jorgelo@google.com>
Date: Fri Jun 23 16:42:24 2017

libjpeg: Fix more -Wshift-negative-value errors.

BUG= 706832 
TEST=Remove -Wno-shift-negative-values from Chrome OS build.
TEST=Build Chrome for Chrome OS, no warnings/errors.
TEST=Build Chrome with chromeos=1, no warnings/errors.

Change-Id: I71936ec6d3a41c787febe1b83f865fed2cd322fa
Reviewed-on: https://chromium-review.googlesource.com/546035
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Jorge Lucangeli Obes <jorgelo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481926}
[modify] https://crrev.com/24fe1e506d53c202a7903adc3c186acc3fe01273/third_party/libjpeg/google.shift-negative-values.patch
[modify] https://crrev.com/24fe1e506d53c202a7903adc3c186acc3fe01273/third_party/libjpeg/jdhuff.c

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b83a794c51569c4a9c0197d745b372b14657a0de

commit b83a794c51569c4a9c0197d745b372b14657a0de
Author: Jorge Lucangeli Obes <jorgelo@google.com>
Date: Fri Jun 23 18:48:47 2017

Build: Don't set -Wno-shift-negative-value for CrOS.

Last user was removed in
https://chromium-review.googlesource.com/c/544198/.

BUG= 706832 
TEST=Build Chrome for Chrome OS.
CQ-DEPEND=CL:544198

Change-Id: I90e2f3087b0b2ef2926c223db305242be5ac6dcc
Reviewed-on: https://chromium-review.googlesource.com/544597
Commit-Queue: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481971}
[modify] https://crrev.com/b83a794c51569c4a9c0197d745b372b14657a0de/build/config/compiler/BUILD.gn

This is done. Some follow-up work tracked in issue 736134.