Panic with malformed ntfs in non_resident_value_data_and_position about ntfs HOT 4 CLOSED

@Arthur-PI Code looks good. Note that the /// comment for an error variant is parsed by #[derive(Display) from displaydoc::Display, which is why it contains formatting strings for all fields. Please take care of them as well, so that the context information in the error variant is actually printed.

As we're working asynchronously and I'll be the only one reviewing your PR(s), I think that a single PR with one fix per commit is the best here. Thanks for referencing the test file in each commit - that makes it very easy for me to follow what you've found :)

from ntfs.

Hey @Arthur-PI! Thank you for your fuzzing efforts and for opening this bug report. This is a very good catch!

When writing the ntfs crate, my style was to use the [start..end] range slice only when start and end have already undergone extensive validation and are safe to use. An example for this style is

Offset and length are not necessarily checked and reported individually. Looking at the code again, I also found places where start and end are put into a slice.get(start..end) call, and a single error is returned if get doesn't yield a result:

As you ask for my preferred version to deal with such cases:

• Check if there is a suitable error variant. Don't hesitate to add one. Exact error messages are way better than a single message reused for multiple cases.
  In this case, NtfsError::MissingIndexAllocation is a specific error when an $INDEX_ALLOCATION attribute was expected but not found. It's unrelated to the reported problem.

• Changing [start..end] to .get(start..end) is also how I would fix this particular problem.
  One could also check start and end individually, like it's done in validate_resident_value_sizes, but I'd say it's too easy to miss a check here.

• Add an error variant InvalidNonResidentValueDataRange, with fields position, range, and size, and return that as the specific error for this case.
  Use self.position() for the position field, start..end for the range field, and self.file.ntfs().file_record_size() for the size field. This gives all important context to understand and pinpoint the error.
  Doing it like this is also 100% consistent with what's done for slice.get(start..end) in index_entry.rs.

• Use .get(start..end).ok_or(.....)? instead of an if-else block.
  This doesn't add an indendation level for the happy case, and therefore scales better if we later need to introduce another check with early return in case of an error.

Again, thanks a lot for your help with this!

from ntfs.

Thanks you for this very complete response, those explanations are really helpful 👍🏼.
So I've taking into account what you've said and I made the fix accordingly, here is the new version.

pub(crate) fn non_resident_value_data_and_position(&self) -> Result<(&'f [u8], NtfsPosition)> {
    debug_assert!(!self.is_resident());
    let start = self.offset + self.non_resident_value_data_runs_offset() as usize;
    let end = self.offset + self.attribute_length() as usize;
    let position = self.file.position() + start;
    let data = &self.file.record_data().get(start..end).ok_or(
        NtfsError::InvalidNonResidentValueDataRange {
            position,
            range: start..end,
            size: self.file.record_data().len(),
        },
    )?;
    Ok((data, position))
}

With this new error:

/// The range of non resident value data when slicing is invalid
InvalidNonResidentValueDataRange {
    position: NtfsPosition,
    range: Range<usize>,
    size: usize,
}

Now I have fix for 8 bugs similar to this one, do you prefer one PR for each fix or one big PR with one fix per commit. As you wish. I'll also provide you all the test files that create those crashes (I'll assign a test case by commit) if you want do dig a little bit in why this happens and if the problem is deeper in the parsing process.

Thank you for your work and help.

from ntfs.

Fixed in #23

from ntfs.