Eric Biggers wrote:
> 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 <ebigge...@gmail.com>
> ---
> 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.
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.
Jean-Pierre
> + */
> +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.
>
------------------------------------------------------------------------------
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
