embedded-graphics/tinybmp

Crash / Out of Bounds read in RawBmp

symeonp opened this issue · 11 comments

symeonp commented
  • Version of tinybmp in use (if applicable): tinybmp 0.3.3

Crash / Out Of Bounds Read in image_data when a malformed BMP is provided

└─$ ./target/release/tinypoc 
thread 'main' panicked at 'range start index 536871106 out of range for slice of length 54', library/core/src/slice/index.rs:52:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

And the PoC in Rust:

use embedded_graphics::prelude::*;
use tinybmp::{RawBmp, Bpp, Header, RawPixel, RowOrder};

fn main() {
    let bmp = RawBmp::from_slice(include_bytes!("./crasher.bmp"))
    .expect("Failed to parse BMP image");
}

I've discovered this issue while fuzzing the RawBmp functions - the issue here is that there is no user validation/sanitasation within the header values and as such when a malformed BMP such as the following is provided

> hexdump -C /Users/symeonp/Desktop/fuzz_target_1/crasher.bmp
00000000  42 4d 00 00 c2 c2 c2 c2  c2 c2 c2 00 00 20 28 00  |BM..�������.. (.|
00000010  00 00 20 00 00 00 00 20  28 00 00 00 20 00 00 00  |.. .... (... ...|
00000020  00 00 00 00 08 00 03 00  00 00 00 00 00 00 00 00  |................|
00000030  00 00 00 cb 08 08                                 |...�..|
00000036

after parsing methods the following can be observed:

●    35      /// [`pixels`]: #method.pixels
     36      pub fn from_slice(bytes: &'a [u8]) -> Result<Self, ParseError> {
     37          let (_remaining, (header, color_table)) =
●    38              Header::parse(bytes).map_err(|_| ParseError::Header)?;
     39  
●→   40          let image_data = &bytes[header.image_data_start..];
     41  
     42          Ok(Self {
     43              header,
     44              color_table,
     45              image_data,
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── threads ────
[#0] Id 1, Name: "tinypoc", stopped 0x55555555cde4 in tinybmp::raw_bmp::RawBmp::from_slice (), reason: BREAKPOINT
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── trace ────
[#0] 0x55555555cde4 → tinybmp::raw_bmp::RawBmp::from_slice(bytes=&[u8] {
  data_ptr: 0x55555559a000,
  length: 0x36
})
[#1] 0x55555555cc2e → tinypoc::main()
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
gef➤  p bytes
$11 = &[u8] {
  data_ptr: 0x55555559a000,
  length: 0x36
}
gef➤  p header
$12 = tinybmp::header::Header {
  file_size: 0xc2c20000,
  image_data_start: 0x200000c2,
  image_size: embedded_graphics_core::geometry::size::Size {
    width: 0x20,
    height: 0x282000
  },
  bpp: tinybmp::header::Bpp::Bits32,
  image_data_len: 0x80000,
  channel_masks: core::option::Option<tinybmp::header::ChannelMasks>::None,
  row_order: tinybmp::header::RowOrder::BottomUp
}
gef➤  p/d bytes
$13 = &[u8] {
  data_ptr: 93824992518144,
  length: 54
}
gef➤  p/d header.image_data_start
$14 = 536871106
gef➤  c

So image_data_start now holds the user controlled value 0x200000c2 / 536871106 when the total bytes is 54, thus assigning image_data leads to an out of bounds read where sure enough Rust does protect accessing it:

gef➤  c
Continuing.
thread 'main' panicked at 'range start index 536871106 out of range for slice of length 54', library/core/src/slice/index.rs:52:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This might lead to a Denial-Of-Service condition though - let me know your thoughts.
Thanks

Test case

crasher.bmp.zip(See attached)

jamwaffles commented

Thanks for opening this issue, as well as providing a failing test case and explanation - it's much appreciated. My immediate thought is to validate the header during parsing which I think would solve this DoS. Can you see issues with this approach?

symeonp commented

Hi James,

Yes I agree - validating during the parsing and bailing out would be the best way I guess.
Can happily have a look and fuzz it again once you think you've done some changes!

Thanks

jamwaffles commented

I've had some time to actually look through the code (and apologies for not doing this sooner). It turns out this bug was fixed in cf9fae2, although this is not yet released as there are some breaking changes that I want to merge before pushing 0.4.0 out. Could you re-fuzz with the latest commit in master and see if the OOB is fixed? A quick unit test on my side returns an error as intended.

If it's fixed, feel free to close this issue.

symeonp commented

Hi James,

Nice one, please bear with me I'll have a look tomorrow on the bank holiday and get back to you..

Thanks

symeonp commented

Hello James,

Apologies for getting back to you only now, had a bit of a hectic week! So unfortunately that codes leads to this new bug:

INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 4227520569
INFO: Loaded 1 modules   (12508 inline 8-bit counters): 12508 [0x55b8cf2ee360, 0x55b8cf2f143c), 
INFO: Loaded 1 PC tables (12508 PCs): 12508 [0x55b8cf2f1440,0x55b8cf322200), 
fuzz/target/x86_64-unknown-linux-gnu/release/fuzz_target_1: Running 1 inputs 1 time(s) each.
Running: fuzz/artifacts/fuzz_target_1/minimized-from-a6482a4ec1f7ae0cb9b60aee35a4cbffe009fd5f
thread '<unnamed>' panicked at 'attempt to negate with overflow', /home/kali/Desktop/tinybmp/src/header/dib_header.rs:109:52
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
==19088== ERROR: libFuzzer: deadly signal
    #0 0x55b8cf0820d1 in __sanitizer_print_stack_trace /rustc/llvm/src/llvm-project/compiler-rt/lib/asan/asan_stack.cpp:87:3
    #1 0x55b8cf161b70 in fuzzer::PrintStackTrace() (/home/kali/Desktop/tinybmp/fuzz/target/x86_64-unknown-linux-gnu/release/fuzz_target_1+0x1e3b70) (BuildId: 5c0dc81be261ddc2fa3566d23d776022b24f51ba)
    #2 0x55b8cf134e65 in fuzzer::Fuzzer::CrashCallback() (/home/kali/Desktop/tinybmp/fuzz/target/x86_64-unknown-linux-gnu/release/fuzz_target_1+0x1b6e65) (BuildId: 5c0dc81be261ddc2fa3566d23d776022b24f51ba)
    #3 0x7fa5e6e091ff  (/lib/x86_64-linux-gnu/libpthread.so.0+0x131ff) (BuildId: 56ddcf61fc6f83724f028b494a5cf5d29732df62)
    #4 0x7fa5e6b0c8a0 in __libc_signal_restore_set signal/../sysdeps/unix/sysv/linux/internal-signals.h:105:3
    #5 0x7fa5e6b0c8a0 in raise signal/../sysdeps/unix/sysv/linux/raise.c:47:3
    #6 0x7fa5e6af6545 in abort stdlib/abort.c:79:7
    #7 0x55b8cf1c55e6 in std::sys::unix::abort_internal::hde619e458afd9651 /rustc/263edd43c5255084292329423c61a9d69715ebfa/library/std/src/sys/unix/mod.rs:292:14
    #8 0x55b8ceff5576 in std::process::abort::hd83a06c6cebea559 /rustc/263edd43c5255084292329423c61a9d69715ebfa/library/std/src/process.rs:2119:5
    #9 0x55b8cf12f265 in libfuzzer_sys::initialize::_$u7b$$u7b$closure$u7d$$u7d$::hd1b6eb90a6d3a551 (/home/kali/Desktop/tinybmp/fuzz/target/x86_64-unknown-linux-gnu/release/fuzz_target_1+0x1b1265) (BuildId: 5c0dc81be261ddc2fa3566d23d776022b24f51ba)
    #10 0x55b8cf1bb225 in std::panicking::rust_panic_with_hook::hd1b8a285eaf7d0b1 /rustc/263edd43c5255084292329423c61a9d69715ebfa/library/std/src/panicking.rs:702:17
    #11 0x55b8cf1bb038 in std::panicking::begin_panic_handler::_$u7b$$u7b$closure$u7d$$u7d$::h09ea81c898720b2f /rustc/263edd43c5255084292329423c61a9d69715ebfa/library/std/src/panicking.rs:586:13
    #12 0x55b8cf1b8263 in std::sys_common::backtrace::__rust_end_short_backtrace::h76ccd99a822922a0 /rustc/263edd43c5255084292329423c61a9d69715ebfa/library/std/src/sys_common/backtrace.rs:138:18
    #13 0x55b8cf1bada1 in rust_begin_unwind /rustc/263edd43c5255084292329423c61a9d69715ebfa/library/std/src/panicking.rs:584:5
    #14 0x55b8ceff6842 in core::panicking::panic_fmt::h14c8dbaaa973d6e3 /rustc/263edd43c5255084292329423c61a9d69715ebfa/library/core/src/panicking.rs:142:14
    #15 0x55b8ceff670c in core::panicking::panic::h96f99f3ab76f1beb /rustc/263edd43c5255084292329423c61a9d69715ebfa/library/core/src/panicking.rs:48:5
    #16 0x55b8cf0e97bd in tinybmp::header::dib_header::DibHeader::parse::h71af701e59ad93c2 (/home/kali/Desktop/tinybmp/fuzz/target/x86_64-unknown-linux-gnu/release/fuzz_target_1+0x16b7bd) (BuildId: 5c0dc81be261ddc2fa3566d23d776022b24f51ba)

It looks like the attached test case crashes at tinybmp::header::dib_header::DibHeader::parse with 'attempt to negate with overflow',
Do you want me to have a look and try helping you analysing it? More than happy to do it!

> hexdump -C /Users/symeonp/Desktop/minimized-crash-overflow.bmp
00000000  42 4d 42 ff ff ff 01 00  00 00 00 00 00 00 28 00  |BMB���........(.|
00000010  00 00 00 00 00 00 00 00  00 80 00 32 18 00 00 00  |...........2....|
00000020  00 00 00 00 fe fe fe 0d  0d 00 00 00 00 03 00 00  |....���.........|
00000030  00 00 80 00 00 00                                 |......|
00000036

minimized-crash-overflow.bmp.zip

rfuest commented

Did you use the latest master version? This might have been fixed in dbce097. In my test the file did still cause a panic, but not in DibHeader::parse.

symeonp commented

No, I have used cf9fae2 as per James above comment.

symeonp commented

Ah, I got it wrong, let me try the latest version from master...

symeonp commented

Here's the new crash on latest master version:

000\000\000\000\004"-
thread '<unnamed>' panicked at 'attempt to multiply with overflow', /home/kali/Desktop/tinybmp/src/raw_bmp.rs:40:27
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
==19670== ERROR: libFuzzer: deadly signal
    #0 0x563866c586a1 in __sanitizer_print_stack_trace /rustc/llvm/src/llvm-project/compiler-rt/lib/asan/asan_stack.cpp:87:3
    #1 0x563866f85cb0 in fuzzer::PrintStackTrace() (/home/kali/Desktop/tinybmp/fuzz/target/x86_64-unknown-linux-gnu/release/fuzz_target_1+0x58acb0) (BuildId: ed06bfca53ef4f0e44feb1de3c722506b167b8f5)

> hexdump -C /Users/symeonp/Desktop/crash-multiply-overflow.bmp
00000000  42 4d 42 ff ff ff ff 2d  39 ff ff ff 00 a1 28 00  |BMB����-9���.�(.|
00000010  00 00 ff ff ff ff ff ff  ff 71 fe 32 20 00 00 00  |..�������q�2 ...|
00000020  00 00 00 00 fe fe fe 0d  0d 0d 0d 0d 03 00 00 00  |....���.........|
00000030  00 00 80 00 00 00                                 |......|
00000036

crash-multiply-overflow.bmp.zip

rfuest commented

Thanks. This crash should be fixed by #32, but the crash with minimized-crash-overflow.bmp still exist.

rfuest commented

The minimized-crash-overflow.bmp should now also be fixed.