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.stechau...@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
>
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
