| Message ID | 20240704102617.1132337-12-hadess@hadess.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix a number of static analysis issues #5 | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | fail | WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search 4: B1 Line exceeds max length (118>80): "bluez-5.76/tools/mesh/mesh-db.c:551:3: cast_overflow: Truncation due to cast operation on "ele_cnt" from 32 to 8 bits." 5: B1 Line exceeds max length (175>80): "bluez-5.76/tools/mesh/mesh-db.c:551:3: overflow_sink: "ele_cnt", which might have overflowed, is passed to "remote_add_node((uint8_t const *)uuid, unicast, ele_cnt, key_idx)"." 6: B3 Line contains hard tab characters (\t): "549| continue;" 8: B3 Line contains hard tab characters (\t): "551|-> remote_add_node((const uint8_t *)uuid, unicast, ele_cnt," 9: B3 Line contains hard tab characters (\t): "552| key_idx);" 10: B3 Line contains hard tab characters (\t): "553| for (j = 1; j < key_cnt; j++) {" |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
> On Jul 4, 2024, at 3:27 AM, Bastien Nocera <hadess@hadess.net> wrote: > > Error: INTEGER_OVERFLOW (CWE-190): [#def29] [important] > bluez-5.76/tools/mesh/mesh-db.c:551:3: cast_overflow: Truncation due to cast operation on "ele_cnt" from 32 to 8 bits. > bluez-5.76/tools/mesh/mesh-db.c:551:3: overflow_sink: "ele_cnt", which might have overflowed, is passed to "remote_add_node((uint8_t const *)uuid, unicast, ele_cnt, key_idx)". > 549| continue; > 550| > 551|-> remote_add_node((const uint8_t *)uuid, unicast, ele_cnt, > 552| key_idx); > 553| for (j = 1; j < key_cnt; j++) { > --- > tools/mesh/mesh-db.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/tools/mesh/mesh-db.c b/tools/mesh/mesh-db.c > index 1d047691d240..abcc09d523a5 100644 > --- a/tools/mesh/mesh-db.c > +++ b/tools/mesh/mesh-db.c > @@ -503,7 +503,8 @@ static void load_remotes(json_object *jcfg) > uint8_t uuid[16]; > uint16_t unicast, key_idx; > const char *str; > - int ele_cnt, key_cnt; > + uint8_t ele_cnt; > + int key_cnt; > int j; > > jnode = json_object_array_get_idx(jnodes, i); > @@ -533,9 +534,6 @@ static void load_remotes(json_object *jcfg) > > ele_cnt = json_object_array_length(jarray); > > - if (ele_cnt > MAX_ELE_COUNT) > - continue; > - What happens if the json file is corrupted and there are more than 255 elements in the array? > json_object_object_get_ex(jnode, "netKeys", &jarray); > if (!jarray || json_object_get_type(jarray) != json_type_array) > continue; > -- > 2.45.2 > >
On Thu, 2024-07-04 at 11:45 -0700, Brian Gix wrote: > > > On Jul 4, 2024, at 3:27 AM, Bastien Nocera <hadess@hadess.net> > > wrote: > > > > Error: INTEGER_OVERFLOW (CWE-190): [#def29] [important] > > bluez-5.76/tools/mesh/mesh-db.c:551:3: cast_overflow: Truncation > > due to cast operation on "ele_cnt" from 32 to 8 bits. > > bluez-5.76/tools/mesh/mesh-db.c:551:3: overflow_sink: "ele_cnt", > > which might have overflowed, is passed to "remote_add_node((uint8_t > > const *)uuid, unicast, ele_cnt, key_idx)". > > 549| continue; > > 550| > > 551|-> remote_add_node((const uint8_t *)uuid, unicast, > > ele_cnt, > > 552| key_idx); > > 553| for (j = 1; j < key_cnt; j++) { > > --- > > tools/mesh/mesh-db.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/tools/mesh/mesh-db.c b/tools/mesh/mesh-db.c > > index 1d047691d240..abcc09d523a5 100644 > > --- a/tools/mesh/mesh-db.c > > +++ b/tools/mesh/mesh-db.c > > @@ -503,7 +503,8 @@ static void load_remotes(json_object *jcfg) > > uint8_t uuid[16]; > > uint16_t unicast, key_idx; > > const char *str; > > - int ele_cnt, key_cnt; > > + uint8_t ele_cnt; > > + int key_cnt; > > int j; > > > > jnode = json_object_array_get_idx(jnodes, i); > > @@ -533,9 +534,6 @@ static void load_remotes(json_object *jcfg) > > > > ele_cnt = json_object_array_length(jarray); > > > > - if (ele_cnt > MAX_ELE_COUNT) > > - continue; > > - > > What happens if the json file is corrupted and there are more than > 255 elements in the array? ele_cnt is a uint8_t, so it will wrap around. We could add that if you preferred (I checked, and the array length is cached): diff --git a/tools/mesh/mesh-db.c b/tools/mesh/mesh-db.c index abcc09d523a5..4c74e874986c 100644 --- a/tools/mesh/mesh-db.c +++ b/tools/mesh/mesh-db.c @@ -529,7 +529,8 @@ static void load_remotes(json_object *jcfg) continue; json_object_object_get_ex(jnode, "elements", &jarray); - if (!jarray || json_object_get_type(jarray) != json_type_array) + if (!jarray || json_object_get_type(jarray) != json_type_array || + json_object_array_length(jarray) > MAX_ELE_COUNT) continue; ele_cnt = json_object_array_length(jarray); > > > json_object_object_get_ex(jnode, "netKeys", &jarray); > > if (!jarray || json_object_get_type(jarray) != > > json_type_array) > > continue; > > -- > > 2.45.2 > > > >
Sent from my iPhone > On Jul 4, 2024, at 12:51 PM, Bastien Nocera <hadess@hadess.net> wrote: > > On Thu, 2024-07-04 at 11:45 -0700, Brian Gix wrote: >> >>> On Jul 4, 2024, at 3:27 AM, Bastien Nocera <hadess@hadess.net> >>> wrote: >>> >>> Error: INTEGER_OVERFLOW (CWE-190): [#def29] [important] >>> bluez-5.76/tools/mesh/mesh-db.c:551:3: cast_overflow: Truncation >>> due to cast operation on "ele_cnt" from 32 to 8 bits. >>> bluez-5.76/tools/mesh/mesh-db.c:551:3: overflow_sink: "ele_cnt", >>> which might have overflowed, is passed to "remote_add_node((uint8_t >>> const *)uuid, unicast, ele_cnt, key_idx)". >>> 549| continue; >>> 550| >>> 551|-> remote_add_node((const uint8_t *)uuid, unicast, >>> ele_cnt, >>> 552| key_idx); >>> 553| for (j = 1; j < key_cnt; j++) { >>> --- >>> tools/mesh/mesh-db.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/tools/mesh/mesh-db.c b/tools/mesh/mesh-db.c >>> index 1d047691d240..abcc09d523a5 100644 >>> --- a/tools/mesh/mesh-db.c >>> +++ b/tools/mesh/mesh-db.c >>> @@ -503,7 +503,8 @@ static void load_remotes(json_object *jcfg) >>> uint8_t uuid[16]; >>> uint16_t unicast, key_idx; >>> const char *str; >>> - int ele_cnt, key_cnt; >>> + uint8_t ele_cnt; >>> + int key_cnt; >>> int j; >>> >>> jnode = json_object_array_get_idx(jnodes, i); >>> @@ -533,9 +534,6 @@ static void load_remotes(json_object *jcfg) >>> >>> ele_cnt = json_object_array_length(jarray); >>> >>> - if (ele_cnt > MAX_ELE_COUNT) >>> - continue; >>> - >> >> What happens if the json file is corrupted and there are more than >> 255 elements in the array? > > ele_cnt is a uint8_t, so it will wrap around. > > We could add that if you preferred (I checked, and the array length is > cached): > > diff --git a/tools/mesh/mesh-db.c b/tools/mesh/mesh-db.c > index abcc09d523a5..4c74e874986c 100644 > --- a/tools/mesh/mesh-db.c > +++ b/tools/mesh/mesh-db.c > @@ -529,7 +529,8 @@ static void load_remotes(json_object *jcfg) > continue; > > json_object_object_get_ex(jnode, "elements", &jarray); > - if (!jarray || json_object_get_type(jarray) != json_type_array) > + if (!jarray || json_object_get_type(jarray) != json_type_array || > + json_object_array_length(jarray) > MAX_ELE_COUNT) > continue; I’m ok with this addition. There is a hard limit on the number of elements in a node, and this edit appears to restore that check. > > ele_cnt = json_object_array_length(jarray); > > > >> >>> json_object_object_get_ex(jnode, "netKeys", &jarray); >>> if (!jarray || json_object_get_type(jarray) != >>> json_type_array) >>> continue; >>> -- >>> 2.45.2 >>> >>> >
diff --git a/tools/mesh/mesh-db.c b/tools/mesh/mesh-db.c index 1d047691d240..abcc09d523a5 100644 --- a/tools/mesh/mesh-db.c +++ b/tools/mesh/mesh-db.c @@ -503,7 +503,8 @@ static void load_remotes(json_object *jcfg) uint8_t uuid[16]; uint16_t unicast, key_idx; const char *str; - int ele_cnt, key_cnt; + uint8_t ele_cnt; + int key_cnt; int j; jnode = json_object_array_get_idx(jnodes, i); @@ -533,9 +534,6 @@ static void load_remotes(json_object *jcfg) ele_cnt = json_object_array_length(jarray); - if (ele_cnt > MAX_ELE_COUNT) - continue; - json_object_object_get_ex(jnode, "netKeys", &jarray); if (!jarray || json_object_get_type(jarray) != json_type_array) continue;
Error: INTEGER_OVERFLOW (CWE-190): [#def29] [important] bluez-5.76/tools/mesh/mesh-db.c:551:3: cast_overflow: Truncation due to cast operation on "ele_cnt" from 32 to 8 bits. bluez-5.76/tools/mesh/mesh-db.c:551:3: overflow_sink: "ele_cnt", which might have overflowed, is passed to "remote_add_node((uint8_t const *)uuid, unicast, ele_cnt, key_idx)". 549| continue; 550| 551|-> remote_add_node((const uint8_t *)uuid, unicast, ele_cnt, 552| key_idx); 553| for (j = 1; j < key_cnt; j++) { --- tools/mesh/mesh-db.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)