accessor: adapt and restrict contents access

Add checks for split variant inputs when accessing the payload
contents via the accessor instance. Both cases, accessing via the
safe `contents` method and via the previousely unsafe
`open_contents_at_range` call are covered.

Reduce possible misuse by wrapping the current plain content range
into an opaque `ContentRange` type with an additional optional
payload reference field to check consistency between the payload
reference encoded in the metadata archive and the payload header'
found in the payload data archive.

Because of the additional type wrapping and the payload header check,
the `open_contents_at_range` is considered safe now, dropping the
previously unsafe implementation.
The corresponding interfaces have been adapted accordingly.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>

diff --git a/src/accessor/aio.rs b/src/accessor/aio.rs
index 73b1025281c24fa512bd13f9a6d8cdefe8f7b8ea..eb89f8f0268315e83e5ba8359ca2fe363e36020f 100644 (file)
--- a/src/accessor/aio.rs
+++ b/src/accessor/aio.rs
@@ -7,14 +7,13 @@
 use std::future::Future;
 use std::io;
 use std::mem;
-use std::ops::Range;
 use std::os::unix::fs::FileExt;
 use std::path::Path;
 use std::pin::Pin;
 use std::sync::Arc;
 use std::task::{Context, Poll};
 
-use crate::accessor::{self, cache::Cache, MaybeReady, ReadAt, ReadAtOperation};
+use crate::accessor::{self, cache::Cache, ContentRange, MaybeReady, ReadAt, ReadAtOperation};
 use crate::decoder::aio::Decoder;
 use crate::format::GoodbyeItem;
 use crate::util;
@@ -153,13 +152,16 @@ impl<T: Clone + ReadAt> Accessor<T> {
     ///
     /// This will provide a reader over an arbitrary range of the archive file, so unless this
     /// comes from a actual file entry data, the contents might not make much sense.
-    pub unsafe fn open_contents_at_range(&self, range: Range<u64>) -> FileContents<T> {
-        FileContents {
-            inner: unsafe { self.inner.open_contents_at_range(range) },
+    pub async fn open_contents_at_range(
+        &self,
+        range: &ContentRange,
+    ) -> io::Result<FileContents<T>> {
+        Ok(FileContents {
+            inner: self.inner.open_contents_at_range(range).await?,
             at: 0,
             buffer: Vec::new(),
             future: None,
-        }
+        })
     }
 
     /// Following a hardlink.
@@ -235,7 +237,7 @@ impl<T: Clone + ReadAt> FileEntry<T> {
     }
 
     /// For use with unsafe accessor methods.
-    pub fn content_range(&self) -> io::Result<Option<Range<u64>>> {
+    pub fn content_range(&self) -> io::Result<Option<ContentRange>> {
         self.inner.content_range()
     }
 

diff --git a/src/accessor/mod.rs b/src/accessor/mod.rs
index 92d689dac0065f24912a484dd7c8e5a3347c9f13..48605ebe412e8dc2992aebac6b00826b61b28b0f 100644 (file)
--- a/src/accessor/mod.rs
+++ b/src/accessor/mod.rs
@@ -17,7 +17,7 @@ use endian_trait::Endian;
 
 use crate::binary_tree_array;
 use crate::decoder::{self, DecoderImpl};
-use crate::format::{self, FormatVersion, GoodbyeItem};
+use crate::format::{self, FormatVersion, GoodbyeItem, PayloadRef};
 use crate::util;
 use crate::{Entry, EntryKind, PxarVariant};
 
@@ -54,6 +54,16 @@ impl EntryRangeInfo {
     }
 }
 
+/// Stores a content range to be accessed via the `Accessor` as well as the payload reference to
+/// perform consistency checks on payload references for archives accessed via split variant input.
+#[derive(Clone)]
+pub struct ContentRange {
+    // Range of the content
+    content: Range<u64>,
+    // Optional payload ref
+    payload_ref: Option<PayloadRef>,
+}
+
 /// awaitable version of `ReadAt`.
 async fn read_at<T>(input: &T, buf: &mut [u8], offset: u64) -> io::Result<usize>
 where
@@ -335,13 +345,12 @@ impl<T: Clone + ReadAt> AccessorImpl<T> {
         })
     }
 
-    /// Allow opening arbitrary contents from a specific range.
-    pub unsafe fn open_contents_at_range(&self, range: Range<u64>) -> FileContentsImpl<T> {
-        if let Some((payload_input, _)) = &self.input.payload() {
-            FileContentsImpl::new(payload_input.clone(), range)
-        } else {
-            FileContentsImpl::new(self.input.archive().clone(), range)
-        }
+    /// Open contents at provided range
+    pub async fn open_contents_at_range(
+        &self,
+        range: &ContentRange,
+    ) -> io::Result<FileContentsImpl<T>> {
+        FileContentsImpl::new(&self.input, range).await
     }
 
     /// Following a hardlink breaks a couple of conventions we otherwise have, particularly we will
@@ -758,7 +767,7 @@ impl<T: Clone + ReadAt> FileEntryImpl<T> {
     }
 
     /// For use with unsafe accessor methods.
-    pub fn content_range(&self) -> io::Result<Option<Range<u64>>> {
+    pub fn content_range(&self) -> io::Result<Option<ContentRange>> {
         match self.entry.kind {
             EntryKind::File { offset: None, .. } => {
                 io_bail!("cannot open file, reader provided no offset")
@@ -767,7 +776,10 @@ impl<T: Clone + ReadAt> FileEntryImpl<T> {
                 size,
                 offset: Some(offset),
                 payload_offset: None,
-            } => Ok(Some(offset..(offset + size))),
+            } => Ok(Some(ContentRange {
+                content: offset..(offset + size),
+                payload_ref: None,
+            })),
             // Payload offset beats regular offset if some
             EntryKind::File {
                 size,
@@ -775,7 +787,13 @@ impl<T: Clone + ReadAt> FileEntryImpl<T> {
                 payload_offset: Some(payload_offset),
             } => {
                 let start_offset = payload_offset + size_of::<format::Header>() as u64;
-                Ok(Some(start_offset..start_offset + size))
+                Ok(Some(ContentRange {
+                    content: start_offset..start_offset + size,
+                    payload_ref: Some(PayloadRef {
+                        offset: payload_offset,
+                        size,
+                    }),
+                }))
             }
             _ => Ok(None),
         }
@@ -785,11 +803,8 @@ impl<T: Clone + ReadAt> FileEntryImpl<T> {
         let range = self
             .content_range()?
             .ok_or_else(|| io_format_err!("not a file"))?;
-        if let Some((ref payload_input, _)) = self.input.payload() {
-            Ok(FileContentsImpl::new(payload_input.clone(), range))
-        } else {
-            Ok(FileContentsImpl::new(self.input.archive().clone(), range))
-        }
+
+        FileContentsImpl::new(&self.input, &range).await
     }
 
     #[inline]
@@ -897,8 +912,25 @@ pub(crate) struct FileContentsImpl<T> {
 }
 
 impl<T: Clone + ReadAt> FileContentsImpl<T> {
-    pub fn new(input: T, range: Range<u64>) -> Self {
-        Self { input, range }
+    async fn new(
+        input: &PxarVariant<T, (T, Range<u64>)>,
+        range: &ContentRange,
+    ) -> io::Result<Self> {
+        let (input, range) = if let Some((payload_input, payload_range)) = input.payload() {
+            if let Some(payload_ref) = &range.payload_ref {
+                let header: format::Header =
+                    read_entry_at(payload_input, payload_ref.offset).await?;
+                format::check_payload_header_and_size(&header, payload_ref.size)?;
+            }
+            if payload_range.start > range.content.start || payload_range.end < range.content.end {
+                io_bail!("out of range access for payload");
+            }
+            (payload_input.clone(), range.content.clone())
+        } else {
+            (input.archive().clone(), range.content.clone())
+        };
+
+        Ok(Self { input, range })
     }
 
     #[inline]

diff --git a/src/accessor/sync.rs b/src/accessor/sync.rs
index df2ed23708a28ff959d376fbab98c9da3624bf55..76e8c03b0af4c27976ac4a0c33b3a6a2273b6aab 100644 (file)
--- a/src/accessor/sync.rs
+++ b/src/accessor/sync.rs
@@ -1,14 +1,13 @@
 //! Blocking `pxar` random access handling.
 
 use std::io;
-use std::ops::Range;
 use std::os::unix::fs::FileExt;
 use std::path::Path;
 use std::pin::Pin;
 use std::sync::Arc;
 use std::task::Context;
 
-use crate::accessor::{self, cache::Cache, MaybeReady, ReadAt, ReadAtOperation};
+use crate::accessor::{self, cache::Cache, ContentRange, MaybeReady, ReadAt, ReadAtOperation};
 use crate::decoder::Decoder;
 use crate::format::GoodbyeItem;
 use crate::util::poll_result_once;
@@ -142,11 +141,14 @@ impl<T: Clone + ReadAt> Accessor<T> {
     ///
     /// This will provide a reader over an arbitrary range of the archive file, so unless this
     /// comes from a actual file entry data, the contents might not make much sense.
-    pub unsafe fn open_contents_at_range(&self, range: Range<u64>) -> FileContents<T> {
-        FileContents {
-            inner: unsafe { self.inner.open_contents_at_range(range) },
+    pub unsafe fn open_contents_at_range(
+        &self,
+        range: &ContentRange,
+    ) -> io::Result<FileContents<T>> {
+        Ok(FileContents {
+            inner: poll_result_once(self.inner.open_contents_at_range(range))?,
             at: 0,
-        }
+        })
     }
 
     /// Following a hardlink.
@@ -291,7 +293,7 @@ impl<T: Clone + ReadAt> FileEntry<T> {
     }
 
     /// For use with unsafe accessor methods.
-    pub fn content_range(&self) -> io::Result<Option<Range<u64>>> {
+    pub fn content_range(&self) -> io::Result<Option<ContentRange>> {
         self.inner.content_range()
     }