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

[Daniel P. Berrangé]

On Wed, Jul 03, 2019 at 02:42:30PM +0200, Fabiano Fidêncio wrote:
> 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, ...).

You can avoid this kind of matching at all but just using a different
Libosinfo.InstallConfig. eg just copy the master config and set the
password params to a sanitized  value & generate a new script.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|