1
0
Fork 0
mirror of https://github.com/ansible-collections/community.general.git synced 2024-09-14 20:13:21 +02:00

nmcli: writing secrets to command line is a security hole (#3160)

* nmcli: use `stdin` for setting private `wifi_sec` options

I.E.:
* `802-11-wireless-security.leap-password`
* `802-11-wireless-security.psk`
* `802-11-wireless-security.wep-key0`
* `802-11-wireless-security.wep-key1`
* `802-11-wireless-security.wep-key2`
* `802-11-wireless-security.wep-key3`

* Changelog fragement formatting.

* Update changelogs/fragments/3160-pass-wifi-secrets-via-stdin-to-nmcli-module.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

* Make `wifi_sec_secret_options()` into a constant

* Minor cleanup

`'set ' + key + ' ' + value`
=>
`'set %s %s' % (key, value)`

* Change `casing`

* Change `WIFI_SEC_SECRET_OPTIONS` from `list` to `tuple`

* Update `edit_connection()` to not reset `edit_commands`

It will just re`set` them if `edit_connection()` is called more than 
once.

* Do not call `edit_connection()` if `connection_update(*)` fails

* Fixed `pep8` issue `E713` in tests

`test for membership should be 'not in'`

* Simplify `create_connection()`/`modify_connection()` logic

* `WIFI_SEC_SECRET_OPTIONS`=>`SECRET_OPTIONS`, options are prefixed

* Moved `if key in self.SECRET_OPTIONS` into `if value is not None` check

We don't need to do anything is the value is None

Co-authored-by: Felix Fontein <felix@fontein.de>
This commit is contained in:
David Hummel 2021-08-08 09:35:52 -07:00 committed by GitHub
parent 2831bc45f5
commit 7f96b7df60
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 166 additions and 3 deletions

View file

@ -0,0 +1,4 @@
security_fixes:
- nmcli - do not pass WiFi secrets on the ``nmcli`` command line. Use ``nmcli con edit``
instead and pass secrets as ``stdin``
(https://github.com/ansible-collections/community.general/issues/3145).

View file

@ -709,6 +709,15 @@ class Nmcli(object):
platform = 'Generic' platform = 'Generic'
distribution = None distribution = None
SECRET_OPTIONS = (
'802-11-wireless-security.leap-password',
'802-11-wireless-security.psk',
'802-11-wireless-security.wep-key0',
'802-11-wireless-security.wep-key1',
'802-11-wireless-security.wep-key2',
'802-11-wireless-security.wep-key3'
)
def __init__(self, module): def __init__(self, module):
self.module = module self.module = module
self.state = module.params['state'] self.state = module.params['state']
@ -792,6 +801,8 @@ class Nmcli(object):
else: else:
self.ipv6_method = None self.ipv6_method = None
self.edit_commands = []
def execute_command(self, cmd, use_unsafe_shell=False, data=None): def execute_command(self, cmd, use_unsafe_shell=False, data=None):
if isinstance(cmd, list): if isinstance(cmd, list):
cmd = [to_text(item) for item in cmd] cmd = [to_text(item) for item in cmd]
@ -1079,12 +1090,17 @@ class Nmcli(object):
# Constructing the command. # Constructing the command.
for key, value in options.items(): for key, value in options.items():
if value is not None: if value is not None:
if key in self.SECRET_OPTIONS:
self.edit_commands += ['set %s %s' % (key, value)]
continue
cmd.extend([key, value]) cmd.extend([key, value])
return self.execute_command(cmd) return self.execute_command(cmd)
def create_connection(self): def create_connection(self):
status = self.connection_update('create') status = self.connection_update('create')
if status[0] == 0 and self.edit_commands:
status = self.edit_connection()
if self.create_connection_up: if self.create_connection_up:
status = self.up_connection() status = self.up_connection()
return status return status
@ -1105,7 +1121,15 @@ class Nmcli(object):
return self.execute_command(cmd) return self.execute_command(cmd)
def modify_connection(self): def modify_connection(self):
return self.connection_update('modify') status = self.connection_update('modify')
if status[0] == 0 and self.edit_commands:
status = self.edit_connection()
return status
def edit_connection(self):
data = "\n".join(self.edit_commands + ['save', 'quit'])
cmd = [self.nmcli_bin, 'con', 'edit', self.conn_name]
return self.execute_command(cmd, data=data)
def show_connection(self): def show_connection(self):
cmd = [self.nmcli_bin, '--show-secrets', 'con', 'show', self.conn_name] cmd = [self.nmcli_bin, '--show-secrets', 'con', 'show', self.conn_name]

View file

@ -697,6 +697,23 @@ def mocked_ethernet_connection_dhcp_to_static(mocker):
)) ))
@pytest.fixture
def mocked_secure_wireless_create_failure(mocker):
mocker_set(mocker,
execute_return=(1, "", ""))
@pytest.fixture
def mocked_secure_wireless_modify_failure(mocker):
mocker_set(mocker,
connection_exists=True,
execute_return=None,
execute_side_effect=(
(0, "", ""),
(1, "", ""),
))
@pytest.fixture @pytest.fixture
def mocked_dummy_connection_static_unchanged(mocker): def mocked_dummy_connection_static_unchanged(mocker):
mocker_set(mocker, mocker_set(mocker,
@ -1652,6 +1669,52 @@ def test_create_secure_wireless(mocked_generic_connection_create, capfd):
Test : Create secure wireless connection Test : Create secure wireless connection
""" """
with pytest.raises(SystemExit):
nmcli.main()
assert nmcli.Nmcli.execute_command.call_count == 2
arg_list = nmcli.Nmcli.execute_command.call_args_list
add_args, add_kw = arg_list[0]
assert add_args[0][0] == '/usr/bin/nmcli'
assert add_args[0][1] == 'con'
assert add_args[0][2] == 'add'
assert add_args[0][3] == 'type'
assert add_args[0][4] == 'wifi'
assert add_args[0][5] == 'con-name'
assert add_args[0][6] == 'non_existent_nw_device'
add_args_text = list(map(to_text, add_args[0]))
for param in ['connection.interface-name', 'wireless_non_existant',
'ipv4.addresses', '10.10.10.10/24',
'802-11-wireless.ssid', 'Brittany',
'802-11-wireless-security.key-mgmt', 'wpa-psk']:
assert param in add_args_text
edit_args, edit_kw = arg_list[1]
assert edit_args[0][0] == '/usr/bin/nmcli'
assert edit_args[0][1] == 'con'
assert edit_args[0][2] == 'edit'
assert edit_args[0][3] == 'non_existent_nw_device'
edit_kw_data = edit_kw['data'].split()
for param in ['802-11-wireless-security.psk', 'VERY_SECURE_PASSWORD',
'save',
'quit']:
assert param in edit_kw_data
out, err = capfd.readouterr()
results = json.loads(out)
assert not results.get('failed')
assert results['changed']
@pytest.mark.parametrize('patch_ansible_module', TESTCASE_SECURE_WIRELESS, indirect=['patch_ansible_module'])
def test_create_secure_wireless_failure(mocked_secure_wireless_create_failure, capfd):
"""
Test : Create secure wireless connection w/failure
"""
with pytest.raises(SystemExit): with pytest.raises(SystemExit):
nmcli.main() nmcli.main()
@ -1671,16 +1734,88 @@ def test_create_secure_wireless(mocked_generic_connection_create, capfd):
for param in ['connection.interface-name', 'wireless_non_existant', for param in ['connection.interface-name', 'wireless_non_existant',
'ipv4.addresses', '10.10.10.10/24', 'ipv4.addresses', '10.10.10.10/24',
'802-11-wireless.ssid', 'Brittany', '802-11-wireless.ssid', 'Brittany',
'802-11-wireless-security.key-mgmt', 'wpa-psk', '802-11-wireless-security.key-mgmt', 'wpa-psk']:
'802-11-wireless-security.psk', 'VERY_SECURE_PASSWORD']:
assert param in add_args_text assert param in add_args_text
out, err = capfd.readouterr()
results = json.loads(out)
assert results.get('failed')
assert 'changed' not in results
@pytest.mark.parametrize('patch_ansible_module', TESTCASE_SECURE_WIRELESS, indirect=['patch_ansible_module'])
def test_modify_secure_wireless(mocked_generic_connection_modify, capfd):
"""
Test : Modify secure wireless connection
"""
with pytest.raises(SystemExit):
nmcli.main()
assert nmcli.Nmcli.execute_command.call_count == 2
arg_list = nmcli.Nmcli.execute_command.call_args_list
add_args, add_kw = arg_list[0]
assert add_args[0][0] == '/usr/bin/nmcli'
assert add_args[0][1] == 'con'
assert add_args[0][2] == 'modify'
assert add_args[0][3] == 'non_existent_nw_device'
add_args_text = list(map(to_text, add_args[0]))
for param in ['connection.interface-name', 'wireless_non_existant',
'ipv4.addresses', '10.10.10.10/24',
'802-11-wireless.ssid', 'Brittany',
'802-11-wireless-security.key-mgmt', 'wpa-psk']:
assert param in add_args_text
edit_args, edit_kw = arg_list[1]
assert edit_args[0][0] == '/usr/bin/nmcli'
assert edit_args[0][1] == 'con'
assert edit_args[0][2] == 'edit'
assert edit_args[0][3] == 'non_existent_nw_device'
edit_kw_data = edit_kw['data'].split()
for param in ['802-11-wireless-security.psk', 'VERY_SECURE_PASSWORD',
'save',
'quit']:
assert param in edit_kw_data
out, err = capfd.readouterr() out, err = capfd.readouterr()
results = json.loads(out) results = json.loads(out)
assert not results.get('failed') assert not results.get('failed')
assert results['changed'] assert results['changed']
@pytest.mark.parametrize('patch_ansible_module', TESTCASE_SECURE_WIRELESS, indirect=['patch_ansible_module'])
def test_modify_secure_wireless_failure(mocked_secure_wireless_modify_failure, capfd):
"""
Test : Modify secure wireless connection w/failure
"""
with pytest.raises(SystemExit):
nmcli.main()
assert nmcli.Nmcli.execute_command.call_count == 2
arg_list = nmcli.Nmcli.execute_command.call_args_list
add_args, add_kw = arg_list[1]
assert add_args[0][0] == '/usr/bin/nmcli'
assert add_args[0][1] == 'con'
assert add_args[0][2] == 'modify'
assert add_args[0][3] == 'non_existent_nw_device'
add_args_text = list(map(to_text, add_args[0]))
for param in ['connection.interface-name', 'wireless_non_existant',
'ipv4.addresses', '10.10.10.10/24',
'802-11-wireless.ssid', 'Brittany',
'802-11-wireless-security.key-mgmt', 'wpa-psk']:
assert param in add_args_text
out, err = capfd.readouterr()
results = json.loads(out)
assert results.get('failed')
assert 'changed' not in results
@pytest.mark.parametrize('patch_ansible_module', TESTCASE_DUMMY_STATIC, indirect=['patch_ansible_module']) @pytest.mark.parametrize('patch_ansible_module', TESTCASE_DUMMY_STATIC, indirect=['patch_ansible_module'])
def test_create_dummy_static(mocked_generic_connection_create, capfd): def test_create_dummy_static(mocked_generic_connection_create, capfd):
""" """