[pdm-devel] [PATCH proxmox] rrd: restrict archive path via regex

[pdm-devel] [PATCH proxmox] rrd: restrict archive path via regex

[Lukas Wagner]

The `rel_path` parameter is used as a relative path inside the `rrdb`
base directory to build the final path for the archive file. Usually,
this is something like 'node/localhost/cpu_avg1'. For PBS, this is fine,
since these paths are hardcoded or derived from safe datastore names. In
PDM however, these paths are built from potentially 'untrusted' (as in,
one could 'pretend' to be a PBS/PVE remote and send malicious data)
metric data points - so we should have additional safe guards in place
to disallow potentially dangerous paths like '../abc' which would escape
the base directory.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-rrd/Cargo.toml   |  4 +++-
 proxmox-rrd/src/cache.rs | 11 +++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/proxmox-rrd/Cargo.toml b/proxmox-rrd/Cargo.toml
index 5ff6ec98..240d00cc 100644
--- a/proxmox-rrd/Cargo.toml
+++ b/proxmox-rrd/Cargo.toml
@@ -16,15 +16,17 @@ proxmox-router = { workspace = true, features = ["cli", "server"] }
 [dependencies]
 anyhow.workspace = true
 bitflags.workspace = true
+const_format.workspace = true
 crossbeam-channel.workspace = true
 log.workspace = true
 nix.workspace = true
+regex.workspace = true
 serde = { workspace = true, features = ["derive"] }
 serde_cbor.workspace = true
 serde_json.workspace = true
 serde_plain.workspace = true
 
-proxmox-schema = { workspace = true, features = [ "api-macro" ] }
+proxmox-schema = { workspace = true, features = [ "api-macro", "api-types" ] }
 proxmox-sys.workspace = true
 proxmox-time.workspace = true
 
diff --git a/proxmox-rrd/src/cache.rs b/proxmox-rrd/src/cache.rs
index 29d46ed5..042b4213 100644
--- a/proxmox-rrd/src/cache.rs
+++ b/proxmox-rrd/src/cache.rs
@@ -8,8 +8,11 @@ use std::thread::spawn;
 use std::time::SystemTime;
 
 use anyhow::{bail, format_err, Error};
+use const_format::concatcp;
 use crossbeam_channel::{bounded, TryRecvError};
 
+use proxmox_schema::api_types::SAFE_ID_REGEX_STR;
+use proxmox_schema::const_regex;
 use proxmox_sys::fs::{create_path, CreateOptions};
 
 use crate::rrd::{AggregationFn, DataSourceType, Database};
@@ -21,6 +24,10 @@ use journal::*;
 mod rrd_map;
 use rrd_map::*;
 
+const_regex! {
+    DATAPOINT_PATH_REGEX = concatcp!(r"^", SAFE_ID_REGEX_STR, r"(/", SAFE_ID_REGEX_STR, r")+$");
+}
+
 /// RRD cache - keep RRD data in RAM, but write updates to disk
 ///
 /// This cache is designed to run as single instance (no concurrent
@@ -214,6 +221,10 @@ impl Cache {
         dst: DataSourceType,
         new_only: bool,
     ) -> Result<(), Error> {
+        if !DATAPOINT_PATH_REGEX.is_match(rel_path) {
+            bail!("invalid datapoint path: {rel_path}");
+        }
+
         let journal_applied = self.apply_journal()?;
 
         self.state
-- 
2.47.3



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel

Re: [pdm-devel] [PATCH proxmox] rrd: restrict archive path via regex

[Thomas Lamprecht]

Am 19.11.25 um 12:11 schrieb Lukas Wagner:
> The `rel_path` parameter is used as a relative path inside the `rrdb`
> base directory to build the final path for the archive file. Usually,
> this is something like 'node/localhost/cpu_avg1'. For PBS, this is fine,
> since these paths are hardcoded or derived from safe datastore names. In
> PDM however, these paths are built from potentially 'untrusted' (as in,
> one could 'pretend' to be a PBS/PVE remote and send malicious data)
> metric data points - so we should have additional safe guards in place
> to disallow potentially dangerous paths like '../abc' which would escape
> the base directory.

thanks for tackling this.

> diff --git a/proxmox-rrd/src/cache.rs b/proxmox-rrd/src/cache.rs
> index 29d46ed5..042b4213 100644
> --- a/proxmox-rrd/src/cache.rs
> +++ b/proxmox-rrd/src/cache.rs
> @@ -8,8 +8,11 @@ use std::thread::spawn;
>  use std::time::SystemTime;
>  
>  use anyhow::{bail, format_err, Error};
> +use const_format::concatcp;
>  use crossbeam_channel::{bounded, TryRecvError};
>  
> +use proxmox_schema::api_types::SAFE_ID_REGEX_STR;
> +use proxmox_schema::const_regex;
>  use proxmox_sys::fs::{create_path, CreateOptions};
>  
>  use crate::rrd::{AggregationFn, DataSourceType, Database};
> @@ -21,6 +24,10 @@ use journal::*;
>  mod rrd_map;
>  use rrd_map::*;
>  
> +const_regex! {
> +    DATAPOINT_PATH_REGEX = concatcp!(r"^", SAFE_ID_REGEX_STR, r"(/", SAFE_ID_REGEX_STR, r")+$");
> +}
> +
>  /// RRD cache - keep RRD data in RAM, but write updates to disk
>  ///
>  /// This cache is designed to run as single instance (no concurrent
> @@ -214,6 +221,10 @@ impl Cache {
>          dst: DataSourceType,
>          new_only: bool,
>      ) -> Result<(), Error> {
> +        if !DATAPOINT_PATH_REGEX.is_match(rel_path) {

Hmm, not really sure if we want to couple this to SAFE_ID here, especially if the
main goal is to avoid breaking out the filesystem.
This approach could probably get away with forbidding `../` explicitly.

That said, IMO this is a bit overfitted to the current usage and problem, we have
quite a few other public function that allow passing rel_path, which might be used
in the future for these things.

For these it's IMO often better to ensure the actual file operations are contained,
i.e. open these rel_path's using openat2 [0] with a dirfd from the basedir directory
and the open_how RESOLVE_BENEATH mode used, so that it's anchored to the correct
directory. nix has bindings for this syscall [1].

We could combine that with your approach (favoring just bailing on matching "../")
to get some better UX, but the "definitive" protection would come from the openat2
usage.

[0]: man openat2 or https://manpages.debian.org/trixie/manpages-dev/openat2.2.en.html
[1]: https://docs.rs/nix/latest/nix/fcntl/fn.openat2.html

> +            bail!("invalid datapoint path: {rel_path}");
> +        }
> +
>          let journal_applied = self.apply_journal()?;
>  
>          self.state



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel