[PATCH] ntfs: fix name offset validation in ntfs_non_resident_attr_value_is_valid

Hongling Zeng posted 1 patch 1 month, 3 weeks ago
There is a newer version of this series
fs/ntfs/attrib.c | 33 ++++++++++++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)
[PATCH] ntfs: fix name offset validation in ntfs_non_resident_attr_value_is_valid
Posted by Hongling Zeng 1 month, 3 weeks ago
ntfs_attr_update_meta() performs memmove operations on attribute names
when converting between sparse and non-sparse attributes:

- Converting to sparse shifts the name forward by 8 bytes
  (name_offset + 8)
- Converting from sparse shifts the name backward by 8 bytes
  (name_offset - 8)

However, the validator does not check that name_offset is within safe
boundaries for these operations. A malicious MFT record could set
name_offset such that:

1. The name is positioned at the very end of a non-sparse attribute.
   Converting to sparse would shift the name forward by 8 bytes,
   writing beyond the attribute boundary.

2. The name overlaps with the mapping pairs, causing corruption during
   conversion.

Add validation to ensure:
- For named attributes, name_offset is within valid bounds
- Name does not extend beyond the attribute or overlap with mapping pairs
- For non-sparse attributes, name_end + 8 fits within attr_len to allow
  room for the forward shift when becoming sparse

Note: name_offset validation only applies when name_length != 0, as
unnamed attributes use name_offset = 0 which is valid.

Fixes: 7e2a1c554bc4 ("ntfs: Fix min_len for compressed/sparse attributes in ntfs_non_resident_attr_value_is_valid()")
Cc: stable@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
 fs/ntfs/attrib.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index d354c3b0fae1..778b13cf9762 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -693,6 +693,9 @@ static bool ntfs_non_resident_attr_value_is_valid(const struct attr_record *a)
 	u32 attr_len;
 	u32 min_len;
 	u16 mp_offset;
+	u16 name_offset;
+	u16 name_len;
+	u32 name_end;
 
 	attr_len = le32_to_cpu(a->length);
 	min_len = offsetof(struct attr_record, data.non_resident.initialized_size) +
@@ -706,7 +709,35 @@ static bool ntfs_non_resident_attr_value_is_valid(const struct attr_record *a)
 		return false;
 
 	mp_offset = le16_to_cpu(a->data.non_resident.mapping_pairs_offset);
-	return mp_offset >= min_len && mp_offset <= attr_len;
+	if (mp_offset < min_len || mp_offset > attr_len)
+		return false;
+
+	/*
+	 * Validate name_offset for named attributes.
+	 * Unnamed attributes have name_length = 0 and name_offset may be 0.
+	 */
+	name_len = le16_to_cpu(a->name_length);
+	if (name_len) {
+		name_offset = le16_to_cpu(a->name_offset);
+
+		if (name_offset < min_len || name_offset >= attr_len)
+			return false;
+
+		name_end = name_offset + name_len * sizeof(__le16);
+		if (name_end > attr_len || name_end > mp_offset)
+			return false;
+
+		/*
+		 * For non-sparse -> sparse conversion, name shifts forward
+		 * by 8 bytes. Ensure there's room for the shift.
+		 */
+		if (!(a->flags & ATTR_IS_SPARSE) &&
+		     name_end + 8 > attr_len)
+			return false;
+	}
+
+	return true;
+
 }
 
 static bool ntfs_attr_value_is_valid(struct ntfs_volume *vol,
-- 
2.25.1
Re: [PATCH] ntfs: fix name offset validation in ntfs_non_resident_attr_value_is_valid
Posted by Namjae Jeon 1 month, 3 weeks ago
> @@ -706,7 +709,35 @@ static bool ntfs_non_resident_attr_value_is_valid(const struct attr_record *a)
>                 return false;
>
>         mp_offset = le16_to_cpu(a->data.non_resident.mapping_pairs_offset);
> -       return mp_offset >= min_len && mp_offset <= attr_len;
> +       if (mp_offset < min_len || mp_offset > attr_len)
> +               return false;
> +
> +       /*
> +        * Validate name_offset for named attributes.
> +        * Unnamed attributes have name_length = 0 and name_offset may be 0.
> +        */
> +       name_len = le16_to_cpu(a->name_length);
name_length is a u8, not a __le16 field, so it must not be passed to
le16_to_cpu().

+               /*
+                * For non-sparse -> sparse conversion, name shifts forward
+                * by 8 bytes. Ensure there's room for the shift.
+                */
+               if (!(a->flags & ATTR_IS_SPARSE) &&
+                    name_end + 8 > attr_len)
+                       return false;
This check should not depend on name_length. Unnamed attributes also
need eight bytes of room when converting from non-sparse to sparse,
because compressed_size is added and mapping_pairs_offset is advanced
by eight bytes. Also, compressed attributes do not go through this
conversion, so the condition should exclude both sparse and compressed
attributes. how about adding the following check outside the if
(a->name_length) block?

                   if (!(a->flags & (ATTR_IS_SPARSE | ATTR_COMPRESSION_MASK)) &&
                          attr_len - mp_offset <
                                 sizeof(a->data.non_resident.compressed_size))
                          return false;
Please check the attached patch.
Thanks.