On 7/3/19 8:42 AM, Fabiano Fidêncio wrote:
> 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, ...).
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
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
