| Message ID | 20210413042535.892882-1-dominique.martinet@atmark-techno.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | parse_json: fix read overflow in json parser | expand |
On 13.04.21 06:25, Dominique Martinet wrote: > json_tokener_parse calls strlen on the buffer we pass it, so we need to > nul-terminate whatever we read from sw-description. > > While we are here read return value was never checked so add basic > checks > > Reported-by: ASAN > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > > For reference here is the address sanitizer output: > ================================================================= > ==887523==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62100002a518 at pc 0x7f9de8979b11 bp 0x7f9de16ea570 sp 0x7f9de16e9d20 > READ of size 4121 at 0x62100002a518 thread T2 > #0 0x7f9de8979b10 in __interceptor_strlen ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:370 > #1 0x7f9de876639a in json_tokener_parse_ex (/lib/x86_64-linux-gnu/libjson-c.so.5+0x839a) > #2 0x7f9de8768b16 in json_tokener_parse_verbose (/lib/x86_64-linux-gnu/libjson-c.so.5+0xab16) > #3 0x7f9de8768b7d in json_tokener_parse (/lib/x86_64-linux-gnu/libjson-c.so.5+0xab7d) > #4 0x5580a89b7592 in parse_json parser/parser.c:1029 > > 0x62100002a518 is located 0 bytes to the right of 4120-byte region [0x621000029500,0x62100002a518) > allocated by thread T2 here: > #0 0x7f9de89e6e8f in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145 > #1 0x5580a89b7547 in parse_json parser/parser.c:1016 > #2 0x5580a8a00a0a (/path/to/swupdate/swupdate_unstripped+0xfca0a) > > Thread T2 created by T0 here: > #0 0x7f9de89922a2 in __interceptor_pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:214 > #1 0x5580a8953b84 in start_thread core/pctl.c:84 > > SUMMARY: AddressSanitizer: heap-buffer-overflow ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:370 in __interceptor_strlen > Shadow bytes around the buggy address: > 0x0c427fffd450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c427fffd460: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c427fffd470: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c427fffd480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x0c427fffd490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > =>0x0c427fffd4a0: 00 00 00[fa]fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c427fffd4b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c427fffd4c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c427fffd4d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c427fffd4e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x0c427fffd4f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > Shadow byte legend (one shadow byte represents 8 application bytes): > Addressable: 00 > Partially addressable: 01 02 03 04 05 06 07 > Heap left redzone: fa > Freed heap region: fd > Stack left redzone: f1 > Stack mid redzone: f2 > Stack right redzone: f3 > Stack after return: f5 > Stack use after scope: f8 > Global redzone: f9 > Global init order: f6 > Poisoned by user: f7 > Container overflow: fc > Array cookie: ac > Intra object redzone: bb > ASan internal: fe > Left alloca redzone: ca > Right alloca redzone: cb > Shadow gap: cc > ==887523==ABORTING > > > I don't use the json parser and just stumbled on that as my lua was > invalid (ugh), so will defer to your opinion on what to do on partial > reads etc. > > > This also made me think of EINTR errors, there are current only a > handful of places that actually check for it-- perhaps we should have > some wrapper around read to handle that for all users? (e.g. gnutar has > read_safe and read_full iirc, one just loops over EINTR, the second also > loops over short reads until an error happens or 0 byte is read) > > Anyway, that's not an obvious fix so that's also on hold until current > in flight patches are merged, it's not a real problem for now. > > > parser/parser.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/parser/parser.c b/parser/parser.c > index feb310084032..85241f44413a 100644 > --- a/parser/parser.c > +++ b/parser/parser.c > @@ -1013,7 +1013,7 @@ int parse_json(struct swupdate_cfg *swcfg, const char *filename) > return -EBADF; > > size = stbuf.st_size; > - string = (char *)malloc(size); > + string = (char *)malloc(size+1); > if (!string) > return -ENOMEM; > > @@ -1025,6 +1025,15 @@ int parse_json(struct swupdate_cfg *swcfg, const char *filename) > > ret = read(fd, string, size); > close(fd); > + if (ret < 0) { > + ret = -errno; > + free(string); > + return ret; > + } > + if (ret != size) { > + ERROR("partial read of %s, proceeding anyway", filename); > + } > + string[ret] = '\0'; > > cfg = json_tokener_parse(string); > if (!cfg) { > Applied to -master, thanks ! Best regards, Stefano Babic
diff --git a/parser/parser.c b/parser/parser.c index feb310084032..85241f44413a 100644 --- a/parser/parser.c +++ b/parser/parser.c @@ -1013,7 +1013,7 @@ int parse_json(struct swupdate_cfg *swcfg, const char *filename) return -EBADF; size = stbuf.st_size; - string = (char *)malloc(size); + string = (char *)malloc(size+1); if (!string) return -ENOMEM; @@ -1025,6 +1025,15 @@ int parse_json(struct swupdate_cfg *swcfg, const char *filename) ret = read(fd, string, size); close(fd); + if (ret < 0) { + ret = -errno; + free(string); + return ret; + } + if (ret != size) { + ERROR("partial read of %s, proceeding anyway", filename); + } + string[ret] = '\0'; cfg = json_tokener_parse(string); if (!cfg) {
json_tokener_parse calls strlen on the buffer we pass it, so we need to nul-terminate whatever we read from sw-description. While we are here read return value was never checked so add basic checks Reported-by: ASAN Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> --- For reference here is the address sanitizer output: ================================================================= ==887523==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62100002a518 at pc 0x7f9de8979b11 bp 0x7f9de16ea570 sp 0x7f9de16e9d20 READ of size 4121 at 0x62100002a518 thread T2 #0 0x7f9de8979b10 in __interceptor_strlen ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:370 #1 0x7f9de876639a in json_tokener_parse_ex (/lib/x86_64-linux-gnu/libjson-c.so.5+0x839a) #2 0x7f9de8768b16 in json_tokener_parse_verbose (/lib/x86_64-linux-gnu/libjson-c.so.5+0xab16) #3 0x7f9de8768b7d in json_tokener_parse (/lib/x86_64-linux-gnu/libjson-c.so.5+0xab7d) #4 0x5580a89b7592 in parse_json parser/parser.c:1029 0x62100002a518 is located 0 bytes to the right of 4120-byte region [0x621000029500,0x62100002a518) allocated by thread T2 here: #0 0x7f9de89e6e8f in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145 #1 0x5580a89b7547 in parse_json parser/parser.c:1016 #2 0x5580a8a00a0a (/path/to/swupdate/swupdate_unstripped+0xfca0a) Thread T2 created by T0 here: #0 0x7f9de89922a2 in __interceptor_pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:214 #1 0x5580a8953b84 in start_thread core/pctl.c:84 SUMMARY: AddressSanitizer: heap-buffer-overflow ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:370 in __interceptor_strlen Shadow bytes around the buggy address: 0x0c427fffd450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c427fffd460: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c427fffd470: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c427fffd480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c427fffd490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x0c427fffd4a0: 00 00 00[fa]fa fa fa fa fa fa fa fa fa fa fa fa 0x0c427fffd4b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c427fffd4c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c427fffd4d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c427fffd4e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c427fffd4f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==887523==ABORTING I don't use the json parser and just stumbled on that as my lua was invalid (ugh), so will defer to your opinion on what to do on partial reads etc. This also made me think of EINTR errors, there are current only a handful of places that actually check for it-- perhaps we should have some wrapper around read to handle that for all users? (e.g. gnutar has read_safe and read_full iirc, one just loops over EINTR, the second also loops over short reads until an error happens or 0 byte is read) Anyway, that's not an obvious fix so that's also on hold until current in flight patches are merged, it's not a real problem for now. parser/parser.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)