On Tue, Jul 02, 2019 at 09:21:44PM +0200, Fabiano Fidêncio wrote:
> Let's not expose the user/root password in the CLI and, instead, let's
> rely on a file passed by the admin and read the password from there.
>
> 'CVE-2019-10183' has been assigned to the virt-install --unattended
> admin-password=xxx disclosure issue.
>
> Signed-off-by: Fabiano Fidêncio <fidencio@xxxxxxxxxx>
> ---
> man/virt-install.pod | 14 ++++++++++----
> tests/cli-test-xml/admin-password.txt | 1 +
> tests/cli-test-xml/user-password.txt | 3 +++
> tests/clitest.py | 18 +++++++++--------
> virtinst/cli.py | 4 ++--
> virtinst/install/unattended.py | 28 +++++++++++++++++----------
> 6 files changed, 44 insertions(+), 24 deletions(-)
> create mode 100644 tests/cli-test-xml/admin-password.txt
> create mode 100644 tests/cli-test-xml/user-password.txt
>
> diff --git a/man/virt-install.pod b/man/virt-install.pod
> index d8bd4127..0e655fef 100644
> --- a/man/virt-install.pod
> +++ b/man/virt-install.pod
> @@ -612,13 +612,19 @@ Choose which libosinfo unattended profile to use. Most distros have
> a 'desktop' and a 'jeos' profile. virt-install will default to 'desktop'
> if this is unspecified.
>
> -=item B<admin-password=>
> +=item B<admin-password-file=>
>
> -Set the VM OS admin/root password
> +A file used to set the VM OS admin/root password from. This option can
> +be used either as "admin-password-file=/path/to/passowrd-file" or as
> +"admin-password-file=/dev/fd/n", being n the file descriptor of the
> +password-file.
Typo fixed in my local branch.
> -=item B<user-password=>
> +=item B<user-password-file=>
>
> -Set the VM user password. The username is your current host username
> +A file used to set the VM user password. This option can be used either as
> +"user-password-file=/path/to/password-file" or as
> +"user-password-file=/dev/fd/n", being n the file descriptor of the
> +password-file. The username is your current host username.
>
> =item B<product-key=>
>
> diff --git a/tests/cli-test-xml/admin-password.txt b/tests/cli-test-xml/admin-password.txt
> new file mode 100644
> index 00000000..323fae03
> --- /dev/null
> +++ b/tests/cli-test-xml/admin-password.txt
> @@ -0,0 +1 @@
> +foobar
> diff --git a/tests/cli-test-xml/user-password.txt b/tests/cli-test-xml/user-password.txt
> new file mode 100644
> index 00000000..625999ba
> --- /dev/null
> +++ b/tests/cli-test-xml/user-password.txt
> @@ -0,0 +1,3 @@
> +blah
> +
> +
Are these random white spaces intentional? There are two lines and
both with different number of white spaces. If not I can fix it before
pushing.
[...]
> diff --git a/virtinst/install/unattended.py b/virtinst/install/unattended.py
> index ea3f9066..4f311296 100644
> --- a/virtinst/install/unattended.py
> +++ b/virtinst/install/unattended.py
> @@ -39,23 +39,21 @@ def _make_installconfig(script, osobj, unattended_data, arch, hostname, url):
>
> # Set user-password.
> # In case it's required and not passed, just raise a RuntimeError.
> - if script.requires_user_password() and not unattended_data.user_password:
> + if (script.requires_user_password() and
> + not unattended_data.get_user_password()):
> raise RuntimeError(
> _("%s requires the user-password to be set.") %
> osobj.name)
> - config.set_user_password(
> - unattended_data.user_password if unattended_data.user_password
> - else "")
> + config.set_user_password(unattended_data.get_user_password() or "")
>
> # Set the admin-password:
> # In case it's required and not passed, just raise a RuntimeError.
> - if script.requires_admin_password() and not unattended_data.admin_password:
> + if (script.requires_admin_password() and
> + not unattended_data.get_admin_password()):
> raise RuntimeError(
> _("%s requires the admin-password to be set.") %
> osobj.name)
> - config.set_admin_password(
> - unattended_data.admin_password if unattended_data.admin_password
> - else "")
> + config.set_admin_password(unattended_data.get_admin_password() or "")
>
> # Set the target disk.
> # virtiodisk is the preferred way, in case it's supported, otherwise
> @@ -205,10 +203,20 @@ class OSInstallScript:
>
> class UnattendedData():
> profile = None
> - admin_password = None
> - user_password = None
> + admin_password_file = None
> + user_password_file = None
> product_key = None
>
> + def get_user_password(self):
> + if self.user_password_file:
> + with open(self.user_password_file) as pwd:
> + return pwd.read().rstrip("\n\r")
> +
> + def get_admin_password(self):
> + if self.admin_password_file:
> + with open(self.admin_password_file) as pwd:
> + return pwd.read().rstrip("\n\r")
> +
I guess this is related to my question above, we will strip trailing
lines, everything else is considered as a valid password including
other white space characters?
Pavel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
