[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