Re: [PATCH 2/4] fs/iso9660: Prevent read past the end of system use area
> On Dec 15, 2022, at 10:00 AM, Thomas Schmitt wrote:
>
> Hi,
>
> On Wed, 14 Dec 2022 18:55:03 + Lidong Chen wrote:
>> In the code, the for loop advanced the entry pointer to the
>> next entry before checking if the next entry is within the
>> system use area boundary. Another issue in the code was that
>> there is no check for the size of system use area. For a
>> corrupted system, the size of system use area can be less than
>> the size of SUSP entry size (5 bytes).
>
> The minimum size of a SUSP entry is 4, not 5. Examples of 4-byte entries
> are ST in SUSP and RE in RRIP. (Ending before ST would be harmless, because
> ST marks the end of the SUSP entry chain. But RE may appear before the end.)
>
I will fix it.
>
>> These can cause buffer
>> overrun. The fixes added the checks to ensure the read is
>> valid and within the boundary.
>>
>> Signed-off-by: Lidong Chen
>> ---
>> grub-core/fs/iso9660.c | 31 +++
>> 1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
>> index 4f4cd6165..9170fa820 100644
>> --- a/grub-core/fs/iso9660.c
>> +++ b/grub-core/fs/iso9660.c
>> @@ -49,6 +49,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
>> #define GRUB_ISO9660_VOLDESC_PART3
>> #define GRUB_ISO9660_VOLDESC_END 255
>>
>> +#define GRUB_ISO9660_SUSP_HEADER_SZ 5
>
> So i think this should be 4, not 5.
> If we find reason why it should be 5, then the name SUSP_HEADER_SZ is not
> appropriate. The SUSP header size is surely 4.
You are right. SUSP-1.12 stated:
“If the remaining allocated space following the last recorded System Use
Entry in a System Use field or Continuation Area is less than 4 bytes long,
It cannot contain a System Use Entry and should be ignored”
It implied the minimum SUSP Entry size is 4. I will fix it.
>
>
>> +
>> /* The head of a volume descriptor. */
>> struct grub_iso9660_voldesc
>> {
>> @@ -272,6 +274,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
>> grub_off_t off,
>> if (sua_size <= 0)
>> return GRUB_ERR_NONE;
>>
>> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
>> +return grub_error (GRUB_ERR_BAD_FS, "invalid susp entry size");
>> +
>
> Here we need 4.
>
>
>> sua = grub_malloc (sua_size);
>> if (!sua)
>> return grub_errno;
>> @@ -281,10 +286,14 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
>> grub_off_t off,
>> if (err)
>> return err;
>>
>> - for (entry = (struct grub_iso9660_susp_entry *) sua; (char *) entry <
>> (char *) sua + sua_size - 1 && entry->len > 0;
>> - entry = (struct grub_iso9660_susp_entry *)
>> - ((char *) entry + entry->len))
>> + entry = (struct grub_iso9660_susp_entry *) sua;
>> +
>> + while (entry->len > 0)
>> {
>> + /* Ensure the entry is within System Use Area */
>> + if ((char *) entry + entry->len > (sua + sua_size))
>> +break;
>> +
>> /* The last entry. */
>> if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
>> break;
>> @@ -300,6 +309,15 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
>> grub_off_t off,
>>off = grub_le_to_cpu32 (ce->off);
>>ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
>>
>> + if (sua_size <= 0)
>> +break;
>> +
>> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
>
> 4 would be appropriate here.
>
>
>> +{
>> + grub_free (sua);
>> + return grub_error (GRUB_ERR_BAD_FS, "invalid CE entry size");
>> +}
>> +
>>grub_free (sua);
>>sua = grub_malloc (sua_size);
>>if (!sua)
>> @@ -319,6 +337,11 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
>> grub_off_t off,
>>grub_free (sua);
>>return 0;
>> }
>> +
>> + entry = (struct grub_iso9660_susp_entry *) ((char *) entry +
>> entry->len);
>
> This is equivalent to the third statement in the "for" which was
> replaced by a while-loop. So far so good.
>
> But i believe to see an old bug. This advancing of entry breaks the
> chain of CE if the first entry of the Continuation Area is again a CE.
>
> err = grub_disk_read (node->data->disk, ce_block, off,
>sua_size, sua);
> ...
> entry = (struct grub_iso9660_susp_entry *) sua;
>}
>
> if (hook (entry, hook_arg))
>{
> grub_free (sua);
> return 0;
>}
>
> entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
>
> hook() will not interpret CE (and has no means to advance "entry").
> entry = entry + entry->len; will then skip over the entry so that the loop
> will not interpret it either.
> The SUSP area will be perceived as having ended, although more SUSP entries
> are present for the file in question.
>
> I hereby ask for more opinions about this. Maybe i misinterpret the old loop.
>
> Background:
> CE points to the block and offset where the chain of SUSP entries goes on.
>
Re: [PATCH 2/4] fs/iso9660: Prevent read past the end of system use area
Hi, while preparing a proposal how to avoid skipping of CE (and ST) if they are found at the start of a continuation area, i came to a problem of patch [2/4] which i did not see when reviewing it yesterday: > + return grub_error (GRUB_ERR_BAD_FS, "invalid CE entry size"); It is not about the size of the CE entry but about the size of the continuation area which the CE entry announces. So i propose as error message "invalid continuation area size in CE entry" Have a nice day :) Thomas ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/4] fs/iso9660: Prevent read past the end of system use area
Hi,
On Wed, 14 Dec 2022 18:55:03 + Lidong Chen wrote:
> In the code, the for loop advanced the entry pointer to the
> next entry before checking if the next entry is within the
> system use area boundary. Another issue in the code was that
> there is no check for the size of system use area. For a
> corrupted system, the size of system use area can be less than
> the size of SUSP entry size (5 bytes).
The minimum size of a SUSP entry is 4, not 5. Examples of 4-byte entries
are ST in SUSP and RE in RRIP. (Ending before ST would be harmless, because
ST marks the end of the SUSP entry chain. But RE may appear before the end.)
> These can cause buffer
> overrun. The fixes added the checks to ensure the read is
> valid and within the boundary.
>
> Signed-off-by: Lidong Chen
> ---
> grub-core/fs/iso9660.c | 31 +++
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
> index 4f4cd6165..9170fa820 100644
> --- a/grub-core/fs/iso9660.c
> +++ b/grub-core/fs/iso9660.c
> @@ -49,6 +49,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
> #define GRUB_ISO9660_VOLDESC_PART3
> #define GRUB_ISO9660_VOLDESC_END 255
>
> +#define GRUB_ISO9660_SUSP_HEADER_SZ 5
So i think this should be 4, not 5.
If we find reason why it should be 5, then the name SUSP_HEADER_SZ is not
appropriate. The SUSP header size is surely 4.
> +
> /* The head of a volume descriptor. */
> struct grub_iso9660_voldesc
> {
> @@ -272,6 +274,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
>if (sua_size <= 0)
> return GRUB_ERR_NONE;
>
> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
> +return grub_error (GRUB_ERR_BAD_FS, "invalid susp entry size");
> +
Here we need 4.
>sua = grub_malloc (sua_size);
>if (!sua)
> return grub_errno;
> @@ -281,10 +286,14 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
>if (err)
> return err;
>
> - for (entry = (struct grub_iso9660_susp_entry *) sua; (char *) entry <
> (char *) sua + sua_size - 1 && entry->len > 0;
> - entry = (struct grub_iso9660_susp_entry *)
> - ((char *) entry + entry->len))
> + entry = (struct grub_iso9660_susp_entry *) sua;
> +
> + while (entry->len > 0)
> {
> + /* Ensure the entry is within System Use Area */
> + if ((char *) entry + entry->len > (sua + sua_size))
> +break;
> +
>/* The last entry. */
>if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
> break;
> @@ -300,6 +309,15 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
> off = grub_le_to_cpu32 (ce->off);
> ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
>
> + if (sua_size <= 0)
> + break;
> +
> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
4 would be appropriate here.
> + {
> + grub_free (sua);
> + return grub_error (GRUB_ERR_BAD_FS, "invalid CE entry size");
> + }
> +
> grub_free (sua);
> sua = grub_malloc (sua_size);
> if (!sua)
> @@ -319,6 +337,11 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
> grub_off_t off,
> grub_free (sua);
> return 0;
> }
> +
> + entry = (struct grub_iso9660_susp_entry *) ((char *) entry +
> entry->len);
This is equivalent to the third statement in the "for" which was
replaced by a while-loop. So far so good.
But i believe to see an old bug. This advancing of entry breaks the
chain of CE if the first entry of the Continuation Area is again a CE.
err = grub_disk_read (node->data->disk, ce_block, off,
sua_size, sua);
...
entry = (struct grub_iso9660_susp_entry *) sua;
}
if (hook (entry, hook_arg))
{
grub_free (sua);
return 0;
}
entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
hook() will not interpret CE (and has no means to advance "entry").
entry = entry + entry->len; will then skip over the entry so that the loop
will not interpret it either.
The SUSP area will be perceived as having ended, although more SUSP entries
are present for the file in question.
I hereby ask for more opinions about this. Maybe i misinterpret the old loop.
Background:
CE points to the block and offset where the chain of SUSP entries goes on.
It is needed if the SUSP entry set exceeds the size limit of 255 bytes for
a directory record.
The byte at (ce->blk * block_size + ce->off) is the first byte of the next
SUSP entry in the chain.
Linux hates SUSP crossing block boundaries. So we ISO producers use further
CE entries to hop over block boundaries.
libisofs can produce a Continuation Area with first and only entry CE,
which then points to a new block with more SUSP payload. This happens if a
continuation block offers not much more than 28 free bytes, so that only
the 28
[PATCH 2/4] fs/iso9660: Prevent read past the end of system use area
In the code, the for loop advanced the entry pointer to the
next entry before checking if the next entry is within the
system use area boundary. Another issue in the code was that
there is no check for the size of system use area. For a
corrupted system, the size of system use area can be less than
the size of SUSP entry size (5 bytes). These can cause buffer
overrun. The fixes added the checks to ensure the read is
valid and within the boundary.
Signed-off-by: Lidong Chen
---
grub-core/fs/iso9660.c | 31 +++
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index 4f4cd6165..9170fa820 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -49,6 +49,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
#define GRUB_ISO9660_VOLDESC_PART 3
#define GRUB_ISO9660_VOLDESC_END 255
+#define GRUB_ISO9660_SUSP_HEADER_SZ5
+
/* The head of a volume descriptor. */
struct grub_iso9660_voldesc
{
@@ -272,6 +274,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
grub_off_t off,
if (sua_size <= 0)
return GRUB_ERR_NONE;
+ if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
+return grub_error (GRUB_ERR_BAD_FS, "invalid susp entry size");
+
sua = grub_malloc (sua_size);
if (!sua)
return grub_errno;
@@ -281,10 +286,14 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
grub_off_t off,
if (err)
return err;
- for (entry = (struct grub_iso9660_susp_entry *) sua; (char *) entry < (char
*) sua + sua_size - 1 && entry->len > 0;
- entry = (struct grub_iso9660_susp_entry *)
-((char *) entry + entry->len))
+ entry = (struct grub_iso9660_susp_entry *) sua;
+
+ while (entry->len > 0)
{
+ /* Ensure the entry is within System Use Area */
+ if ((char *) entry + entry->len > (sua + sua_size))
+break;
+
/* The last entry. */
if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
break;
@@ -300,6 +309,15 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
grub_off_t off,
off = grub_le_to_cpu32 (ce->off);
ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
+ if (sua_size <= 0)
+ break;
+
+ if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
+ {
+ grub_free (sua);
+ return grub_error (GRUB_ERR_BAD_FS, "invalid CE entry size");
+ }
+
grub_free (sua);
sua = grub_malloc (sua_size);
if (!sua)
@@ -319,6 +337,11 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
grub_off_t off,
grub_free (sua);
return 0;
}
+
+ entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
+
+ if (((sua + sua_size) - (char *) entry) < GRUB_ISO9660_SUSP_HEADER_SZ)
+break;
}
grub_free (sua);
@@ -574,7 +597,7 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *entry,
char *old;
grub_size_t sz;
- csize = entry->len - 5;
+ csize = entry->len - GRUB_ISO9660_SUSP_HEADER_SZ;
old = ctx->filename;
if (ctx->filename_alloc)
{
--
2.35.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
