sheredom / json.h

🗄️ single header json parser for C and C++
The Unlicense
698 stars 77 forks source link

Buffer overflow in json_skip_whitespace #80

Closed skeeto closed 1 year ago

skeeto commented 1 year ago
#include "json.h"
static char s[2] = "1e";
int main(void) { json_parse(s, 2); }
$ cc -g3 -fsanitize=address crash.c 
$ ./a.out 
ERROR: AddressSanitizer: global-buffer-overflow
READ of size 1 at 0x5561efd8d583 thread T0
    #0 json_skip_whitespace json.h/json.h:505
    #1 json_skip_all_skippables json.h/json.h:637
    #2 json_parse_ex json.h/json.h:2038
    #3 json_parse json.h/json.h:2115
    #4 main json.h/crash.c:3
sheredom commented 1 year ago

Not sure what I've done, but when I test this with the following test:


UTEST(random, whitespace_overrun) {
  const char payload[] = "1e";
  struct json_value_s *const root = json_parse(payload, sizeof(payload));
  ASSERT_FALSE(root);
}

I do not get sanitizer errors 🤔

sheredom commented 1 year ago

Weird - if I run it standalone I see the bug. How odd! I'll fix, just wish I could have a test case.

sheredom commented 1 year ago

Ah! I'm an idiot, I used sizeof(payload) when I should have used 2, got it with a test. Nice find, thanks!

skeeto commented 1 year ago

Thanks for the quick response! The test would be tighter if you didn't include the null terminator, since otherwise ASan will give it slack with reading the zero byte beyond the specified length. So it wasn't so much the sizeof that was wrong but the missing array size.

--- a/test/main.cpp
+++ b/test/main.cpp
@@ -909,4 +909,4 @@ UTEST(random, overflow) {
 UTEST(random, whitespace_overrun) {
-  const char payload[] = "1e";
-  struct json_value_s *const root = json_parse(payload, 2);
+  const char payload[2] = "1e";
+  struct json_value_s *const root = json_parse(payload, sizeof(payload));
   ASSERT_FALSE(root);
sheredom commented 1 year ago

I actually fixed the test case better in #83 after your suggestion!