On Wed, Jul 3, 2019 at 1:54 PM Pavel Hrdina <phrdina@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
