 |
 |
@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, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
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