perm check: forbid undefined/empty ACL path

to detect similar issues to that fixed in the previous commit early on
and without the potential for dangerous side-effects.

root@pam is intentionally still allowed before the check in case such
situations can be triggered by misconfiguration - root@pam can then
still clean up the affected configs via the GUI/API, and not just via
manual editing.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
index 5d78c6fa0aac92b70c044cfd82b8b3346022da29..104e7e3036c7b0889c7038ba3101dc7c27614307 100644 (file)
--- a/src/PVE/API2/AccessControl.pm
+++ b/src/PVE/API2/AccessControl.pm
@@ -115,6 +115,7 @@ my sub verify_auth : prototype($$$$$$$) {
     my ($rpcenv, $username, $pw_or_ticket, $otp, $path, $privs, $new_format) = @_;
 
     my $normpath = PVE::AccessControl::normalize_path($path);
+    die "invalid path - $path\n" if defined($path) && !defined($normpath);
 
     my $ticketuser;
     if (($ticketuser = PVE::AccessControl::verify_ticket($pw_or_ticket, 1)) &&

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index a2e0458c30cbb6ad41e72e18208e0b2d1c29569c..3725a7d329564bfa79364604911a65dccc44b95c 100644 (file)
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -498,6 +498,8 @@ my $assemble_short_lived_ticket = sub {
 
     $path = normalize_path($path);
 
+    die "invalid ticket path\n" if !defined($path);
+
     my $secret_data = "$username:$path";
 
     return PVE::Ticket::assemble_rsa_ticket(
@@ -509,6 +511,8 @@ my $verify_short_lived_ticket = sub {
 
     $path = normalize_path($path);
 
+    die "invalid ticket path\n" if !defined($path);
+
     my $secret_data = "$username:$path";
 
     my ($rsa_pub, $rsa_mtime) = get_pubkey();
@@ -1166,6 +1170,8 @@ sub lookup_username {
 sub normalize_path {
     my $path = shift;
 
+    return undef if !$path;
+
     $path =~ s|/+|/|g;
 
     $path =~ s|/$||;
@@ -1641,6 +1647,12 @@ sub roles {
 
     return 'Administrator' if $user eq 'root@pam'; # root can do anything
 
+    if (!defined($path)) {
+       # this shouldn't happen!
+       warn "internal error: ACL check called for undefined ACL path!\n";
+       return {};
+    }
+
     if (pve_verify_tokenid($user, 1)) {
        my $tokenid = $user;
        my ($username, $token) = split_tokenid($tokenid);

diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
index e03974a4afb6a89abf328bd2f027d8f385018da8..0ee23460bd4ed153e271a32f4cbc401e087647fb 100644 (file)
--- a/src/PVE/RPCEnvironment.pm
+++ b/src/PVE/RPCEnvironment.pm
@@ -140,6 +140,12 @@ sub permissions {
        return { map { $_ => 1 } keys %{$cfg->{roles}->{'Administrator'}} };
     }
 
+    if (!defined($path)) {
+       # this shouldn't happen!
+       warn "internal error: ACL check called for undefined ACL path!\n";
+       return {};
+    }
+
     if (PVE::AccessControl::pve_verify_tokenid($user, 1)) {
        my ($username, $token) = PVE::AccessControl::split_tokenid($user);
        my $cfg = $self->{user_cfg};
@@ -439,8 +445,10 @@ sub exec_api2_perm_check {
            raise_perm_exc();
        }
        my $path = PVE::Tools::template_replace($tmplpath, $param);
-       $path = PVE::AccessControl::normalize_path($path);
-       return $self->check_full($username, $path, $privs, $any, $noerr);
+       my $normpath = PVE::AccessControl::normalize_path($path);
+       warn "Failed to normalize '$path'\n" if !defined($normpath) && defined($path);
+
+       return $self->check_full($username, $normpath, $privs, $any, $noerr);
     } elsif ($test eq 'userid-group') {
        my $userid = $param->{userid};
        my ($t, $privs, %options) = @$check;
@@ -490,6 +498,7 @@ sub exec_api2_perm_check {
        my ($t, $tmplpath) = @$check;
        my $path = PVE::Tools::template_replace($tmplpath, $param);
        $path = PVE::AccessControl::normalize_path($path);
+       return 0 if !defined($path); # should already die in API2::ACL
        return $self->check_perm_modify($username, $path, $noerr);
     } else {
        die "unknown permission test";