Re: [PATCH 1/1] fs/udf: Fix out of bounds access

[Darren Kenny <darren.kenny@oracle.com>]

Hi Li,

In general looks good...

On Thursday, 2023-06-01 at 18:50:19 UTC, Lidong Chen wrote:
> Implemented a boundary check before advancing the allocation
> descriptors pointer.
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
>  grub-core/fs/udf.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/grub-core/fs/udf.c b/grub-core/fs/udf.c
> index 12e88ab62..2359222eb 100644
> --- a/grub-core/fs/udf.c
> +++ b/grub-core/fs/udf.c
> @@ -458,6 +458,7 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>    char *ptr;
>    grub_ssize_t len;
>    grub_disk_addr_t filebytes;
> +  char *end_ptr;
>  
>    switch (U16 (node->block.fe.tag.tag_ident))
>      {
> @@ -476,9 +477,17 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>        return 0;
>      }
>  
> +  end_ptr = (char *) node + get_fshelp_size (node->data);
> +
>    if ((U16 (node->block.fe.icbtag.flags) & GRUB_UDF_ICBTAG_FLAG_AD_MASK)
>        == GRUB_UDF_ICBTAG_FLAG_AD_SHORT)
>      {
> +      if ((end_ptr - ptr) < (grub_ssize_t) sizeof (struct grub_udf_short_ad))
>

Should this probably also be testing ptr < end_ptr?

I wonder if a local macro like this would be useful:

  #define GRUB_UDF_INVALID_STRUCT_PTR(_ptr, _struct) \
  	  ((char *) (_ptr) >= end_ptr ||
      ((grub_ssize_t)(end_ptr - (char*)(_ptr)) < (grub_ssize_t)sizeof(_struct))

or the more positive and succinct version, and subsequent negated (!) test:

  #define GRUB_UDF_VALID_STRUCT_PTR(_ptr, _struct) \
  	  ((char *)(_ptr) <= (end_ptr - sizeof(_struct)))

> +	{
> +	  grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> +	  return 0;
> +	}
> +
>        struct grub_udf_short_ad *ad = (struct grub_udf_short_ad *) ptr;
>  
>        filebytes = fileblock * U32 (node->data->lvd.bsize);
> @@ -528,10 +537,23 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>  	  filebytes -= adlen;
>  	  ad++;
>  	  len -= sizeof (struct grub_udf_short_ad);
> +
> +	  if ((char *) ad >= end_ptr ||
> +	      (end_ptr - (char *) ad) < (grub_ssize_t) sizeof (struct grub_udf_short_ad))
> +	    {
> +	      grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> +	      return 0;
> +	    }
>  	}
>      }
>    else
>      {
> +      if ((end_ptr - ptr) < (grub_ssize_t) sizeof (struct grub_udf_long_ad))
> +	{
> +	  grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> +	  return 0;
> +	}
> +
>        struct grub_udf_long_ad *ad = (struct grub_udf_long_ad *) ptr;
>  
>        filebytes = fileblock * U32 (node->data->lvd.bsize);
> @@ -583,6 +605,13 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>  	  filebytes -= adlen;
>  	  ad++;
>  	  len -= sizeof (struct grub_udf_long_ad);
> +
> +	  if ((char *) ad >= end_ptr ||
> +	      (end_ptr - (char *) ad) < (grub_ssize_t) sizeof (struct grub_udf_long_ad))
> +	    {
> +	      grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> +	      return 0;
> +	    }
>  	}
>      }
>  
> @@ -602,6 +631,7 @@ grub_udf_read_file (grub_fshelp_node_t node,
>      case GRUB_UDF_ICBTAG_FLAG_AD_IN_ICB:
>        {
>  	char *ptr;
> +	char *end_ptr = (char *) node + get_fshelp_size (node->data);
>  
>  	ptr = ((U16 (node->block.fe.tag.tag_ident) == GRUB_UDF_TAG_IDENT_FE) ?
>  	       ((char *) &node->block.fe.ext_attr[0]
> @@ -609,6 +639,12 @@ grub_udf_read_file (grub_fshelp_node_t node,
>  	       ((char *) &node->block.efe.ext_attr[0]
>                  + U32 (node->block.efe.ext_attr_length)));
>  
> +	if (ptr > end_ptr || (ptr + pos) > end_ptr || (ptr + pos + len) > end_ptr)
>

Not sure there is a need for all of these, would the last one not
suffice? Might be worth testing that pos and len are > 0 if only using
that one.

Thanks,

Darren.

> +	  {
> +	    grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> +	    return 0;
> +	  }
> +
>  	grub_memcpy (buf, ptr + pos, len);
>  
>  	return len;
> -- 
> 2.39.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel