Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Out of Bounds Write in v1.0.4 #3

Closed
Halcy0nic opened this issue Jul 22, 2022 · 10 comments
Closed

Out of Bounds Write in v1.0.4 #3

Halcy0nic opened this issue Jul 22, 2022 · 10 comments

Comments

@Halcy0nic
Copy link

Hi!

While I was using the tool I had some fuzz tests running in the background and I think there might be an out of bounds write bug in the webp to png converter. I compiled the tool from source using the default instructions/Makefile. I can't exactly figure out from the backtrace where the out of bounds write is happening in png2webp.c, but a rough guess would be somewhere around:

if(reverse)

memcpy(&ext, *argv + len - 4, 4);

memcpy(&extmatch, (char[4]){"webp"}, 4);

I've attached the valgrind and gdb output below with a copy of the file used to trigger the issue:

Screen Shot 2022-07-22 at 10 55 26 AM

Screen Shot 2022-07-22 at 11 36 28 AM

Crash file

This would possibly allow an attacker to overwrite heap memory with attacker provided data.
crash.zip

@landfillbaby
Copy link
Owner

What OS, architecture, and compiler were you testing on?

@landfillbaby
Copy link
Owner

landfillbaby commented Jul 23, 2022

I tested that file on a version I just compiled on Termux on my Pixel 6:

$ png2webp -rv crash1_overflow.webp
Decoding crash1_overflow.webp ...
FORTIFY: read: count 18446744073709549715 > SSIZE_MAX
Aborted

The problem seems to be that, against the C standard, certain platforms use ssize_t for fread's parameters instead of size_t.
Try again using this patch, and when I'm at my PC I'll look into it further.

diff --git a/png2webp.c b/png2webp.c
index 42443f5..30bd4fd 100644
--- a/png2webp.c
+++ b/png2webp.c
@@ -319,6 +319,14 @@ static bool w2p(char *ip, char *op) {
   }
   size_t l = ((uint32_t)(i[4] | (i[5] << 8) | (i[6] << 16) | (i[7] << 24))) + 8;
   // ^ RIFF header size
+  if(l < 12
+#ifdef SSIZE_MAX
+    || l - 12 > SSIZE_MAX
+#endif
+  ) {
+    PF("ERROR reading %s: %s", IP, k[2]);
+    goto w2p_close;
+  }
   x = malloc(l);
   if(!x) {
     PF("ERROR reading %s: %s", IP, *k);

@landfillbaby
Copy link
Owner

I need to check this doesn't happen on platforms that don't define SSIZE_MAX, e.g. Windows.

@landfillbaby
Copy link
Owner

landfillbaby commented Jul 23, 2022

Also what fuzzer are you using? I might use it myself. I hope it's AFL 🐇

@landfillbaby
Copy link
Owner

Fixed in v1.0.5 (I think).
Won't close until you answer my questions though :)
I knew there was something up with my WebP reading code, thanks!

@landfillbaby
Copy link
Owner

landfillbaby commented Jul 23, 2022

@Halcy0nic feel free to add this to your trophy list lol 🥇
BTW the part you guessed is safe, it's just the file extension replacement.

@Halcy0nic
Copy link
Author

Sweet! Just tested it out and it seems fixed. Thanks again!

@Halcy0nic
Copy link
Author

Also you are correct, the fuzzer I was using is AFL++ lol

@Halcy0nic
Copy link
Author

I tested on a few Linux distros (Debian, ubuntu, etc), all 64 bit

@landfillbaby
Copy link
Owner

Ok good thank you :) I'll close this now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants