From 7f96b7df60bad4bd85c787401d521717c842da23 Mon Sep 17 00:00:00 2001 From: David Hummel <6109326+hummeltech@users.noreply.github.com> Date: Sun, 8 Aug 2021 09:35:52 -0700 Subject: [PATCH] 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 * 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 --- ...wifi-secrets-via-stdin-to-nmcli-module.yml | 4 + plugins/modules/net_tools/nmcli.py | 26 +++- .../plugins/modules/net_tools/test_nmcli.py | 139 +++++++++++++++++- 3 files changed, 166 insertions(+), 3 deletions(-) create mode 100644 changelogs/fragments/3160-pass-wifi-secrets-via-stdin-to-nmcli-module.yml diff --git a/changelogs/fragments/3160-pass-wifi-secrets-via-stdin-to-nmcli-module.yml b/changelogs/fragments/3160-pass-wifi-secrets-via-stdin-to-nmcli-module.yml new file mode 100644 index 0000000000..47e1837a0b --- /dev/null +++ b/changelogs/fragments/3160-pass-wifi-secrets-via-stdin-to-nmcli-module.yml @@ -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). diff --git a/plugins/modules/net_tools/nmcli.py b/plugins/modules/net_tools/nmcli.py index 92d1e65ef7..06b868dace 100644 --- a/plugins/modules/net_tools/nmcli.py +++ b/plugins/modules/net_tools/nmcli.py @@ -709,6 +709,15 @@ class Nmcli(object): platform = 'Generic' 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): self.module = module self.state = module.params['state'] @@ -792,6 +801,8 @@ class Nmcli(object): else: self.ipv6_method = None + self.edit_commands = [] + def execute_command(self, cmd, use_unsafe_shell=False, data=None): if isinstance(cmd, list): cmd = [to_text(item) for item in cmd] @@ -1079,12 +1090,17 @@ class Nmcli(object): # Constructing the command. for key, value in options.items(): if value is not None: + if key in self.SECRET_OPTIONS: + self.edit_commands += ['set %s %s' % (key, value)] + continue cmd.extend([key, value]) return self.execute_command(cmd) def create_connection(self): status = self.connection_update('create') + if status[0] == 0 and self.edit_commands: + status = self.edit_connection() if self.create_connection_up: status = self.up_connection() return status @@ -1105,7 +1121,15 @@ class Nmcli(object): return self.execute_command(cmd) 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): cmd = [self.nmcli_bin, '--show-secrets', 'con', 'show', self.conn_name] diff --git a/tests/unit/plugins/modules/net_tools/test_nmcli.py b/tests/unit/plugins/modules/net_tools/test_nmcli.py index c1b3e93ed4..9f131c3873 100644 --- a/tests/unit/plugins/modules/net_tools/test_nmcli.py +++ b/tests/unit/plugins/modules/net_tools/test_nmcli.py @@ -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 def mocked_dummy_connection_static_unchanged(mocker): mocker_set(mocker, @@ -1652,6 +1669,52 @@ def test_create_secure_wireless(mocked_generic_connection_create, capfd): 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): 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', 'ipv4.addresses', '10.10.10.10/24', '802-11-wireless.ssid', 'Brittany', - '802-11-wireless-security.key-mgmt', 'wpa-psk', - '802-11-wireless-security.psk', 'VERY_SECURE_PASSWORD']: + '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_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() 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_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']) def test_create_dummy_static(mocked_generic_connection_create, capfd): """