* [pve-devel] [PATCH manager] fix #3389: Skip CSRF token check also for API tokens on file upload
@ 2021-04-21 11:59 Lorenz Stechauner
2021-04-22 8:03 ` [pve-devel] applied: " Thomas Lamprecht
2021-04-22 8:05 ` [pve-devel] " Fabian Grünbichler
0 siblings, 2 replies; 4+ messages in thread
From: Lorenz Stechauner @ 2021-04-21 11:59 UTC (permalink / raw)
To: pve-devel
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');
}
return {
--
2.20.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* [pve-devel] applied: [PATCH manager] fix #3389: Skip CSRF token check also for API tokens on file upload
2021-04-21 11:59 [pve-devel] [PATCH manager] fix #3389: Skip CSRF token check also for API tokens on file upload Lorenz Stechauner
@ 2021-04-22 8:03 ` Thomas Lamprecht
2021-04-22 8:05 ` [pve-devel] " Fabian Grünbichler
1 sibling, 0 replies; 4+ messages in thread
From: Thomas Lamprecht @ 2021-04-22 8:03 UTC (permalink / raw)
To: Proxmox VE development discussion, Lorenz Stechauner
On 21.04.21 13:59, 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(-)
>
>
applied, thanks! `git show -w` really helped reviewing that one ;-)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH manager] fix #3389: Skip CSRF token check also for API tokens on file upload
2021-04-21 11:59 [pve-devel] [PATCH manager] fix #3389: Skip CSRF token check also for API tokens on file upload Lorenz Stechauner
2021-04-22 8:03 ` [pve-devel] applied: " Thomas Lamprecht
@ 2021-04-22 8:05 ` Fabian Grünbichler
2021-04-22 8:08 ` Thomas Lamprecht
1 sibling, 1 reply; 4+ messages in thread
From: Fabian Grünbichler @ 2021-04-22 8:05 UTC (permalink / raw)
To: Proxmox VE development discussion
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
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH manager] fix #3389: Skip CSRF token check also for API tokens on file upload
2021-04-22 8:05 ` [pve-devel] " Fabian Grünbichler
@ 2021-04-22 8:08 ` Thomas Lamprecht
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Lamprecht @ 2021-04-22 8:08 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Grünbichler
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?
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-22 8:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-21 11:59 [pve-devel] [PATCH manager] fix #3389: Skip CSRF token check also for API tokens on file upload Lorenz Stechauner
2021-04-22 8:03 ` [pve-devel] applied: " Thomas Lamprecht
2021-04-22 8:05 ` [pve-devel] " Fabian Grünbichler
2021-04-22 8:08 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox