Chrome: Heap buffer overflow in WebP

There are two different possible attack vectors.

1. First, once you "local root" within the sandboxed process what can you compromise?

a. Can the session cookies be stolen? (Think gmail inline attachment rendering a malicious webp image).

b. Can you launch attack on other local network resources from within the compromised sandbox – read insecure filesystems, make tcp sockets to local resources etc – these are not prevented by OS sandboxing capabilities that Chrome depends on.

2. Second, can you probe for vulnerabilities in OS sandboxing that chrome depends on to break out of sandbox? On older unpatched OS versions this is definitely possible. These attack vectors may have been fixed in newer patches of OS already so you won't see a new CVE and patch for the same from the OS vendors.

Feels like it must have, otherwise what would be the point? But perhaps the CVE will come from Microsoft or some other vendor, if it's Renderer -> Kernel exploit.

Unrelated, what does "TFA" stand for? The only thing I can come up with is "the fucking article", which I really doubt is correct.

I'm not that familiar with Chrome's process isolation but I think that the renderer process that displays your Reddit.com tab would still render that image, even if it came from a different domain.

Chrome doesn't spin up another process to render the image and then transfer the pixels.

Wouldn’t the exploit code be able to bypass all those cross domain checks? As long as they are running in the same sandbox, right?

TBH I just used Reddit as an example of "site with images", I'm not actually concerned about my Reddit account/ think it'd be a good target.

The server is usually processing the images anyway, at the very least to remove EXIF metadata, but also to resize and compress the images for different resolutions.

You'd set up the server so that it's okay if it gets exploited. For example, maybe its only job is decoding images.

Then transcode in a different server.

Not transcoding user-uploaded files is borderline negligent.

Fair - I was really just thinking about the ton of things on my Linux systems which link against libwebp.

Correct, hence "[they] can't prevent all security bugs". They are written in C/C++ primarily because their primary users (e.g. web browsers) will still use C/C++ anyway, so the interoperability is more important. This is changing thanks to Rust and other languages, but the pace is still much slower than we hope.

Because browser vendors have already invested too much into the existing C/C++ code base. They can thus accept new code with the same degree of coding standard.

So every website would ship with its own codecs? There aren’t shared caches across domains. Why would anyone choose that much latency+badwidth+compute (which new require JS enabled) to use a new codec over the native ones? Certainly not the small–medium sites.

Nice! I did a quick proof of concept for JPEG 2000 a few years back but it really needed the sophistication of your multithreading and SIMD work.

First, I would submit that possibly people have learned from this - WebP is 12 years old and both the level of craft and tools have improved since then. If you suggested Rust you would have been proposing far more of a gamble than now.

Second, I don’t think this is preventing better formats due to safety conservatism - AVIF is at roughly 83% and climbing – but it does support the argument that the Chrome team has too much unilateral control over the web. I don’t think their opposition to JPEG-XL is solely based on the support cost.

Image decoding is complex

> For some datacenters, that 10% saving would be worth the effort and could push back costly maintenance to increase egress bandwidth.

No argument - my point was simply that very few sites on the web fall into that category.

> And I would argue that beside Facebook, the end user right clicking and saving the image for them to use in an inappropriate manner

That’s only true for a subset of sites, only to the extent that this wasn’t covered by fair use, and it came up enough that it was a common objection.

Interesting - I’ve never seen that much compared to mozjpeg and other optimized codecs. We also lazy-load large images.

> It's better that the codec is 100% safe but there's a tiny amount of unsafe glue code, [...]

I meant that many codec cannot be easily made in such way because memory allocation can occur here and there.

Wait, really? (Searches) Oh indeed [1]. I should note that the glue code is more than 1000 lines of C++ though. Still glad to see that it got into the production.

[1] https://source.chromium.org/chromium/chromium/src/+/main:thi...

As pointed out by another commenter, WUFFS would not itself allocate memory, which is fine, but would need some additional help to perform that task

After some more reading, I concluded that authors did think about this possibility but wrote a wrong code.

I have mentioned that there was the hard-coded maximum number of entries. This is derived from zlib's enough.c [1], which determines the maximum possible size of 2-level table for given number of symbols, N and the maximum allowed bit length (here 15). I've verified that those numbers indeed come from this program:

    for ((color_cache_size = 0; color_cache_size < 12; ++color_cache_size)); do
        # Note that color_cache_size = 0 entirely disables the color cache, so no symbols
        ./enough $((256 + 24 + (($color_cache_size > 0) << $color_cache_size))) 8 15
    done

So at the worst case, there are 256 + 24 + 2^11 = 2328 symbols possible, and the maximum table size is 2704. (Caveat: I couldn't verify values for color_cache_size >= 8 with the current version of enough.c. Probably the value was calculated with an alternative implementation using bigints.) So this bug cannot be found if any randomly constructed Huffman tree was thrown!

But enough.c states that it covers "all possible valid and complete prefix codes". In the other words it assumes that the Huffman tree has been already verified to be correct. If it's not the case, it is very easy to make much worse cases. For example `enough.c 280 8 15` returns 654, which is possible with the following tree:

    Len  Code range                            #         Root entry         Overhead  #
    ---  ------------------------------------  ---       -----------------  --------  ---
    1    0                                     1         0xxxxxxx           0         128
    9    10000000:0 .. 11110110:1              238       10000000-11110110  2^1       119
         11110111:0                            1         11110111           2^2       1
    10   11110111:10 .. 11110111:11            2
         11111000:00 .. 11111110:11            28        11111000-11111110  2^2       7
         11111111:00 .. 11111111:10            3         11111111           2^7       1
    11   11111111:110                          1
    12   11111111:1110                         1
    13   11111111:11110                        1
    15   11111111:1111100 .. 11111111:1111111  4

But the following partial tree should be able to reach 768 entries:

    Len  Code range                            #         Root entry         Overhead  #
    ---  ------------------------------------  ---       -----------------  --------  ---
    9    00000000:0                            1         00000000           2^7       1
    10   00000000:10                           1
    11   00000000:110                          1
    12   00000000:1110                         1
    13   00000000:11110                        1
    14   00000000:111110                       1
    15   00000000:1111110 .. 00000000:1111111  2
         00000001:0000000 .. 00000010:1111111  256       00000001-00000010  2^7       2
         00000011:0000000 .. 00000011:0001111  1         00000011           2^7       1

So the real issue here is that the lack of tree validation before the tree construction, I believe. I'm surprised that this check was not yet implemented (I actually checked libwebp to make sure that I wasn't missing one). Given this blind spot, an automated test based on the domain knowledge is likely useless to catch this bug.

[1] https://github.com/madler/zlib/blob/master/examples/enough.c

I think he is talking about NSO group, or whoever sold it to them. However, obviously that's unlikely to get revealed.

That's a general limitation with tagged pointers, I belive? Have you come across demonstrations of such exploits?

I'm going by the bugreports I see for a FOSS app I develop: https://github.com/celzero/rethink-app/issues/962

See: https://source.android.com/docs/security/test/memory-safety/...

Hard. Apple actually has a RFC for this where functions taking buffer-like parameters are adjusted to take an additional length parameter and then the compiler edits the code to plumb lengths through all of these things to insert a bounds check at use. This can work in many cases, but not all.

Rolling out this sort of change across a large codebase is hard as shit. While it sounds like it is mostly transparent, as soon as you run into a sufficiently large codebase all sorts of things start blowing up that you need to fix by hand before such a feature can be rolled out.

You can also do this with pointer tagging and some other techniques, but without hardware support this is amazingly slow. You can see just how much slower an asan build is, for example.

I think the short answer is "trivial in some cases, impossible in others". It's almost certainly possible that your compiler could inspect every allocation and tag each pointer with it internally. The problem comes with everything else - once you add loops and conditionals the length of that pointer can be all over the place. You'd basically need a symbolic executor tracking every pointer.

There are some big issues with this:

1. It's slow. Symbolic execution involves the interpretation of your program.

2. It would be imperfect and you'd likely have false positives.

3. It would likely be incomplete - for example, how would you handle the situation of only having a header?

So it's a good idea but it's very hard to make practically useful.

The easy way to do it involves changing the ABI of pointers so that they are now (address, bounds) pairs instead of just addresses. However, an awful lot of C code assumes that a pointer is just an address, and changing the ABI in this way will break the vast majority of non-trivial programs. (Witness the difficulty CHERI has in getting major software to work with it.)

You can, it's called valgrind (or more accurately, memcheck). And people don't use valgrind because it is slooooooow. Dynamic checking is useful, but not an ultimate way to go.

> remembers buffers' size

Where?

Once you have a bare pointer, you've lost track of what the original definition might have been, so you (the compiler / runtime / programmer) have no way of knowing that you've exceeded the size.

It's trivial but Big Tech is in bed with Big Hacker

Or it's hard like everyone keeps saying.

I'm going with the second option

> Only when Rust will change the default to keep all run-time checks also in release builds, it will be able to claim that by default it is safer than C.

Which checks are you thinking of? The only thing that comes to mind is that integer overflow wraps instead of panics, but given that bounds are checked, it is still going to be a panic or logic bug rather than a buffer overflow.

It sounds like you're referring to sanitizers.

1. Notably, some sanitizers are not intended for production use. I think this has changed a bit for asan but at one point it made vulns easier to exploit. These aren't mitigations.

2. They're extremely expensive. You need tons of bookkeeping for pointers for them to work. If you're willing to take that hit I don't really understand why you're using C, just use a GC'd language, which is probably going to be faster at that point.

> Only when Rust will change the default to keep all run-time checks also in release builds, it will be able to claim that by default it is safer than C.

The only thing Rust turns off at release is that unsigned integer overflows panic in debug but wrap on release. That wrap can not lead to memory unsafety.

FWIW it is not recommended to use asan+co for release builds. These are designed as debugging tools, if you use them in production builds they may actually open up new bugs. See also: https://www.openwall.com/lists/oss-security/2016/02/17/9

I don't think anyone has built anything practically usable that is meant for production, though it wouldn't be impossible to do so.

> the default for Rust releases is also to omit many run-time checks.

...because the type system and borrow checker satisfies them at compile-time?

The only checks that are omitted at runtime are:

- checks that are exhaustively proven to be unnecessary by LLVM - checks that can never be triggered in the absence of UB

You shouldn't be triggering UB checks at runtime. If you rely on these checks, you're relying on UB itself, when all UB should be provably impossible.

Do you release with `-fsanitize=address` or what?

Rust is novel because it bundles lots of existing ideas into one usable package. Even the borrow checking is not new, it's adapted and improved from research languages like Cyclone.

No one is saying that bounds checking is novel. It's really C and C++ that are novel, in that they are the only languages where bounds checking is not the default.

But if you're having the "memory safe replacement for C/C++" conversation it shouldn't surprise you that people bring up Rust.

Yea but did anyone else market hot water first? I think not!

Many bugs can be prevented.

Google has a history of releasing vulnerability details and PoCs before updates could be rolled out; CVE-2020-17087 was perhaps the worst example.

The two weeks grace period is in place for your run-of-the-mill 90-day disclosure time period, but for actively exploited bugs that extension period is up to three days.

After digging in deeper, it appears they have the fix rolling out 6 days after the report came in, so they're within their own deadline I suppose. Their statement about publishing the details doesn't mention releasing the details in a month like their own projects would, though: (https://www.bleepingcomputer.com/news/google/google-fixes-an...):

> "Access to bug details and links may be kept restricted until a majority of users are updated with a fix," Google said. "We will also retain restrictions if the bug exists in a third party library that other projects similarly depend on, but haven't yet fixed."

Google's inconsistencies when it comes to disclosure timelines irk me. "Wait until all third parties have also updated their software" isn't a luxury Google provides to others when they're the ones finding bugs. I'm all for swift disclosure timelines and pressure on manufacturers, but every Google team seems to have their own rules and guidelines written to serve themselves rather than general security.