From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 38CAD1FF391 for ; Wed, 12 Jun 2024 15:17:01 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4DDC811578; Wed, 12 Jun 2024 15:17:36 +0200 (CEST) From: Christian Ebner To: pbs-devel@lists.proxmox.com Date: Wed, 12 Jun 2024 15:17:12 +0200 Message-Id: <20240612131713.133353-3-c.ebner@proxmox.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20240612131713.133353-1-c.ebner@proxmox.com> References: <20240612131713.133353-1-c.ebner@proxmox.com> MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.027 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: [pbs-devel] [PATCH v4 pxar 2/3] accessor: adapt and restrict contents access X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 --- changes since version 3: - move `ContentRange` to accessor - move payload header check to `FileContentsImpl` new, make it private - drop unsafe for `open_contents_at_range` - refactor src/accessor/aio.rs | 16 ++++++----- src/accessor/mod.rs | 68 ++++++++++++++++++++++++++++++++------------ src/accessor/sync.rs | 16 ++++++----- 3 files changed, 68 insertions(+), 32 deletions(-) diff --git a/src/accessor/aio.rs b/src/accessor/aio.rs index 73b1025..eb89f8f 100644 --- 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 Accessor { /// /// 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) -> FileContents { - FileContents { - inner: unsafe { self.inner.open_contents_at_range(range) }, + pub async fn open_contents_at_range( + &self, + range: &ContentRange, + ) -> io::Result> { + 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 FileEntry { } /// For use with unsafe accessor methods. - pub fn content_range(&self) -> io::Result>> { + pub fn content_range(&self) -> io::Result> { self.inner.content_range() } diff --git a/src/accessor/mod.rs b/src/accessor/mod.rs index 92d689d..48605eb 100644 --- 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, + // Optional payload ref + payload_ref: Option, +} + /// awaitable version of `ReadAt`. async fn read_at(input: &T, buf: &mut [u8], offset: u64) -> io::Result where @@ -335,13 +345,12 @@ impl AccessorImpl { }) } - /// Allow opening arbitrary contents from a specific range. - pub unsafe fn open_contents_at_range(&self, range: Range) -> FileContentsImpl { - 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::new(&self.input, range).await } /// Following a hardlink breaks a couple of conventions we otherwise have, particularly we will @@ -758,7 +767,7 @@ impl FileEntryImpl { } /// For use with unsafe accessor methods. - pub fn content_range(&self) -> io::Result>> { + pub fn content_range(&self) -> io::Result> { match self.entry.kind { EntryKind::File { offset: None, .. } => { io_bail!("cannot open file, reader provided no offset") @@ -767,7 +776,10 @@ impl FileEntryImpl { 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 FileEntryImpl { payload_offset: Some(payload_offset), } => { let start_offset = payload_offset + size_of::() 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 FileEntryImpl { 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 { } impl FileContentsImpl { - pub fn new(input: T, range: Range) -> Self { - Self { input, range } + async fn new( + input: &PxarVariant)>, + range: &ContentRange, + ) -> io::Result { + 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 df2ed23..76e8c03 100644 --- 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 Accessor { /// /// 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) -> FileContents { - FileContents { - inner: unsafe { self.inner.open_contents_at_range(range) }, + pub unsafe fn open_contents_at_range( + &self, + range: &ContentRange, + ) -> io::Result> { + Ok(FileContents { + inner: poll_result_once(self.inner.open_contents_at_range(range))?, at: 0, - } + }) } /// Following a hardlink. @@ -291,7 +293,7 @@ impl FileEntry { } /// For use with unsafe accessor methods. - pub fn content_range(&self) -> io::Result>> { + pub fn content_range(&self) -> io::Result> { self.inner.content_range() } -- 2.39.2 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel