[pdm-devel] [PATCH proxmox v2] rrd: update_value: restrict archive file path

[pdm-devel] [PATCH proxmox v2] rrd: update_value: restrict archive file path

[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.

This commit adds a check which ensures that the path does not contain
'../' which could be used to access a parent directory.

In the future, it would be best to ensure that the actual file
operations are contained. This could be achieved by using openat2 with a
dirfd from the basedir directory and the open_how RESOLVE_BENEATH mode
being used, so that it's anchored to the correct directory. This is a
bigger change, which is why it is not done at this very moment.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-rrd/src/cache.rs | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/proxmox-rrd/src/cache.rs b/proxmox-rrd/src/cache.rs
index 29d46ed5..fe5c18ec 100644
--- a/proxmox-rrd/src/cache.rs
+++ b/proxmox-rrd/src/cache.rs
@@ -214,6 +214,10 @@ impl Cache {
         dst: DataSourceType,
         new_only: bool,
     ) -> Result<(), Error> {
+        if rel_path.contains("../") {
+            bail!("invalid path when trying to update value: {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 v2] rrd: update_value: restrict archive file path

[Lukas Wagner]

On Thu Nov 20, 2025 at 12:00 PM CET, Lukas Wagner wrote:
> 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.
>
> This commit adds a check which ensures that the path does not contain
> '../' which could be used to access a parent directory.
>
> In the future, it would be best to ensure that the actual file
> operations are contained. This could be achieved by using openat2 with a
> dirfd from the basedir directory and the open_how RESOLVE_BENEATH mode
> being used, so that it's anchored to the correct directory. This is a
> bigger change, which is why it is not done at this very moment.
>
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  proxmox-rrd/src/cache.rs | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/proxmox-rrd/src/cache.rs b/proxmox-rrd/src/cache.rs
> index 29d46ed5..fe5c18ec 100644
> --- a/proxmox-rrd/src/cache.rs
> +++ b/proxmox-rrd/src/cache.rs
> @@ -214,6 +214,10 @@ impl Cache {
>          dst: DataSourceType,
>          new_only: bool,
>      ) -> Result<(), Error> {
> +        if rel_path.contains("../") {
> +            bail!("invalid path when trying to update value: {rel_path}");
> +        }
> +
>          let journal_applied = self.apply_journal()?;
>  
>          self.state

Ping, just so this is not overlooked :)


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

[pdm-devel] applied: [PATCH proxmox v2] rrd: update_value: restrict archive file path

[Thomas Lamprecht]

On Thu, 20 Nov 2025 12:00:33 +0100, Lukas Wagner wrote:
> 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.
> 
> [...]

Applied, thanks!

[1/1] rrd: update_value: restrict archive file path
      commit: 948b0f0f3b66656cb4e5e9d00e7a2bc8ce9fc0ea


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