From 0d746b4198abf84290a093b83cf02b4203d73d9f Mon Sep 17 00:00:00 2001 From: Matt Davis Date: Mon, 26 Nov 2018 15:33:48 -0800 Subject: [PATCH] split PS wrapper and payload (CVE-2018-16859) (#49145) * prevent scriptblock logging from logging payload contents * added tests to verify no payload contents in PS Operational event log * fix script action to send split-aware wrapper * fix CLIXML error parser (return to -EncodedCommand exposed problems with it) * addresses CVE-2018-16859 --- changelogs/fragments/ps_sb_logging.yaml | 2 ++ lib/ansible/executor/module_common.py | 3 ++- lib/ansible/executor/powershell/__init__.py | 0 .../executor/powershell/bootstrap_wrapper.ps1 | 7 +++++++ lib/ansible/plugins/action/script.py | 13 +++++++------ lib/ansible/plugins/connection/winrm.py | 14 +++++++++----- lib/ansible/plugins/shell/powershell.py | 16 ++++++++++++---- .../targets/win_become/tasks/main.yml | 19 ++++++++++++++++++- 8 files changed, 57 insertions(+), 17 deletions(-) create mode 100644 changelogs/fragments/ps_sb_logging.yaml create mode 100644 lib/ansible/executor/powershell/__init__.py create mode 100644 lib/ansible/executor/powershell/bootstrap_wrapper.ps1 diff --git a/changelogs/fragments/ps_sb_logging.yaml b/changelogs/fragments/ps_sb_logging.yaml new file mode 100644 index 00000000000..78241df4494 --- /dev/null +++ b/changelogs/fragments/ps_sb_logging.yaml @@ -0,0 +1,2 @@ +bugfixes: +- Windows - prevent sensitive content from appearing in scriptblock logging (CVE 2018-16859) diff --git a/lib/ansible/executor/module_common.py b/lib/ansible/executor/module_common.py index 009eb06c483..2260097334c 100644 --- a/lib/ansible/executor/module_common.py +++ b/lib/ansible/executor/module_common.py @@ -850,7 +850,8 @@ def _find_module_utils(module_name, b_module_data, module_path, module_args, tas # FUTURE: smuggle this back as a dict instead of serializing here; the connection plugin may need to modify it module_json = json.dumps(exec_manifest) - b_module_data = exec_wrapper.replace(b"$json_raw = ''", b"$json_raw = @'\r\n%s\r\n'@" % to_bytes(module_json)) + # delimit the payload JSON from the wrapper to keep sensitive contents out of scriptblocks (which can be logged) + b_module_data = to_bytes(exec_wrapper) + b'\0\0\0\0' + to_bytes(module_json) elif module_substyle == 'jsonargs': module_args_json = to_bytes(json.dumps(module_args)) diff --git a/lib/ansible/executor/powershell/__init__.py b/lib/ansible/executor/powershell/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/lib/ansible/executor/powershell/bootstrap_wrapper.ps1 b/lib/ansible/executor/powershell/bootstrap_wrapper.ps1 new file mode 100644 index 00000000000..cfad82703db --- /dev/null +++ b/lib/ansible/executor/powershell/bootstrap_wrapper.ps1 @@ -0,0 +1,7 @@ +&chcp.com 65001 > $null +$exec_wrapper_str = $input | Out-String +$split_parts = $exec_wrapper_str.Split(@("`0`0`0`0"), 2, [StringSplitOptions]::RemoveEmptyEntries) +If (-not $split_parts.Length -eq 2) { throw "invalid payload" } +Set-Variable -Name json_raw -Value $split_parts[1] +$exec_wrapper = [ScriptBlock]::Create($split_parts[0]) +&$exec_wrapper diff --git a/lib/ansible/plugins/action/script.py b/lib/ansible/plugins/action/script.py index 000b2951f12..be8cc0fa960 100644 --- a/lib/ansible/plugins/action/script.py +++ b/lib/ansible/plugins/action/script.py @@ -119,12 +119,13 @@ class ActionModule(ActionBase): exec_data = None # WinRM requires a special wrapper to work with environment variables if self._connection.transport == "winrm": - pay = self._connection._create_raw_wrapper_payload(script_cmd, - env_dict) - exec_data = exec_wrapper.replace(b"$json_raw = ''", - b"$json_raw = @'\r\n%s\r\n'@" - % to_bytes(pay)) - script_cmd = "-" + pay = self._connection._create_raw_wrapper_payload(script_cmd, env_dict) + exec_data = to_bytes(exec_wrapper) + b"\0\0\0\0" + to_bytes(pay) + + # build the necessary exec wrapper command + # FUTURE: this still doesn't let script work on Windows with non-pipelined connections or + # full manual exec of KEEP_REMOTE_FILES + script_cmd = self._connection._shell.build_module_command(env_string='', shebang='#!powershell', cmd='') result.update(self._low_level_execute_command(cmd=script_cmd, in_data=exec_data, sudoable=True, chdir=chdir)) diff --git a/lib/ansible/plugins/connection/winrm.py b/lib/ansible/plugins/connection/winrm.py index 28aa5a62d79..b5e8dd7c593 100644 --- a/lib/ansible/plugins/connection/winrm.py +++ b/lib/ansible/plugins/connection/winrm.py @@ -104,6 +104,7 @@ import traceback import json import tempfile import subprocess +import xml.etree.ElementTree as ET HAVE_KERBEROS = False try: @@ -537,14 +538,17 @@ class Connection(ConnectionBase): return (result.status_code, result.std_out, result.std_err) def is_clixml(self, value): - return value.startswith(b"#< CLIXML") + return value.startswith(b"#< CLIXML\r\n") # hacky way to get just stdout- not always sure of doc framing here, so use with care def parse_clixml_stream(self, clixml_doc, stream_name='Error'): - clear_xml = clixml_doc.replace(b'#< CLIXML\r\n', b'') - doc = xmltodict.parse(clear_xml) - lines = [l.get('#text', '').replace('_x000D__x000A_', '') for l in doc.get('Objs', {}).get('S', {}) if l.get('@S') == stream_name] - return '\r\n'.join(lines) + clixml = ET.fromstring(clixml_doc.split(b"\r\n", 1)[-1]) + namespace_match = re.match(r'{(.*)}', clixml.tag) + namespace = "{%s}" % namespace_match.group(1) if namespace_match else "" + + strings = clixml.findall("./%sS" % namespace) + lines = [e.text.replace('_x000D__x000A_', '') for e in strings if e.attrib.get('S') == stream_name] + return to_bytes('\r\n'.join(lines)) # FUTURE: determine buffer size at runtime via remote winrm config? def _put_file_stdin_iterator(self, in_path, out_path, buffer_size=250000): diff --git a/lib/ansible/plugins/shell/powershell.py b/lib/ansible/plugins/shell/powershell.py index 693d4127967..0b1bc43a3f2 100644 --- a/lib/ansible/plugins/shell/powershell.py +++ b/lib/ansible/plugins/shell/powershell.py @@ -44,6 +44,7 @@ import base64 import os import re import shlex +import pkgutil from ansible.errors import AnsibleError from ansible.module_utils._text import to_text @@ -78,8 +79,10 @@ begin { # stream JSON including become_pw, ps_module_payload, bin_module_payload, become_payload, write_payload_path, preserve directives # exec runspace, capture output, cleanup, return module output - # NB: do not adjust the following line- it is replaced when doing non-streamed module output - $json_raw = '' + # only init and stream in $json_raw if it wasn't set by the enclosing scope + if (-not $(Get-Variable "json_raw" -ErrorAction SilentlyContinue)) { + $json_raw = '' + } } process { $input_as_string = [string]$input @@ -1966,9 +1969,11 @@ class ShellModule(ShellBase): return self._encode_script(script) def build_module_command(self, env_string, shebang, cmd, arg_path=None): + bootstrap_wrapper = pkgutil.get_data("ansible.executor.powershell", "bootstrap_wrapper.ps1") + # pipelining bypass if cmd == '': - return '-' + return self._encode_script(script=bootstrap_wrapper, strict_mode=False, preserve_rc=False) # non-pipelining @@ -1976,8 +1981,11 @@ class ShellModule(ShellBase): cmd_parts = list(map(to_text, cmd_parts)) if shebang and shebang.lower() == '#!powershell': if not self._unquote(cmd_parts[0]).lower().endswith('.ps1'): + # we're running a module via the bootstrap wrapper cmd_parts[0] = '"%s.ps1"' % self._unquote(cmd_parts[0]) - cmd_parts.insert(0, '&') + wrapper_cmd = "type " + cmd_parts[0] + " | " + self._encode_script(script=bootstrap_wrapper, + strict_mode=False, preserve_rc=False) + return wrapper_cmd elif shebang and shebang.startswith('#!'): cmd_parts.insert(0, shebang[2:]) elif not shebang: diff --git a/test/integration/targets/win_become/tasks/main.yml b/test/integration/targets/win_become/tasks/main.yml index e860ff78e44..7e461568452 100644 --- a/test/integration/targets/win_become/tasks/main.yml +++ b/test/integration/targets/win_become/tasks/main.yml @@ -1,7 +1,7 @@ - set_fact: become_test_username: ansible_become_test become_test_admin_username: ansible_become_admin - gen_pw: password123! + {{ lookup('password', '/dev/null chars=ascii_letters,digits length=8') }} + gen_pw: "{{ 'password123!' + lookup('password', '/dev/null chars=ascii_letters,digits length=8') }}" - name: create unprivileged user win_user: @@ -29,6 +29,10 @@ - SeInteractiveLogonRight - SeBatchLogonRight +- name: fetch current target date/time for log filtering + raw: '[datetime]::now | Out-String' + register: test_starttime + - name: execute tests and ensure that test user is deleted regardless of success/failure block: - name: ensure current user is not the become user @@ -219,6 +223,7 @@ assert: that: - whoami_out is successful + - become_test_username in whoami_out.stdout when: os_version.stdout_lines[0] == "async" - name: test failure with string become invalid key @@ -320,6 +325,18 @@ - nonascii_output.stdout_lines[0] == 'über den Fußgängerübergang gehen' - nonascii_output.stderr == '' + - name: get PS events containing password or module args created since test start + raw: | + $dt=[datetime]"{{ test_starttime.stdout|trim }}" + (Get-WinEvent -LogName Microsoft-Windows-Powershell/Operational | + ? { $_.TimeCreated -ge $dt -and $_.Message -match "{{ gen_pw }}|whoami" }).Count + register: ps_log_count + + - name: assert no PS events contain password or module args + assert: + that: + - ps_log_count.stdout | int == 0 + # FUTURE: test raw + script become behavior once they're running under the exec wrapper again # FUTURE: add standalone playbook tests to include password prompting and play become keywords