| Message ID | 20200112112618.2951-1-juraj.vijtiuk@sartura.hr |
|---|---|
| State | Superseded |
| Delegated to: | Petr Štetiar |
| Headers | show |
| Series | [OpenWrt-Devel,libubox] blobmsg: blobmsg_parse and blobmsg_parse_array oob read fixes | expand |
juraj.vijtiuk@sartura.hr <juraj.vijtiuk@sartura.hr> [2020-01-12 12:26:18]: Hi, thanks for the fix. > Fix out of bounds read in blobmsg_parse and blobmsg_check_name. The > out of bounds read happens because blob_attr and blobmsg_hdr have > flexible array members, whose size is 0 in the corresponding sizeofs. > For example the __blob_for_each_attr macro checks whether rem >= > sizeof(struct blob_attr). However, what LibFuzzer discovered was, > if the input data was only 4 bytes, the data would be casted to blob_attr, > and later on blob_data(attr) would be called even though attr->data was empty. > The same issue could appear with data larger than 4 bytes, where data > wasn't empty, but contained only the start of the blobmsg_hdr struct, > and blobmsg_hdr name was empty. The bugs were discovered by fuzzing > blobmsg_parse and blobmsg_array_parse with LibFuzzer. I don't know if you're aware, but there is already some LibFuzzer based fuzzing in tests/fuzz/test-fuzz.c and the corpus is in tests/fuzz/corpus. Those checks are run now automatically by CI after each Git push. It would be nice, if you could share the fuzz input (and mods to test-fuzz.c if any) leading to this OOB reads, so we can guard against future regression. BTW this is not mandatory, it's optional, but I'm going to do this anyway as I'm wondering why current fuzzing didn't catched this issue, so you're going to save me some time :-) Thanks! -- ynezz
juraj.vijtiuk@sartura.hr <juraj.vijtiuk@sartura.hr> [2020-01-12 12:26:18]: Hi, > @@ -35,10 +35,16 @@ static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool na > char *limit = (char *) attr + len; > const struct blobmsg_hdr *hdr; > > + if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr)) > + return false; > + why is this change needed? If I'm reading it correctly, then blobmsg_check_name is only called from blobmsg_check_attr_len and there is already much stricter guard: bool blobmsg_check_attr_len(const struct blob_attr *attr, bool name, size_t len) { if (len < sizeof(struct blob_attr)) return false; if (!blobmsg_check_name(attr, len, name)) return false; > hdr = blob_data(attr); > if (name && !hdr->namelen) > return false; > > + if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr) + blobmsg_namelen(hdr) + 1) > + return false; Didn't checked it in detail yet, but isn't it almost the same as the check few lines bellow, just written differently? > if ((char *) hdr->name + blobmsg_namelen(hdr) > limit) > return false; ... > @@ -191,7 +197,11 @@ int blobmsg_parse(const struct blobmsg_policy *policy, int policy_len, > } > > __blob_for_each_attr(attr, data, len) { > + if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr)) > + return -1; If there is such problem, then this should be probably fixed directly in __blob_for_each_attr so we possibly protect other __blob_for_each_attr users[1]. > hdr = blob_data(attr); > + if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr) + blobmsg_namelen(hdr) + 1) > + return -1; It would be really nice to have blobmsg which could prove, that this check is needed. 1. https://lxr.openwrt.org/ident?i=__blob_for_each_attr -- ynezz
Petr Štetiar <ynezz@true.cz> [2020-01-12 12:43:07]:
> Those checks are run now automatically by CI after each Git push.
I don't know if it's obvious how to use the tests/fuzzing inside libubox (and
other C projects), so I've written something small[1] about the CI checks and
used libubox as example.
1. http://lists.infradead.org/pipermail/openwrt-devel/2019-December/020831.html
-- ynezz
Hello, On Sun, Jan 12, 2020 at 01:09:57PM +0100, Petr Štetiar wrote: > > @@ -35,10 +35,16 @@ static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool na > > char *limit = (char *) attr + len; > > const struct blobmsg_hdr *hdr; > > > > + if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr)) > > + return false; > > + > > why is this change needed? If I'm reading it correctly, then blobmsg_check_name > is only called from blobmsg_check_attr_len and there is already much stricter guard: > bool blobmsg_check_attr_len(const struct blob_attr *attr, bool name, size_t len) > { > if (len < sizeof(struct blob_attr)) > return false; > > if (!blobmsg_check_name(attr, len, name)) > return false; > I wasn't sure what the best placement for the check would be, so I put it into the blobmsg_check_name function. My rationale however was that the existing check in blobmsg_check_attr_len checks whether we have enough data to call blobmsg_check_name at all, as it uses attr as a blob_attr struct. The check was added because sizeof(struct blob_attr) is only 4, as it contains a uint32_t and a flexible array member. The size of the struct containing the flexible array member is as if it was omitted when doing a sizeof of the struct, as specified by the C standards from C99 upwards if I recall correctly. Therefore, another len check in blobmsg_check_name is added to check if there is enough data in attr->data so that it makes sense to call blob_data(attr), as otherwise, if the struct has only the first 4 bytes filled in, the flexible array attr->data is empty, so hdr->namelen will access unused memory which causes the oob read. Here is the xxd output for the crash file that causes the invalid read when passed as data to blobmsg_parse_array with a len of 4: xxd crash-a3585b70f1c7ffbdec10f6dadc964336118485c4 00000000: 0300 0004 .... Here is the relevant valgrind output, the main function here simply reads the input and passes it to blobmsg_parse_array: ==10829== Invalid read of size 2 ==10829== at 0x109DFC: blobmsg_namelen (blobmsg.h:74) ==10829== by 0x109446: blobmsg_check_name (blobmsg.c:42) ==10829== by 0x1092DD: blobmsg_check_attr_len (blobmsg.c:79) ==10829== by 0x109A63: blobmsg_parse_array (blobmsg.c:159) ==10829== by 0x10A7BA: main (blobmsg.c:412) ==10829== Address 0x4a2e2b4 is 0 bytes after a block of size 4 alloc'd ==10829== at 0x483877F: malloc (vg_replace_malloc.c:309) ==10829== by 0x10A773: main (blobmsg.c:408) The same issue appears with the same input file for blobmsg_parse. > > hdr = blob_data(attr); > > if (name && !hdr->namelen) > > return false; > > > > + if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr) + blobmsg_namelen(hdr) + 1) > > + return false; > > Didn't checked it in detail yet, but isn't it almost the same as the check few > lines bellow, just written differently? > > if ((char *) hdr->name + blobmsg_namelen(hdr) > limit) > > return false; > > ... > You are correct, it is almost the same, however it prevents another invalid read below, in blobmsg.c:48: if (hdr->name[blobmsg_namelen(hdr)] != 0) return false; I assume that this checks whether hdr->name is a null terminated string. However, namelen seems to store the result returned as if it was returned by strlen, and the limit check doesn't seem to include the terminating null byte, although I suppose modifying that check would definitely be better than having two checks. The check would then look like this: if ((char *) hdr->name + blobmsg_namelen(hdr) + 1 > limit) return false; Here is the xxd of the file that caused the issue to appear: 00000000: 0300 0003 0000 ...... Here is the relevant valgrind output: ==6076== Invalid read of size 1 ==6076== at 0x1094B8: blobmsg_check_name (blobmsg.c:48) ==6076== by 0x1092DD: blobmsg_check_attr_len (blobmsg.c:79) ==6076== by 0x109A63: blobmsg_parse_array (blobmsg.c:159) ==6076== by 0x10A7BA: main (blobmsg.c:412) ==6076== Address 0x4a2e2b6 is 0 bytes after a block of size 6 alloc'd ==6076== at 0x483877F: malloc (vg_replace_malloc.c:309) ==6076== by 0x10A773: main (blobmsg.c:408) > > @@ -191,7 +197,11 @@ int blobmsg_parse(const struct blobmsg_policy *policy, int policy_len, > > } > > > > __blob_for_each_attr(attr, data, len) { > > + if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr)) > > + return -1; > > If there is such problem, then this should be probably fixed directly in > __blob_for_each_attr so we possibly protect other __blob_for_each_attr > users[1]. Can you maybe provide a patch? I'd be happy to test it and let you know what the results are. Regards, Juraj
Juraj Vijtiuk <juraj.vijtiuk@sartura.hr> [2020-01-14 22:11:18]: > Here is the xxd output for the crash file that causes the invalid read when > passed as data to blobmsg_parse_array with a len of 4: > > xxd crash-a3585b70f1c7ffbdec10f6dadc964336118485c4 > 00000000: 0300 0004 .... Thanks for sharing. > Here is the relevant valgrind output, the main function here simply reads > the input and passes it to blobmsg_parse_array: > > ==10829== Invalid read of size 2 > ==10829== at 0x109DFC: blobmsg_namelen (blobmsg.h:74) > ==10829== by 0x109446: blobmsg_check_name (blobmsg.c:42) > ==10829== by 0x1092DD: blobmsg_check_attr_len (blobmsg.c:79) > ==10829== by 0x109A63: blobmsg_parse_array (blobmsg.c:159) > ==10829== by 0x10A7BA: main (blobmsg.c:412) > ==10829== Address 0x4a2e2b4 is 0 bytes after a block of size 4 alloc'd > ==10829== at 0x483877F: malloc (vg_replace_malloc.c:309) > ==10829== by 0x10A773: main (blobmsg.c:408) That malloc is important here, one has to use dynamically allocated buffer and Valgrind in order to see this OOB read access. There's already similar crash from the previous runs, but without malloc'ed buffer Valgrind doesn't spot that, lesson learned. xxd tests/fuzz/corpus/crash-75b146c4e6fac64d3e62236b27c64b50657bab2a 00000000: 0300 0002 .... > I assume that this checks whether hdr->name is a null terminated > string. However, namelen seems to store the result returned as if it was returned by strlen, > and the limit check doesn't seem to include the terminating null byte, > although I suppose modifying that check would definitely be better than > having two checks. The check would then look like this: > > if ((char *) hdr->name + blobmsg_namelen(hdr) + 1 > limit) > return false; Ok, makes sense, I'll check that. > > > __blob_for_each_attr(attr, data, len) { > > > + if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr)) > > > + return -1; > > > > If there is such problem, then this should be probably fixed directly in > > __blob_for_each_attr so we possibly protect other __blob_for_each_attr > > users[1]. > > Can you maybe provide a patch? I'd be happy to test it and let you > know what the results are. Ok. -- ynezz
Juraj Vijtiuk <juraj.vijtiuk@sartura.hr> [2020-01-14 22:11:18]: Hi, I just sent v2 for review[2], can you check it please? > > > @@ -191,7 +197,11 @@ int blobmsg_parse(const struct blobmsg_policy *policy, int policy_len, > > > } > > > > > > __blob_for_each_attr(attr, data, len) { > > > + if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr)) > > > + return -1; > > > > If there is such problem, then this should be probably fixed directly in > > __blob_for_each_attr so we possibly protect other __blob_for_each_attr > > users[1]. > > Can you maybe provide a patch? I'd be happy to test it and let you > know what the results are. Seems like I need more time on this, to come up with some meaningful solution (it's blob related function, but it would need to check blobmsg etc.), I've just prepared some common helper functions which should help. I don't want to block this changes just because of that. I'll try to add you to the Cc: in that follow up patch. 1. https://patchwork.ozlabs.org/patch/1225878/ -- ynezz
Hello, I have checked v2 of the patch, and can confirm that it fixes the out of bounds reads. I have also fuzzed blobmsg_parse and blobmsg_parse_array for an hour with LibFuzzer, to check that there are no more similar shallow issues and no new crashes were found. Thank you for the help! Regards, Juraj On Mon, Jan 20, 2020 at 12:09 PM Petr Štetiar <ynezz@true.cz> wrote: > > Juraj Vijtiuk <juraj.vijtiuk@sartura.hr> [2020-01-14 22:11:18]: > > Hi, > > I just sent v2 for review[2], can you check it please? > > > > > @@ -191,7 +197,11 @@ int blobmsg_parse(const struct blobmsg_policy *policy, int policy_len, > > > > } > > > > > > > > __blob_for_each_attr(attr, data, len) { > > > > + if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr)) > > > > + return -1; > > > > > > If there is such problem, then this should be probably fixed directly in > > > __blob_for_each_attr so we possibly protect other __blob_for_each_attr > > > users[1]. > > > > Can you maybe provide a patch? I'd be happy to test it and let you > > know what the results are. > > Seems like I need more time on this, to come up with some meaningful solution > (it's blob related function, but it would need to check blobmsg etc.), I've > just prepared some common helper functions which should help. I don't want to > block this changes just because of that. I'll try to add you to the Cc: in > that follow up patch. > > 1. https://patchwork.ozlabs.org/patch/1225878/ > > -- ynezz
diff --git a/blobmsg.c b/blobmsg.c index 1dd57e1..0988f60 100644 --- a/blobmsg.c +++ b/blobmsg.c @@ -35,10 +35,16 @@ static bool blobmsg_check_name(const struct blob_attr *attr, size_t len, bool na char *limit = (char *) attr + len; const struct blobmsg_hdr *hdr; + if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr)) + return false; + hdr = blob_data(attr); if (name && !hdr->namelen) return false; + if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr) + blobmsg_namelen(hdr) + 1) + return false; + if ((char *) hdr->name + blobmsg_namelen(hdr) > limit) return false; @@ -191,7 +197,11 @@ int blobmsg_parse(const struct blobmsg_policy *policy, int policy_len, } __blob_for_each_attr(attr, data, len) { + if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr)) + return -1; hdr = blob_data(attr); + if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr) + blobmsg_namelen(hdr) + 1) + return -1; for (i = 0; i < policy_len; i++) { if (!policy[i].name) continue;
Fix out of bounds read in blobmsg_parse and blobmsg_check_name. The out of bounds read happens because blob_attr and blobmsg_hdr have flexible array members, whose size is 0 in the corresponding sizeofs. For example the __blob_for_each_attr macro checks whether rem >= sizeof(struct blob_attr). However, what LibFuzzer discovered was, if the input data was only 4 bytes, the data would be casted to blob_attr, and later on blob_data(attr) would be called even though attr->data was empty. The same issue could appear with data larger than 4 bytes, where data wasn't empty, but contained only the start of the blobmsg_hdr struct, and blobmsg_hdr name was empty. The bugs were discovered by fuzzing blobmsg_parse and blobmsg_array_parse with LibFuzzer. Signed-off-by: Juraj Vijtiuk <juraj.vijtiuk@sartura.hr> CC: Luka Perkov <luka.perkov@sartura.hr>