[PATCH mtd-utils] lib: Fix integer overflow in libiniparser.c

[PATCH mtd-utils] lib: Fix integer overflow in libiniparser.c

[Anton Moryakov]

Report of the static analyzer:
Possible integer underflow: left operand is tainted. 
An integer underflow may occur due to arithmetic operation (subtraction) between values
{ [-2147483648, 2147483647] } and '1', where the first value comes from the expression '(int)strlen(line)'

Corrections explained:
1. Added a check to handle empty strings before decrementing the length of the line (`len = strlen(line) - 1`) to prevent integer underflow.
2. Modified logic toskip empty lines immediately, reducing unnecessary processing.- Ensured safety by validating len >= 0 before accessing line[len] to avoid out-of-bounds access.
3. Improved robustness when handling multiline values by ensuring valid indices for len.- Added relevant comments to clarify changes and their purpose.

Triggers found by static analyzer Svace.

Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>

---
 lib/libiniparser.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/lib/libiniparser.c b/lib/libiniparser.c
index a6ddcc7..da980c0 100644
--- a/lib/libiniparser.c
+++ b/lib/libiniparser.c
@@ -522,9 +522,15 @@ dictionary * iniparser_load(const char * ininame)
 
     while (fgets(line+last, ASCIILINESZ-last, in)!=NULL) {
         lineno++ ;
-        len = (int)strlen(line)-1;
+        len = (int)strlen(line);
+        if(len == 0){
+            memset(line, 0, ASCIILINESZ);
+            last = 0;
+            continue;
+        }
+        len--;
         /* Safety check against buffer overflows */
-        if (line[len]!='\n') {
+        if (len < 0 || line[len]!='\n') {
             fprintf(stderr,
                     "iniparser: input line too long in %s (%d)\n",
                     ininame,
@@ -540,7 +546,7 @@ dictionary * iniparser_load(const char * ininame)
             len-- ;
         }
         /* Detect multi-line */
-        if (line[len]=='\\') {
+        if (len >= 0 && line[len]=='\\') {
             /* Multi-line value */
             last=len ;
             continue ;
-- 
2.30.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH mtd-utils] lib: Fix integer overflow in libiniparser.c

[Zhihao Cheng]

在 2024/12/13 3:12, Anton Moryakov 写道:
> Report of the static analyzer:
> Possible integer underflow: left operand is tainted.
> An integer underflow may occur due to arithmetic operation (subtraction) between values
> { [-2147483648, 2147483647] } and '1', where the first value comes from the expression '(int)strlen(line)'
> 
> Corrections explained:
> 1. Added a check to handle empty strings before decrementing the length of the line (`len = strlen(line) - 1`) to prevent integer underflow.
> 2. Modified logic toskip empty lines immediately, reducing unnecessary processing.- Ensured safety by validating len >= 0 before accessing line[len] to avoid out-of-bounds access.
> 3. Improved robustness when handling multiline values by ensuring valid indices for len.- Added relevant comments to clarify changes and their purpose.
> 
> Triggers found by static analyzer Svace.
> 
> Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
> 
> ---
>   lib/libiniparser.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/libiniparser.c b/lib/libiniparser.c
> index a6ddcc7..da980c0 100644
> --- a/lib/libiniparser.c
> +++ b/lib/libiniparser.c
> @@ -522,9 +522,15 @@ dictionary * iniparser_load(const char * ininame)
>   
>       while (fgets(line+last, ASCIILINESZ-last, in)!=NULL) {
>           lineno++ ;
> -        len = (int)strlen(line)-1;
> +        len = (int)strlen(line);

Hi, Anton.
The 'len' must be greater than 0 if fgets() returns not NULL, the 'line' 
contains at least one letter '\n'.
> +        if(len == 0){
> +            memset(line, 0, ASCIILINESZ);
> +            last = 0;
> +            continue;
> +        }
> +        len--;
>           /* Safety check against buffer overflows */
> -        if (line[len]!='\n') {
> +        if (len < 0 || line[len]!='\n') {
>               fprintf(stderr,
>                       "iniparser: input line too long in %s (%d)\n",
>                       ininame,
> @@ -540,7 +546,7 @@ dictionary * iniparser_load(const char * ininame)
>               len-- ;
>           }

Here, 'len' could be less than zero. I'm fine with the modification here.
>           /* Detect multi-line */
> -        if (line[len]=='\\') {
> +        if (len >= 0 && line[len]=='\\') {
>               /* Multi-line value */
>               last=len ;
>               continue ;
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/