[pbs-devel] [PATCH proxmox-backup v8 4/4] fix: api: avoid race condition in set_backup_owner

[Shannon Sterz]

On Tue Mar 25, 2025 at 11:00 AM CET, Christian Ebner wrote:
> On 3/24/25 13:51, Shannon Sterz wrote:
>> when two clients change the owner of a backup store, a race condition
>> arose. add locking to avoid this.
>>
>> Signed-off-by: Shannon Sterz <s.sterz at proxmox.com>
>> ---
>>   src/api2/admin/datastore.rs | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>> index 483e595c..3e532636 100644
>> --- a/src/api2/admin/datastore.rs
>> +++ b/src/api2/admin/datastore.rs
>> @@ -7,7 +7,7 @@ use std::os::unix::ffi::OsStrExt;
>>   use std::path::{Path, PathBuf};
>>   use std::sync::Arc;
>>
>> -use anyhow::{bail, format_err, Error};
>> +use anyhow::{bail, format_err, Context, Error};
>>   use futures::*;
>>   use hyper::http::request::Parts;
>>   use hyper::{header, Body, Response, StatusCode};
>> @@ -2347,10 +2347,9 @@ pub async fn set_backup_owner(
>>           let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
>>
>>           let backup_group = datastore.backup_group(ns, backup_group);
>> +        let owner = backup_group.get_owner()?;
>
> this needs to read the content from the owner file
>
>>
>>           if owner_check_required {
>> -            let owner = backup_group.get_owner()?;
>> -
>>               let allowed = match (owner.is_token(), new_owner.is_token()) {
>>                   (true, true) => {
>>                       // API token to API token, owned by same user
>> @@ -2397,6 +2396,14 @@ pub async fn set_backup_owner(
>>               );
>>           }
>>
>> +        let _guard = backup_group
>> +            .lock()
>> +            .with_context(|| format!("while setting the owner of group '{backup_group:?}'"))?;
>> +
>> +        if owner != backup_group.get_owner()? {
>
> same here.
>
>> +            bail!("{owner} does not own this group anymore");
>> +        }
>> +
>>           backup_group.set_owner(&new_owner, true)?;
>>
>>           Ok(())
>
> reading from the filesystem is more expensive than the ownership checks
> if required, so IMO it is better to lock the whole operation instead of
> reading the owner file twice. Or is this done to avoid locking over and
> over by unauthorized users?

yes that function has `Permission::Anybody` if you locked the file right
away, creating a dos attack by *any* logged in user for *any* group
would be trivial. getting the owner might be a bit more expensive, but
this way we can still handle multiple requests in parallel easily and
only hold the lock for the part that actually requires it. hence, the
dos potential is lower (imo) and group owners should not change *that*
often in normal operations. so the performance hit is acceptable.