From 2393943fbbb8fa2754a823ef7cf2838dde232f5e Mon Sep 17 00:00:00 2001
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
Date: Fri, 27 May 2022 11:13:43 +0200
Subject: [PATCH] api: namespace list: fix restrictive priv checking

This endpoint only lists all accessible namespace, and one doesn't
necessarily needs to have permissions on the parent itself just to
have OK ACLs on deeper down NS.

So, drop the upfront check on parent but explicitly avoid leaking if
a NS exists or not, i.e., only do so if they got access on the parent
NS.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/api2/admin/namespace.rs | 35 ++++++++++++++++++++---------------
 src/backup/hierarchy.rs     |  2 +-
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/src/api2/admin/namespace.rs b/src/api2/admin/namespace.rs
index 153aafab..24d65dc5 100644
--- a/src/api2/admin/namespace.rs
+++ b/src/api2/admin/namespace.rs
@@ -2,17 +2,17 @@ use anyhow::{bail, Error};
 use serde_json::Value;
 
 use pbs_config::CachedUserInfo;
-use proxmox_router::{ApiMethod, Permission, Router, RpcEnvironment};
+use proxmox_router::{http_bail, ApiMethod, Permission, Router, RpcEnvironment};
 use proxmox_schema::*;
 
 use pbs_api_types::{
     Authid, BackupNamespace, NamespaceListItem, Operation, DATASTORE_SCHEMA, NS_MAX_DEPTH_SCHEMA,
-    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PROXMOX_SAFE_ID_FORMAT,
+    PROXMOX_SAFE_ID_FORMAT,
 };
 
 use pbs_datastore::DataStore;
 
-use crate::backup::{check_ns_modification_privs, check_ns_privs};
+use crate::backup::{check_ns_modification_privs, check_ns_privs, NS_PRIVS_OK};
 
 #[api(
     input: {
@@ -94,29 +94,34 @@ pub fn list_namespaces(
 ) -> Result<Vec<NamespaceListItem>, Error> {
     let parent = parent.unwrap_or_default();
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
-    const PRIVS_OK: u64 = PRIV_DATASTORE_MODIFY | PRIV_DATASTORE_BACKUP | PRIV_DATASTORE_AUDIT;
-    // first do a base check to avoid leaking if a NS exists or not
-
-    check_ns_privs(&store, &parent, &auth_id, PRIVS_OK)?;
-
     let user_info = CachedUserInfo::new()?;
+    // get result up-front to avoid cloning NS, it's relatively cheap anyway (no IO normally)
+    let parent_access = check_ns_privs(&store, &parent, &auth_id, NS_PRIVS_OK);
 
     let datastore = DataStore::lookup_datastore(&store, Some(Operation::Read))?;
 
+    let iter = match datastore.recursive_iter_backup_ns_ok(parent, max_depth) {
+        Ok(iter) => iter,
+        // parent NS doesn't exists and user has no privs on it, avoid info leakage.
+        Err(_) if parent_access.is_err() => http_bail!(FORBIDDEN, "permission check failed"),
+        Err(err) => return Err(err),
+    };
+
     let ns_to_item =
         |ns: BackupNamespace| -> NamespaceListItem { NamespaceListItem { ns, comment: None } };
 
-    Ok(datastore
-        .recursive_iter_backup_ns_ok(parent, max_depth)?
+    let namespace_list: Vec<NamespaceListItem> = iter
         .filter(|ns| {
-            if ns.is_root() {
-                return true; // already covered by access permission above
-            }
             let privs = user_info.lookup_privs(&auth_id, &ns.acl_path(&store));
-            privs & PRIVS_OK != 0
+            privs & NS_PRIVS_OK != 0
         })
         .map(ns_to_item)
-        .collect())
+        .collect();
+
+    if namespace_list.is_empty() && parent_access.is_err() {
+        http_bail!(FORBIDDEN, "permission check failed"); // avoid leakage
+    }
+    Ok(namespace_list)
 }
 
 #[api(
diff --git a/src/backup/hierarchy.rs b/src/backup/hierarchy.rs
index 01006972..d229165f 100644
--- a/src/backup/hierarchy.rs
+++ b/src/backup/hierarchy.rs
@@ -142,7 +142,7 @@ impl<'a> ListAccessibleBackupGroups<'a> {
     }
 }
 
-static NS_PRIVS_OK: u64 =
+pub static NS_PRIVS_OK: u64 =
     PRIV_DATASTORE_MODIFY | PRIV_DATASTORE_READ | PRIV_DATASTORE_BACKUP | PRIV_DATASTORE_AUDIT;
 
 impl<'a> Iterator for ListAccessibleBackupGroups<'a> {