[virt-tools-list] [virt-manager PATCH 2/2] unattended: Don't log user & admin passwords

[Fabiano Fidêncio]

On Wed, Jul 3, 2019 at 1:54 PM Pavel Hrdina <phrdina at redhat.com> wrote:
>
> On Tue, Jul 02, 2019 at 09:21:45PM +0200, Fabiano Fidêncio wrote:
> > Logging user & admin passwords in the command-line is a security issue,
> > let's avoid doing so by:
> > - Not printing the values set by the user when setting up the
> > install-script config file;
> > - Removing the values used in the install-scripts, when printing their
> > content;
> >
> > 'CVE-2019-10183' has been assigned to the virt-install --unattended
> > admin-password=xxx disclosure issue.
> >
> > Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>
> > ---
> >  virtinst/install/unattended.py | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/virtinst/install/unattended.py b/virtinst/install/unattended.py
> > index 4f311296..04758563 100644
> > --- a/virtinst/install/unattended.py
> > +++ b/virtinst/install/unattended.py
> > @@ -97,8 +97,6 @@ def _make_installconfig(script, osobj, unattended_data, arch, hostname, url):
> >      log.debug("InstallScriptConfig created with the following params:")
> >      log.debug("username: %s", config.get_user_login())
> >      log.debug("realname: %s", config.get_user_realname())
> > -    log.debug("user password: %s", config.get_user_password())
> > -    log.debug("admin password: %s", config.get_admin_password())
> >      log.debug("target disk: %s", config.get_target_disk())
> >      log.debug("hardware arch: %s", config.get_hardware_arch())
> >      log.debug("hostname: %s", config.get_hostname())
> > @@ -195,6 +193,14 @@ class OSInstallScript:
> >          content = self.generate()
> >          open(scriptpath, "w").write(content)
> >
> > +        user_password = self._config.get_user_password()
> > +        if user_password:
> > +            content = content.replace(user_password, "[SCRUBBED]")
> > +
> > +        admin_password = self._config.get_admin_password()
> > +        if admin_password:
> > +            content = content.replace(admin_password, "[SCRUBBED]")
>
> There is a small issue with this approach, if you for testing purposes
> or for any other reason use password that matches anything else in the
> script file (kickstart for example) it will be replaced as well:
>
> """"""
> %post --erroronfail
>
> useradd -G wheel phrdina # Add user
> if [SCRUBBED] -z ''; then
>     passwd -d phrdina # Make user account passwordless
> else
>     echo '' |passwd --stdin phrdina
> fi
>
> if [SCRUBBED] -z '[SCRUBBED]'; then
>     passwd -d root # Make root account passwordless
> else
>     echo '[SCRUBBED]' |passwd --stdin root
> fi
> """""
>
> Here I used as a password 'test' and you can see the test command was
> replaced as well.  Probably not a big deal is it modifies only the log
> output, not the actual file but I thought that I should at least point
> to this corner case issue.  Do we care about it or not?

I thought about that and I sincerely don't care much about that.
Otherwise we'll have to learn exactly what to match in all the install
scripts supported (as in, kickstarts, autoyast, jeos, ...).

>
> Pavel