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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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.

>      }
>  
>      return {
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>