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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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;
>>  }
> 
>