On Mon, May 19, 2025 at 09:03:16PM -0500, Andrew Hamilton wrote:
> Correct ntfs_test test failures around attempting to validate attribute
> list entries as attributes. The NTFS code uses common logic in some
> places to parse both attributes and attribute_lists which complicates
> validation. Attribute lists contain different headers including a
> different size of the length field (2 bytes) at offset 4 instead of the
> 4 byte length field used in attributes at offset 4. There are other
> differences as well, but attempting to validate attribute list types
> using attribute header validation was causing failure of the NTFS test
> suite. This change restores some of the validation logic which may be
> shared between attributes and attribute lists to be closer to the
> original logic prior to fixes for previous CVEs. A following commit will
> address some of the implications of removing this validation logic by
> correcting some fuzzer failures (some which are exposed by removing the
> validation in some of the cases).
>
> Fixes: 067b6d225 (fs/ntfs: Implement attribute verification)
>
> Signed-off-by: Andrew Hamilton <adham...@gmail.com>
> ---
> grub-core/fs/ntfs.c | 45 ++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/grub-core/fs/ntfs.c b/grub-core/fs/ntfs.c
> index 3eb70111b..0d087acd8 100644
> --- a/grub-core/fs/ntfs.c
> +++ b/grub-core/fs/ntfs.c
> @@ -219,7 +219,7 @@ validate_attribute (grub_uint8_t *attr, void *end)
>
> /* Return the next attribute if it exists, otherwise return NULL. */
> static grub_uint8_t *
> -next_attribute (grub_uint8_t *curr_attribute, void *end)
> +next_attribute (grub_uint8_t *curr_attribute, void *end, bool validate)
> {
> grub_uint8_t *next = curr_attribute;
>
> @@ -231,7 +231,7 @@ next_attribute (grub_uint8_t *curr_attribute, void *end)
> return NULL;
>
> next += u16at (curr_attribute, 4);
> - if (validate_attribute (next, end) == false)
> + if (validate && validate_attribute (next, end) == false)
> return NULL;
>
> return next;
> @@ -316,13 +316,16 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
> {
> grub_uint8_t *mft_end;
>
> + /* GRUB_NTFS_AF_ALST indicates the attribute list type */
> if (at->flags & GRUB_NTFS_AF_ALST)
> {
> retry:
> while (at->attr_nxt)
> {
> at->attr_cur = at->attr_nxt;
> - at->attr_nxt = next_attribute (at->attr_cur, at->attr_end);
> + /* Go to the next attribute in the list but do not validate */
> + /* because this is the attribute list type. */
May I ask you to stick to the GRUB coding style? [1]
> + at->attr_nxt = next_attribute (at->attr_cur, at->attr_end, false);
> if ((*at->attr_cur == attr) || (attr == 0))
> {
> grub_uint8_t *new_pos, *end;
> @@ -378,7 +381,9 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
> {
> return new_pos;
> }
> - new_pos = next_attribute (new_pos, end);
> + /* Go to the next attribute in the list but do not validate
> */
> + /* because this is the attribute list type. */
Ditto.
> + new_pos = next_attribute (new_pos, end, false);
> }
> grub_error (GRUB_ERR_BAD_FS,
> "can\'t find 0x%X in attribute list",
> @@ -392,7 +397,19 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
> mft_end = at->mft->buf + (at->mft->data->mft_size << GRUB_NTFS_BLK_SHR);
> while (at->attr_cur >= at->mft->buf && at->attr_cur < mft_end &&
> *at->attr_cur != 0xFF)
> {
> - at->attr_nxt = next_attribute (at->attr_cur, at->end);
> + /* We can't use validate_attribute here because this logic
> + * seems to be used for both parsing through attributes
> + * and attribute lists. */
Ditto.
> + grub_uint16_t nsize = u16at (at->attr_cur, 4);
Please do not mix code with variable definitions. Define variables (and
initialize if needed) at the beginning of a function or a given block.
Separate definitions with code with one empty line.
> + if (at->attr_cur + nsize >= at->end ||
> + at->attr_cur + GRUB_NTFS_ATTRIBUTE_HEADER_SIZE >= at->end)
if (at->attr_cur + grub_max (GRUB_NTFS_ATTRIBUTE_HEADER_SIZE, nsize) >=
at->end))
> + {
> + at->attr_nxt = at->attr_cur;
> + break;
> + }
> + else
> + at->attr_nxt = at->attr_cur + nsize;
Is there any chance for overflow here? And in the "if" above?
> if (*at->attr_cur == GRUB_NTFS_AT_ATTRIBUTE_LIST)
> at->attr_end = at->attr_cur;
> if ((*at->attr_cur == attr) || (attr == 0))
> @@ -439,14 +456,18 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
> /* From this point on pa_end is the end of the buffer */
> at->end = pa_end;
>
> - if (validate_attribute (at->attr_nxt, pa_end) == false)
> - return NULL;
> + if (at->attr_end >= pa_end || at->attr_nxt >= pa_end)
> + return NULL;
>
> while (at->attr_nxt)
> {
> if ((*at->attr_nxt == attr) || (attr == 0))
> break;
> - at->attr_nxt = next_attribute (at->attr_nxt, pa_end);
> +
> + grub_uint16_t nxt_offset = u16at (at->attr_nxt, 4);
Again, please move this to the beginning of this code block.
> + at->attr_nxt += nxt_offset;
> + if (nxt_offset == 0 || at->attr_nxt >= (pa_end - 4))
I think at least "(pa_end - 4)" begs for explanation in the comment.
> + at->attr_nxt = NULL;
> }
>
> if (at->attr_nxt >= at->attr_end || at->attr_nxt == NULL)
> @@ -471,7 +492,7 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
> + 1));
> pa = at->attr_nxt + u16at (pa, 4);
>
> - if (validate_attribute (pa, pa_end) == true)
> + if (pa >= pa_end)
> pa = NULL;
>
> while (pa)
> @@ -490,7 +511,9 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
> u32at (pa, 0x10) * (at->mft->data->mft_size <<
> GRUB_NTFS_BLK_SHR),
> at->mft->data->mft_size << GRUB_NTFS_BLK_SHR, 0, 0, 0))
> return NULL;
> - pa = next_attribute (pa, pa_end);
> + pa += u16at(pa, 4);
When you introduce math are you sure there is no chance for overflow?
And a nit, missing space before "("...
Daniel
[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
