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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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