[PATCH 0/1] fs/ntfs3: fix OOB writes in do_action() log replay via unvalidated trailing index entry

[PATCH 0/1] fs/ntfs3: fix OOB writes in do_action() log replay via unvalidated trailing index entry

[Tabriz Hasanli]

Hi Willy, Konstantin,

Thank you for the quick and detailed feedback.

First, apologies about the display name — it was a placeholder from
when I first created the account.  I have since updated it to my real
name: Tabriz Hasanli.

As requested, here is the fix as a proper git format-patch against
mainline (cf72cbb39).  A single patch addresses both check_if_alloc_index()
and check_if_root_index() since they share the same root cause and
the same fix pattern.

Per your note that crafted-FS issues are outside the private
disclosure threat model, I am sending this to the public lists.

Summary of the bug:

check_if_alloc_index() and check_if_root_index() do not stop at
de_is_last() and do not validate the target entry at attr_off.
This allows a crafted NTFS image's $LogFile to direct four
do_action() write operations to an attacker-controlled fake entry
in the trailing gap, producing heap OOB writes of 8 or 56 bytes
during mount.

These are variant siblings of the view.data_off fix (3e127829e57f)
and the DeleteIndexEntryAllocation fix (fc4626bb3656).

Confirmed with userspace ASan harnesses using kernel-faithful
512-byte INDEX_BUFFER geometry (fix_off=0x28, fix_num=2, full
check_index_buffer gate chain).  Harness source files are
available on request.

Thanks,
Tabriz

Tabriz Hasanli (1):
  fs/ntfs3: validate target index entry in
    check_if_alloc_index/check_if_root_index

 fs/ntfs3/fslog.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

--
2.53.0

[PATCH 1/1] fs/ntfs3: validate target index entry in check_if_alloc_index/check_if_root_index

[Tabriz Hasanli]

check_if_alloc_index() and check_if_root_index() walk the index entry
chain to verify that lrh->attr_off lands on a valid entry boundary.
However they have two gaps:

(a) They do not stop at de_is_last() — they walk past the last real
    entry into the trailing gap between the last entry's end and
    hdr->used, which check_index_header() leaves unvalidated.

(b) They do not validate the TARGET entry at attr_off — the walk exits
    as soon as o == attr_off without checking the entry's size, flags,
    or bounds.

This allows attr_off to point to an attacker-controlled "fake entry"
in the trailing gap of a crafted NTFS image's $LogFile.  Four do_action()
cases then use the unvalidated entry:

  SetIndexEntryVcnAllocation / SetIndexEntryVcnRoot:
    de_set_vbn_le() writes 8 bytes at e + e->size - 8.
    Crafted e->size yields an 8-byte OOB write up to ~64KB past the
    buffer.

  UpdateFileNameAllocation / UpdateFileNameRoot:
    memmove() writes 56 bytes at a fixed offset from e.  If e is near
    the buffer end, this overflows by up to 0x50 bytes.

The sibling DeleteIndexEntryAllocation case (commit fc4626bb3656) and
UpdateRecordData cases (commit 3e127829e57f) already carry per-site
guards for the same class of attack; this patch closes the gap at the
validator level so all current and future callers are protected.

Fix both functions to:
  - stop walking at de_is_last()
  - validate the target entry: size is 8-byte aligned, >= minimum
    entry size, and does not extend past hdr->used

Found by static source analysis and confirmed with userspace ASan
harnesses transcribing the kernel's exact validation logic on a
512-byte INDEX_BUFFER with faithful check_index_buffer() geometry.

Signed-off-by: Tabriz Hasanli <cybersec467@gmail.com>
---
 fs/ntfs3/fslog.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/fs/ntfs3/fslog.c b/fs/ntfs3/fslog.c
index ed50c1d0c..d73e3ff1e 100644
--- a/fs/ntfs3/fslog.c
+++ b/fs/ntfs3/fslog.c
@@ -2953,10 +2953,13 @@ static inline bool check_if_root_index(const struct ATTRIB *attr,
 	u32 o = PtrOffset(attr, hdr) + de_off;
 	const struct NTFS_DE *e = Add2Ptr(hdr, de_off);
 	u32 asize = le32_to_cpu(attr->size);
+	u32 used = le32_to_cpu(hdr->used);
+	bool has_subnode = hdr_has_subnode(hdr);
+	u32 min_de = has_subnode ? sizeof(struct NTFS_DE) + sizeof(u64)
+				: sizeof(struct NTFS_DE);
+	u16 esize;
 
 	while (o < ao) {
-		u16 esize;
-
 		if (o >= asize)
 			break;
 
@@ -2964,11 +2967,24 @@ static inline bool check_if_root_index(const struct ATTRIB *attr,
 		if (!esize)
 			break;
 
+		if (de_is_last(e))
+			break;
+
 		o += esize;
+		de_off += esize;
 		e = Add2Ptr(e, esize);
 	}
 
-	return o == ao;
+	if (o != ao)
+		return false;
+
+	/* Validate the target entry itself. */
+	esize = le16_to_cpu(e->size);
+	if (!IS_ALIGNED(esize, 8) || esize < min_de ||
+	    size_add(de_off, esize) > used)
+		return false;
+
+	return true;
 }
 
 static inline bool check_if_alloc_index(const struct INDEX_HDR *hdr,
@@ -2978,10 +2994,12 @@ static inline bool check_if_alloc_index(const struct INDEX_HDR *hdr,
 	u32 o = offsetof(struct INDEX_BUFFER, ihdr) + de_off;
 	const struct NTFS_DE *e = Add2Ptr(hdr, de_off);
 	u32 used = le32_to_cpu(hdr->used);
+	bool has_subnode = hdr_has_subnode(hdr);
+	u32 min_de = has_subnode ? sizeof(struct NTFS_DE) + sizeof(u64)
+				: sizeof(struct NTFS_DE);
+	u16 esize;
 
 	while (o < attr_off) {
-		u16 esize;
-
 		if (de_off >= used)
 			break;
 
@@ -2989,12 +3007,24 @@ static inline bool check_if_alloc_index(const struct INDEX_HDR *hdr,
 		if (!esize)
 			break;
 
+		if (de_is_last(e))
+			break;
+
 		o += esize;
 		de_off += esize;
 		e = Add2Ptr(e, esize);
 	}
 
-	return o == attr_off;
+	if (o != attr_off)
+		return false;
+
+	/* Validate the target entry itself. */
+	esize = le16_to_cpu(e->size);
+	if (!IS_ALIGNED(esize, 8) || esize < min_de ||
+	    size_add(de_off, esize) > used)
+		return false;
+
+	return true;
 }
 
 static inline void change_attr_size(struct MFT_REC *rec, struct ATTRIB *attr,
-- 
2.53.0