[pve-devel] [PATCH v2 container] fix #4192: revamp check for systemd version

[pve-devel] [PATCH v2 container] fix #4192: revamp check for systemd version

[Leo Nunner]

Instead of iterating through several folders, it might just be easier to
check the ldd output of /sbin/init and getting the version from there.
Furthermore, the regex for checking the version has been adapted so that
it's more precise.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
This solution does actually feel cleaner than manually checking all the folders
every time.

 src/PVE/LXC/Setup/Base.pm | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
index cc12914..44b88d9 100644
--- a/src/PVE/LXC/Setup/Base.pm
+++ b/src/PVE/LXC/Setup/Base.pm
@@ -514,19 +514,26 @@ sub clear_machine_id {
     }
 }
 
-# tries to guess the systemd (major) version based on the existence of
-# (/usr)?/lib/systemd/libsystemd-shared<version>.so. It was introduced in v231.
+# tries to guess the systemd (major) version based on the
+# libsystemd-shared<version>.so linked with /sbin/init
 sub get_systemd_version {
     my ($self) = @_;
 
-    my $sd_lib_dir = $self->ct_is_directory("/lib/systemd") ?
-	"/lib/systemd" : "/usr/lib/systemd";
-    my $libsd = PVE::Tools::dir_glob_regex($sd_lib_dir, "libsystemd-shared-.+\.so");
-    if (defined($libsd) && $libsd =~ /libsystemd-shared-(\d+)(?:\..*)?\.so/) {
-	return $1;
-    }
-
-    return undef;
+    my $version = undef;
+    PVE::Tools::run_command(
+	[
+	    'ldd',
+	    '/sbin/init'
+	],
+	outfunc => sub {
+	    my $line = shift;
+	    if ($line =~ /^\s*libsystemd-shared-(\d+)(?:\.[a-zA-Z0-9]*)?\.so/) {
+		$version = $1;
+	    }},
+	errmsg => "ldd on /sbin/init failed"
+    );
+
+    return $version;
 }
 
 sub unified_cgroupv2_support {
-- 
2.30.2

Re: [pve-devel] [PATCH v2 container] fix #4192: revamp check for systemd version

[Fabian Grünbichler]

On September 12, 2022 2:25 pm, Leo Nunner wrote:
> Instead of iterating through several folders, it might just be easier to
> check the ldd output of /sbin/init and getting the version from there.
> Furthermore, the regex for checking the version has been adapted so that
> it's more precise.

ldd is not suited for this purpose for security reasons, since /sbin/init 
is a user/attacker-controlled binary in this case and we are only in a 
chroot while doing the setup, not really containerized. given a crafted 
container template/backup archive/.. this could execute arbitrary code.

it's manpage suggests using

 objdump -p /path/to/binary

and looking at the lines with "NEEDED", which seems to me should be fine 
for what we want to achieve here :)

> 
> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
> This solution does actually feel cleaner than manually checking all the folders
> every time.
> 
>  src/PVE/LXC/Setup/Base.pm | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
> index cc12914..44b88d9 100644
> --- a/src/PVE/LXC/Setup/Base.pm
> +++ b/src/PVE/LXC/Setup/Base.pm
> @@ -514,19 +514,26 @@ sub clear_machine_id {
>      }
>  }
>  
> -# tries to guess the systemd (major) version based on the existence of
> -# (/usr)?/lib/systemd/libsystemd-shared<version>.so. It was introduced in v231.
> +# tries to guess the systemd (major) version based on the
> +# libsystemd-shared<version>.so linked with /sbin/init
>  sub get_systemd_version {
>      my ($self) = @_;
>  
> -    my $sd_lib_dir = $self->ct_is_directory("/lib/systemd") ?
> -	"/lib/systemd" : "/usr/lib/systemd";
> -    my $libsd = PVE::Tools::dir_glob_regex($sd_lib_dir, "libsystemd-shared-.+\.so");
> -    if (defined($libsd) && $libsd =~ /libsystemd-shared-(\d+)(?:\..*)?\.so/) {
> -	return $1;
> -    }
> -
> -    return undef;
> +    my $version = undef;
> +    PVE::Tools::run_command(
> +	[
> +	    'ldd',
> +	    '/sbin/init'
> +	],
> +	outfunc => sub {
> +	    my $line = shift;
> +	    if ($line =~ /^\s*libsystemd-shared-(\d+)(?:\.[a-zA-Z0-9]*)?\.so/) {
> +		$version = $1;
> +	    }},
> +	errmsg => "ldd on /sbin/init failed"
> +    );
> +
> +    return $version;
>  }
>  
>  sub unified_cgroupv2_support {
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v2 container] fix #4192: revamp check for systemd version

[Thomas Lamprecht]

Am 12/09/2022 um 14:41 schrieb Fabian Grünbichler:
>> Instead of iterating through several folders, it might just be easier to
>> check the ldd output of /sbin/init and getting the version from there.
>> Furthermore, the regex for checking the version has been adapted so that
>> it's more precise.
> ldd is not suited for this purpose for security reasons, since /sbin/init 
> is a user/attacker-controlled binary in this case and we are only in a 
> chroot while doing the setup, not really containerized. given a crafted 
> container template/backup archive/.. this could execute arbitrary code.
> 
> it's manpage suggests using
> 
>  objdump -p /path/to/binary
> 
> and looking at the lines with "NEEDED", which seems to me should be fine 
> for what we want to achieve here 😄
> 

tbf, I suggested using something like ldd here, but I mostly meant if we actually
need to further extend this than simply checking three instead of two paths.

But I actually like the much shorter code, so from that POV it could be a nicer
option, but it makes us dependent on actually executing code from the CT archive,
which also assumes the availability of something like ldd or objdump, which may
not be the case in all templates?

Two small nits w.r.t. to the v2 still inline.

Am 12/09/2022 um 14:25 schrieb Leo Nunner:
> +    my $version = undef;
> +    PVE::Tools::run_command(
> +	[
> +	    'ldd',
> +	    '/sbin/init'
> +	],

you can put the array ref in one line, e.g., with Fabian relayed manpage
suggestion it'd be fine to do:

    [ 'objdump', '-p', '/sbin/init' ],

> +	outfunc => sub {
> +	    my $line = shift;
> +	    if ($line =~ /^\s*libsystemd-shared-(\d+)(?:\.[a-zA-Z0-9]*)?\.so/) {
> +		$version = $1;
> +	    }},
> +	errmsg => "ldd on /sbin/init failed"

above is missing a trailing comma, which would ensure that any possible addition of an
option in the future won't need to touch an unrelated line.

> +    );
> +
> +    return $version;
>  }

Re: [pve-devel] [PATCH v2 container] fix #4192: revamp check for systemd version

[Fabian Grünbichler]

On September 12, 2022 4:01 pm, Thomas Lamprecht wrote:
> Am 12/09/2022 um 14:41 schrieb Fabian Grünbichler:
>>> Instead of iterating through several folders, it might just be easier to
>>> check the ldd output of /sbin/init and getting the version from there.
>>> Furthermore, the regex for checking the version has been adapted so that
>>> it's more precise.
>> ldd is not suited for this purpose for security reasons, since /sbin/init 
>> is a user/attacker-controlled binary in this case and we are only in a 
>> chroot while doing the setup, not really containerized. given a crafted 
>> container template/backup archive/.. this could execute arbitrary code.
>> 
>> it's manpage suggests using
>> 
>>  objdump -p /path/to/binary
>> 
>> and looking at the lines with "NEEDED", which seems to me should be fine 
>> for what we want to achieve here 😄
>> 
> 
> tbf, I suggested using something like ldd here, but I mostly meant if we actually
> need to further extend this than simply checking three instead of two paths.
> 
> But I actually like the much shorter code, so from that POV it could be a nicer
> option, but it makes us dependent on actually executing code from the CT archive,
> which also assumes the availability of something like ldd or objdump, which may
> not be the case in all templates?

yeah, objdump from the host would need to be used (else we'd be back to 
square one and execute code from the container template/backup 
archive/.. without adequate protection). probably needs additional 
safeguards like resolving /sbin/init from the system with checks to 
prevent escape from the container /.

> Two small nits w.r.t. to the v2 still inline.
> 
> Am 12/09/2022 um 14:25 schrieb Leo Nunner:
>> +    my $version = undef;
>> +    PVE::Tools::run_command(
>> +	[
>> +	    'ldd',
>> +	    '/sbin/init'
>> +	],
> 
> you can put the array ref in one line, e.g., with Fabian relayed manpage
> suggestion it'd be fine to do:
> 
>     [ 'objdump', '-p', '/sbin/init' ],

see above ;) the whole thing would need to be revamped to no longer 
happen in "protected_call" context, but instead in host context without 
chroot, except for the symlink resolve part. at that point it might not 
be nicer anymoe than the original approach of checking a few paths, 
although it would have the benefit of being much more future-proof 
w.r.t. distros/upstream/.. deciding on new funny places to put that 
library ;)

>> +	outfunc => sub {
>> +	    my $line = shift;
>> +	    if ($line =~ /^\s*libsystemd-shared-(\d+)(?:\.[a-zA-Z0-9]*)?\.so/) {
>> +		$version = $1;
>> +	    }},
>> +	errmsg => "ldd on /sbin/init failed"
> 
> above is missing a trailing comma, which would ensure that any possible addition of an
> option in the future won't need to touch an unrelated line.
> 
>> +    );
>> +
>> +    return $version;
>>  }
> 
>