Re: [pve-devel] [PATCH manager] fix #3389: Skip CSRF token check also for API tokens on file upload

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 22.04.21 10:05, Fabian Grünbichler wrote:
> On April 21, 2021 1:59 pm, Lorenz Stechauner wrote:
>> On file upload, the check for CSRF tokens was already skipped
>> when performing user authentication.This now happens for API
>> tokens also.
>>
>> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
>> ---
>>  PVE/HTTPServer.pm | 34 +++++++++++++++++-----------------
>>  1 file changed, 17 insertions(+), 17 deletions(-)
>>
>> diff --git a/PVE/HTTPServer.pm b/PVE/HTTPServer.pm
>> index 64976c7c..63b8583e 100755
>> --- a/PVE/HTTPServer.pm
>> +++ b/PVE/HTTPServer.pm
>> @@ -79,8 +79,8 @@ sub auth_handler {
>>  
>>      if ($require_auth) {
>>  	if ($api_token) {
>> +	    # returns tokenid actually
>>  	    $username = PVE::AccessControl::verify_token($api_token);
>> -	    $rpcenv->set_user($username); #actually tokenid in this case
>>  	} else {
>>  	    die "No ticket\n" if !$ticket;
>>  
>> @@ -94,25 +94,25 @@ sub auth_handler {
>>  		die "No ticket\n"
>>  		    if ($rel_uri ne '/access/tfa' || $method ne 'POST');
>>  	    }
>> +	}
>>  
>> -	    $rpcenv->set_user($username);
>> -
>> -	    if ($method eq 'POST' && $rel_uri =~ m|^/nodes/([^/]+)/storage/([^/]+)/upload$|) {
>> -		my ($node, $storeid) = ($1, $2);
>> -		# we disable CSRF checks if $isUpload is set,
>> -		# to improve security we check user upload permission here
>> -		my $perm = { check => ['perm', "/storage/$storeid", ['Datastore.AllocateTemplate']] };
>> -		$rpcenv->check_api2_permissions($perm, $username, {});
>> -		$isUpload = 1;
>> -	    }
>> +	$rpcenv->set_user($username);
>>  
>> -	    # we skip CSRF check for file upload, because it is
>> -	    # difficult to pass CSRF HTTP headers with native html forms,
>> -	    # and it should not be necessary at all.
>> -	    my $euid = $>;
>> -	    PVE::AccessControl::verify_csrf_prevention_token($username, $token)
>> -		if !$isUpload && ($euid != 0) && ($method ne 'GET');
>> +	if ($method eq 'POST' && $rel_uri =~ m|^/nodes/([^/]+)/storage/([^/]+)/upload$|) {
>> +	    my ($node, $storeid) = ($1, $2);
>> +	    # we disable CSRF checks if $isUpload is set,
>> +	    # to improve security we check user upload permission here
>> +	    my $perm = { check => ['perm', "/storage/$storeid", ['Datastore.AllocateTemplate']] };
>> +	    $rpcenv->check_api2_permissions($perm, $username, {});
>> +	    $isUpload = 1;
>>  	}
>> +
>> +	# we skip CSRF check for file upload, because it is
>> +	# difficult to pass CSRF HTTP headers with native html forms,
>> +	# and it should not be necessary at all.
>> +	my $euid = $>;
>> +	PVE::AccessControl::verify_csrf_prevention_token($username, $token)
>> +	    if !$isUpload && ($euid != 0) && ($method ne 'GET');
> 
> this verify_csrf_prevention_token call was never triggered for API 
> tokens before, on purpose - one of the design goals of API tokens was to 
> provide stateless API access without requiring round-trips like our 
> regular, ticket-based sessions do. CSRF-prevention also does not make 
> much sense outside of the browser context (whereas API tokens are for 
> automated access by non-browser clients).
> 
> with this change we'd now suddenly require CSRF tokens for API tokens as 
> well, and IIRC API tokens can't even get one of those (and even if they 

> could, none of the existing clients would know about that and could no 
> longer do any non-GET requests with tokens).
> 
> I think (but I haven't tested it!) that
> - setting $isUpload for the API token case as well
> - leaving the CSRF check limited to regular (non-API-token) authentication
> 
> should give us the desired effect without any side-effects. the actual 
> upload handling is in PVE::APIServer::AnyEvent and just seems to check 
> for $isUpload in the result of auth_handler.
> 

you're right, good catch! @Lorenz, care to send out a patch bringing it
back it shape?