Re: [ntfs-3g-devel] [PATCH] Correct validation of multi sector transfer protected records

[Eric Biggers]

On Wed, Jul 27, 2016 at 12:20:24PM +0200, Jean-Pierre André wrote:
> 
> Can you disambiguate the word "sector" here ? This is not
> a physical sector, but an ntfs logical sector whose size
> is NTFS_BLOCK_SIZE (512 bytes). This might not have been
> known to the original developer, and it would be useful
> to have it documented somewhere.
> 

Thanks, I've submitted a revised patch.

--
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

[ntfs-3g-devel] [PATCH] Correct validation of multi sector transfer protected records

[Eric Biggers]

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 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.

Signed-off-by: Eric Biggers 
---
 libntfs-3g/mst.c | 45 +++--
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/libntfs-3g/mst.c b/libntfs-3g/mst.c
index 9dff773..88b9cdb 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 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 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)