Heap buffer overflow on some images in v0.1.4

[Shnatsel]

I've run zune-jpeg on an imageboard dump I obtained specifically for testing image decoders. Some of the images trigger a segmentation fault with a heap buffer overflow with the x86 feature enabled, or a panic with that feature disabled. A sample image triggering the issue is attached: 1637407.jpg

Code to reproduce:

use zune_jpeg::Decoder;
use std::fs::File;
use std::io::BufReader;

fn main() -> std::io::Result<()> {
    let path = std::env::args().skip(1).next().unwrap();
    let mut decoder = Decoder::new();
    decoder.decode_file(path);
    Ok(())
}

You can build it in release mode as normal and observe a segmentation fault, or you can build it with address sanitizer to get more details: RUSTFLAGS=-Zsanitizer=address cargo +nightly build --release --target=x86_64-unknown-linux-gnu
(replace "x86_64-unknown-linux-gnu" with your native platform)

[etemesi254]

Acknowledged.

This led to a big discovery, seems like we are failing on a lot of progressive jpeg images with some specific parameters like the one above.

Investigating and will hopefully land a fix in the next few days.

Also is it okay if this is added to the test bench?

[Shnatsel]

Also is it okay if this is added to the test bench?

I'm not opposed to that, but I assume the image is copyrighted and I'm not the author, so I cannot grant this permission. So I think including it in a MIT OR Apache-2.0 repository is not possible.

[Shnatsel]

Thank you for the fix!

I see that the commit marked as fixing this performs unchecked arithmetic - there is multiplication of untrusted values, which may overflow.

A heap buffer overflow in a JPEG decoder is a pretty serious security vulnerability. Could we introduce some additional bounds checks, possibly even at the cost of a few % of performance?

Memory safety is the primary selling point of Rust implementations of binary decoders, and I would like to take steps to make sure further violations of it cannot happen.

[Shnatsel]

More specifically: since platform-independent code panicked instead of causing memory safety errors, it sounds prudent to add the same bounds checks to the x86-specific code.

[etemesi254]

I see that the commit marked as fixing this performs unchecked arithmetic - there is multiplication of untrusted values, which may overflow.

Should we check for multiplication overflow?? And panic if so??

A heap buffer overflow in a JPEG decoder is a pretty serious security vulnerability. Could we introduce some additional bounds checks, possibly even at the cost of a few % of performance?

I'm reopening this issue and will close it when I have added checks to unsafe code, I believe it's idct and upsampling that don't have checks.

For safety I have no issue with that, that part is parallelized so it should not see perf losses.

The only issue is that it sadly can't be propagated to the caller, since it's in another thread, so it means it's a hard assert, or maybe there is a way to pass errors to the upper caller using scoped thread pools?

[Shnatsel]

Should we check for multiplication overflow?? And panic if so??

This is only necessary if this value is later used for indexing into a slice without bounds checks. I believe the correct way to resolve this is to add bounds checks on indexing, instead of trying to check for overflow on every multiplication.

The only issue is that it sadly can't be propagated to the caller, since it's in another thread, so it means it's a hard assert, or maybe there is a way to pass errors to the upper caller using scoped thread pools?

Can the closure spawned on the thread pool return a value? If so, you can simply use that to propagate the result.

In case that's not possible, a scoped thread pool should be able to borrow some variables from the scope it is in, so you can simply set a variable in the parent thread's scope to Result::Ok<()> at the end of the execution from the worker thread, and initialize it with some error value. You will need one for each worker thread.

[etemesi254]

I am not so sure about it. I tried some variations and all had the xompiler complaining about mismatched types But since it's using `scoped_threadpool`, you are welcome to try and see if a POC may work on your side.

…

On Thu, 30 Jun 2022, 21:07 Shnatsel, ***@***.***> wrote: Should we check for multiplication overflow?? And panic if so?? This is only necessary if this value is later used for indexing into a slice without bounds checks. I believe the correct way to resolve this is to add bounds checks on indexing, instead of trying to check for overflow on every multiplication. The only issue is that it sadly can't be propagated to the caller, since it's in another thread, so it means it's a hard assert, or maybe there is a way to pass errors to the upper caller using scoped thread pools? Can the closure spawned on the thread pool return a value? If so, you can simply use that to propagate the result. In case that's not possible, a scoped thread pool should be able to borrow some variables from the scope it is in, so you can simply set a variable in the parent thread's scope to Result::Ok<()> at the end of the execution from the worker thread, and initialize it with some error value. You will need one for each worker thread. — Reply to this email directly, view it on GitHub <#10 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFZRVEZJN5IFDCAE3IVRBULVRXO7JANCNFSM5ZGLU3CA> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[Shnatsel]

If all else fails, you can always panic in the thread and then catch_unwind. This is even easier to do on the thread boundary when using the std thread spawning APIs.

FWIW the standard library support for scoped threads has been stabilized in 1.63, set to release 6 weeks from now.

But I'll take a panic over a heap buffer overflow any day, so an assert without any panic catching would still be a big improvement.

[etemesi254]

The rust one solves the problem since it has a return type on the trait bound We can assert now and return errors later when 1.63 becomes msrv

…

On Thu, 30 Jun 2022, 21:17 Shnatsel, ***@***.***> wrote: If all else fails, you can always panic in the thread and then catch_unwind <https://doc.rust-lang.org/stable/std/panic/fn.catch_unwind.html>. This is even easier to do on the thread boundary when using the std thread spawning APIs. FWIW the standard library support for scoped threads <https://doc.rust-lang.org/stable/std/thread/struct.Builder.html#method.spawn_scoped> has been stabilized in 1.63, set to release 6 weeks from now. But I'll take a panic over a heap buffer overflow any day, so an assert without any panic catching would still be a big improvement. — Reply to this email directly, view it on GitHub <#10 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFZRVE7C3GCFXJLQWV2WCLLVRXQCLANCNFSM5ZGLU3CA> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[etemesi254]

• IDCT checks, commit 1375b30
• Upsampler checks 1f92bac

[etemesi254]

Done

[Shnatsel]

Thank you!

I'll keep running tests on my datasets and report any issues I find.

[etemesi254]

Appreciated