On Wed, Jul 3, 2019 at 10:53 AM Pavel Hrdina <phrdina@xxxxxxxxxx> wrote:
>
> 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.
Those are intentional ...
>
> [...]
>
> > 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?
We'll strip trailing lines, everything else can be considered a valid
password, even if it includes whitespaces in the end. :-)
>
> Pavel
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
