@bbockelm commented on this pull request.
> @@ -2229,9 +2246,18 @@ int XrdOfs::rename(const char *old_name, // In
// Apply security, as needed
//
- AUTHORIZE2(client, einfo,
- AOP_Rename, "renaming", old_name, &old_Env,
- AOP_Insert, "renaming to", new_name, &new_Env );
+ // Make a copy of the XrdSecEntity object xattrs as the authorization below may change it.
+ AUTHORIZE(client, &old_Env, AOP_Rename, "renaming", old_name, einfo);
+ if (client) client->eaAPI->Add("request.name", "", true);
Added a comment along these lines.
It's actually a fairly important thing to fix and points out that there's a small security issue with the existing `AUTHORIZE2` macro.
Basically, starting in XRootD 5, authorization checks now have side-effects, mutating the object if they succeed. In my testing, I realized that the outcome of the `AOP_Rename` check added this extended attribute, giving the privilege to the `XrdSecEntity` that caused the `AOP_Insert` check to succeed where it would have otherwise failed.
That is, depending on the ordering of the `AUTHORIZE` checks, the client may-or-may-not have permissions to perform the rename -- an obviously silly setup.
Now, really cleaning this up would require a copy constructor for the `XrdSecEntity`. That was a fairly big undertaking so I decided to take this smaller change - ensure the known extended attribute that might get mutated as a side-effect of the authorization check is reset to a harmless state.
--
Reply to this email directly or view it on GitHub:
https://github.com/xrootd/xrootd/pull/1697#discussion_r867367241
You are receiving this because you are subscribed to this thread.
Message ID: <[log in to unmask]>
########################################################################
Use REPLY-ALL to reply to list
To unsubscribe from the XROOTD-DEV list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1
