I found that the validation contained an off-by-one error. The
expression '(u32)(usa_ofs + (usa_count * 2)) > size' used 'usa_count'
after it had been decremented to skip the update sequence number entry.
Consequently, the code could read out of bounds, up to two bytes past the
end of the MST-protected record.
Furthermore, as documented in the comment in layout.h for "NTFS_RECORD"
and also on MSDN for "MULTI_SECTOR_HEADER", the update sequence array
must end before the last le16 in the first logical sector --- not merely
before the end of the record.
Fix the validation and move it into a helper function, as it was done
identically in the read and write paths.
[v2: Say "logical sector" instead of "sector", and document NTFS_BLOCK_SIZE.]
Signed-off-by: Eric Biggers <ebigge...@gmail.com>
---
include/ntfs-3g/layout.h | 6 ++++++
libntfs-3g/mst.c | 45 +++++++++++++++++++++++++++------------------
2 files changed, 33 insertions(+), 18 deletions(-)
diff --git a/include/ntfs-3g/layout.h b/include/ntfs-3g/layout.h
index ddd29c1..98380de 100644
--- a/include/ntfs-3g/layout.h
+++ b/include/ntfs-3g/layout.h
@@ -161,6 +161,12 @@ typedef enum {
#define ntfs_is_empty_recordp(p) ( ntfs_is_magicp(p, empty) )
+/*
+ * The size of a logical sector in bytes, used as the sequence number stride
for
+ * multi-sector transfers. This is intended to be less than or equal to the
+ * physical sector size, since if this were greater than the physical sector
+ * size, then incomplete multi-sector transfers may not be detected.
+ */
#define NTFS_BLOCK_SIZE 512
#define NTFS_BLOCK_SIZE_BITS 9
diff --git a/libntfs-3g/mst.c b/libntfs-3g/mst.c
index 9dff773..af17edb 100644
--- a/libntfs-3g/mst.c
+++ b/libntfs-3g/mst.c
@@ -31,6 +31,21 @@
#include "mst.h"
#include "logging.h"
+/*
+ * Basic validation of a NTFS multi-sector record. The record size must be a
+ * multiple of the logical sector size; and the update sequence array must be
+ * properly aligned, of the expected length, and must end before the last le16
+ * in the first logical sector.
+ */
+static BOOL
+is_valid_record(u32 size, u16 usa_ofs, u16 usa_count)
+{
+ return size % NTFS_BLOCK_SIZE == 0 &&
+ usa_ofs % 2 == 0 &&
+ usa_count == 1 + (size / NTFS_BLOCK_SIZE) &&
+ usa_ofs + ((u32)usa_count * 2) <= NTFS_BLOCK_SIZE - 2;
+}
+
/**
* ntfs_mst_post_read_fixup - deprotect multi sector transfer protected data
* @b: pointer to the data to deprotect
@@ -57,12 +72,9 @@ int ntfs_mst_post_read_fixup_warn(NTFS_RECORD *b, const u32
size,
/* Setup the variables. */
usa_ofs = le16_to_cpu(b->usa_ofs);
- /* Decrement usa_count to get number of fixups. */
- usa_count = le16_to_cpu(b->usa_count) - 1;
- /* Size and alignment checks. */
- if (size & (NTFS_BLOCK_SIZE - 1) || usa_ofs & 1 ||
- (u32)(usa_ofs + (usa_count * 2)) > size ||
- (size >> NTFS_BLOCK_SIZE_BITS) != usa_count) {
+ usa_count = le16_to_cpu(b->usa_count);
+
+ if (!is_valid_record(size, usa_ofs, usa_count)) {
errno = EINVAL;
if (warn) {
ntfs_log_perror("%s: magic: 0x%08lx size: %ld "
@@ -91,7 +103,7 @@ int ntfs_mst_post_read_fixup_warn(NTFS_RECORD *b, const u32
size,
/*
* Check for incomplete multi sector transfer(s).
*/
- while (usa_count--) {
+ while (--usa_count) {
if (*data_pos != usn) {
/*
* Incomplete multi sector transfer detected! )-:
@@ -109,10 +121,10 @@ int ntfs_mst_post_read_fixup_warn(NTFS_RECORD *b, const
u32 size,
data_pos += NTFS_BLOCK_SIZE/sizeof(u16);
}
/* Re-setup the variables. */
- usa_count = le16_to_cpu(b->usa_count) - 1;
+ usa_count = le16_to_cpu(b->usa_count);
data_pos = (u16*)b + NTFS_BLOCK_SIZE/sizeof(u16) - 1;
/* Fixup all sectors. */
- while (usa_count--) {
+ while (--usa_count) {
/*
* Increment position in usa and restore original data from
* the usa into the data buffer.
@@ -171,12 +183,9 @@ int ntfs_mst_pre_write_fixup(NTFS_RECORD *b, const u32
size)
}
/* Setup the variables. */
usa_ofs = le16_to_cpu(b->usa_ofs);
- /* Decrement usa_count to get number of fixups. */
- usa_count = le16_to_cpu(b->usa_count) - 1;
- /* Size and alignment checks. */
- if (size & (NTFS_BLOCK_SIZE - 1) || usa_ofs & 1 ||
- (u32)(usa_ofs + (usa_count * 2)) > size ||
- (size >> NTFS_BLOCK_SIZE_BITS) != usa_count) {
+ usa_count = le16_to_cpu(b->usa_count);
+
+ if (!is_valid_record(size, usa_ofs, usa_count)) {
errno = EINVAL;
ntfs_log_perror("%s", __FUNCTION__);
return -1;
@@ -195,7 +204,7 @@ int ntfs_mst_pre_write_fixup(NTFS_RECORD *b, const u32 size)
/* Position in data of first le16 that needs fixing up. */
data_pos = (le16*)b + NTFS_BLOCK_SIZE/sizeof(le16) - 1;
/* Fixup all sectors. */
- while (usa_count--) {
+ while (--usa_count) {
/*
* Increment the position in the usa and save the
* original data from the data buffer into the usa.
@@ -223,7 +232,7 @@ void ntfs_mst_post_write_fixup(NTFS_RECORD *b)
u16 *usa_pos, *data_pos;
u16 usa_ofs = le16_to_cpu(b->usa_ofs);
- u16 usa_count = le16_to_cpu(b->usa_count) - 1;
+ u16 usa_count = le16_to_cpu(b->usa_count);
ntfs_log_trace("Entering\n");
@@ -234,7 +243,7 @@ void ntfs_mst_post_write_fixup(NTFS_RECORD *b)
data_pos = (u16*)b + NTFS_BLOCK_SIZE/sizeof(u16) - 1;
/* Fixup all sectors. */
- while (usa_count--) {
+ while (--usa_count) {
/*
* Increment position in usa and restore original data from
* the usa into the data buffer.
--
2.9.0
------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are
consuming the most bandwidth. Provides multi-vendor support for NetFlow,
J-Flow, sFlow and other flows. Make informed decisions using capacity planning
reports.http://sdm.link/zohodev2dev
_______________________________________________
ntfs-3g-devel mailing list
ntfs-3g-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ntfs-3g-devel
