From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D863875908 for ; Thu, 22 Apr 2021 10:05:58 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id CEB08189CF for ; Thu, 22 Apr 2021 10:05:58 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id E0C6E189C5 for ; Thu, 22 Apr 2021 10:05:57 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id ACB734200F for ; Thu, 22 Apr 2021 10:05:57 +0200 (CEST) Date: Thu, 22 Apr 2021 10:05:49 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20210421115943.100078-1-l.stechauner@proxmox.com> In-Reply-To: <20210421115943.100078-1-l.stechauner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1619074571.4va2rkfmko.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.080 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [httpserver.pm, proxmox.com] Subject: Re: [pve-devel] [PATCH manager] fix #3389: Skip CSRF token check also for API tokens on file upload X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Apr 2021 08:05:58 -0000 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. >=20 > Signed-off-by: Lorenz Stechauner > --- > PVE/HTTPServer.pm | 34 +++++++++++++++++----------------- > 1 file changed, 17 insertions(+), 17 deletions(-) >=20 > 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 { > =20 > if ($require_auth) { > if ($api_token) { > + # returns tokenid actually > $username =3D PVE::AccessControl::verify_token($api_token); > - $rpcenv->set_user($username); #actually tokenid in this case > } else { > die "No ticket\n" if !$ticket; > =20 > @@ -94,25 +94,25 @@ sub auth_handler { > die "No ticket\n" > if ($rel_uri ne '/access/tfa' || $method ne 'POST'); > } > + } > =20 > - $rpcenv->set_user($username); > - > - if ($method eq 'POST' && $rel_uri =3D~ m|^/nodes/([^/]+)/storage/([= ^/]+)/upload$|) { > - my ($node, $storeid) =3D ($1, $2); > - # we disable CSRF checks if $isUpload is set, > - # to improve security we check user upload permission here > - my $perm =3D { check =3D> ['perm', "/storage/$storeid", ['Datastore.Al= locateTemplate']] }; > - $rpcenv->check_api2_permissions($perm, $username, {}); > - $isUpload =3D 1; > - } > + $rpcenv->set_user($username); > =20 > - # 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 =3D $>; > - PVE::AccessControl::verify_csrf_prevention_token($username, $token) > - if !$isUpload && ($euid !=3D 0) && ($method ne 'GET'); > + if ($method eq 'POST' && $rel_uri =3D~ m|^/nodes/([^/]+)/storage/([^/]+= )/upload$|) { > + my ($node, $storeid) =3D ($1, $2); > + # we disable CSRF checks if $isUpload is set, > + # to improve security we check user upload permission here > + my $perm =3D { check =3D> ['perm', "/storage/$storeid", ['Datastore= .AllocateTemplate']] }; > + $rpcenv->check_api2_permissions($perm, $username, {}); > + $isUpload =3D 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 =3D $>; > + PVE::AccessControl::verify_csrf_prevention_token($username, $token) > + if !$isUpload && ($euid !=3D 0) && ($method ne 'GET'); this verify_csrf_prevention_token call was never triggered for API=20 tokens before, on purpose - one of the design goals of API tokens was to=20 provide stateless API access without requiring round-trips like our=20 regular, ticket-based sessions do. CSRF-prevention also does not make=20 much sense outside of the browser context (whereas API tokens are for=20 automated access by non-browser clients). with this change we'd now suddenly require CSRF tokens for API tokens as=20 well, and IIRC API tokens can't even get one of those (and even if they=20 could, none of the existing clients would know about that and could no=20 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=20 upload handling is in PVE::APIServer::AnyEvent and just seems to check=20 for $isUpload in the result of auth_handler. > } > =20 > return { > --=20 > 2.20.1 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20 =