| Message ID | 20210323151505.10045-1-e.kapllaj@asem.it |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | grubenv: fix buffer overflow on strtok function | expand |
Hi Elvis, On 23.03.21 16:15, Elvis Kapllaj wrote: > grubenv file is a fixed block of memory (1024) not null-terminated. > Using strtok on this buffer can have unpredicted results based on > the memory content immediately after the buffer. > > During an intensive SWUPDATE test, we noticed some random crashes > with the following error: > > [ERROR] : SWUPDATE failed [0] ERROR bootloader/grub.c : grubenv_write : 174 : Not enough free space in envblk file, 1031 > [ERROR] : SWUPDATE failed [0] ERROR corelib/installer.c : update_bootloader_env : 191 : Bootloader-specific error -1 updating its environment Software updated failed > > It turned out to be related to strtok overrunning the env buffer. > The bottom of the grubenv file is filled with "#" characters, and > the latest cycle on the do_while loop was detecting the "####" string as > the `key` env token (very long string). This file is not terminated with > '\0' so strtok was not stopping until it went beyound the buffer area. > On a lucky run, the memory immediately after the buffer was filled with > 0, so strtok returned a very long string for the `key` string, and a > null for `value`. > > Sometimes the memory after the buffer was filled with some other data, > which made the strtok call go on finding a valid `value` string outside > the buffer, resulting in the swupdate crash. Since strtok is destructive > on the string it uses (replaces the string separator with '\0') this > could also lead to memory corruption. > > With this patch, we allocate an extra byte for the '\0' character on the > buffer, to make sure strtok works as expected. > > Signed-off-by: Elvis Kapllaj <e.kapllaj@asem.it> > --- > bootloader/grub.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/bootloader/grub.c b/bootloader/grub.c > index 45fb102..a75af5b 100644 > --- a/bootloader/grub.c > +++ b/bootloader/grub.c > @@ -44,7 +44,12 @@ static int grubenv_open(struct grubenv_t *grubenv) > goto cleanup; > } > > - buf = malloc(size); Agree, code is wrong, thanks for fixing it. > + /* grubenv is a block of 'size' bytes not null-terminated, and this is a > + * problem when using strtok, which is expecting a null-terminated > + * string. Allocate an extra byte to terminate the buffer or strtok will > + * overrun it. > + * . */ > + buf = malloc(size+1); Then switch to calloc() and the buffer is automatically zeroed. > if (!buf) { > ERROR("Not enough memory for environment"); > fclose(fp); > @@ -57,6 +62,8 @@ static int grubenv_open(struct grubenv_t *grubenv) > ret = 1; > goto cleanup; > } > + /* terminate buf */ > + buf[size]='\0'; > > if (memcmp(buf, GRUBENV_HEADER, strlen(GRUBENV_HEADER) -1)) { > ERROR("Invalid grubenv header"); > Best regards, Stefano Babic
Hi stefano,
Yes we could also use calloc to allocate the memory, but it would be less efficient i think.
What we are doing after allocating that block of memory is:
if (fread(buf, 1, size, fp) != size) {
printf("Failed to read file %s\n", GRUBENV_PATH);
ret = 1;
goto cleanup;
}
+buf[size]='\0';
So we are reading size bytes from the file and writing it into the buffer, so it doesn't matter what the previous content was.
To be on the safe side, and fix this issue, we only need to make sure we have allocated an extra byte at the end, and it contains
'\0', so I think using malloc and terminating the buffer would be more efficient than using calloc() to clear all 1025 bytes, on which
the first 1024 are being written by fread() anyway.
Best regards.
Elvis Kapllaj
[1596469214269]
Elvis Kapllaj
R&D - Software Engineer
ASEM S.r.l.
Via Buia 4, 33011 Artegna (UD) Italia
tel +39 0432 967 423
email: e.kapllaj@asem.it<mailto:l.dandrea@asem.it>
website: www.asemautomation.com<http://www.asemautomation.com/>
diff --git a/bootloader/grub.c b/bootloader/grub.c index 45fb102..a75af5b 100644 --- a/bootloader/grub.c +++ b/bootloader/grub.c @@ -44,7 +44,12 @@ static int grubenv_open(struct grubenv_t *grubenv) goto cleanup; } - buf = malloc(size); + /* grubenv is a block of 'size' bytes not null-terminated, and this is a + * problem when using strtok, which is expecting a null-terminated + * string. Allocate an extra byte to terminate the buffer or strtok will + * overrun it. + * . */ + buf = malloc(size+1); if (!buf) { ERROR("Not enough memory for environment"); fclose(fp); @@ -57,6 +62,8 @@ static int grubenv_open(struct grubenv_t *grubenv) ret = 1; goto cleanup; } + /* terminate buf */ + buf[size]='\0'; if (memcmp(buf, GRUBENV_HEADER, strlen(GRUBENV_HEADER) -1)) { ERROR("Invalid grubenv header");
grubenv file is a fixed block of memory (1024) not null-terminated. Using strtok on this buffer can have unpredicted results based on the memory content immediately after the buffer. During an intensive SWUPDATE test, we noticed some random crashes with the following error: [ERROR] : SWUPDATE failed [0] ERROR bootloader/grub.c : grubenv_write : 174 : Not enough free space in envblk file, 1031 [ERROR] : SWUPDATE failed [0] ERROR corelib/installer.c : update_bootloader_env : 191 : Bootloader-specific error -1 updating its environment Software updated failed It turned out to be related to strtok overrunning the env buffer. The bottom of the grubenv file is filled with "#" characters, and the latest cycle on the do_while loop was detecting the "####" string as the `key` env token (very long string). This file is not terminated with '\0' so strtok was not stopping until it went beyound the buffer area. On a lucky run, the memory immediately after the buffer was filled with 0, so strtok returned a very long string for the `key` string, and a null for `value`. Sometimes the memory after the buffer was filled with some other data, which made the strtok call go on finding a valid `value` string outside the buffer, resulting in the swupdate crash. Since strtok is destructive on the string it uses (replaces the string separator with '\0') this could also lead to memory corruption. With this patch, we allocate an extra byte for the '\0' character on the buffer, to make sure strtok works as expected. Signed-off-by: Elvis Kapllaj <e.kapllaj@asem.it> --- bootloader/grub.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)