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

[Cole Robinson]

On 7/3/19 8:42 AM, 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, ...).

I'm not much worried about it either. But presumably we could overwrite
the admin/user pass in the config object with [SCRUBBED], generate the
output, log it, fix the config object and generate the real output and
save. But it's not a requirement IMO

- Cole