From 8c1f701e6e9df29fe991f98265e2dd76acca4b8c Mon Sep 17 00:00:00 2001 From: Matt Davis Date: Mon, 26 Nov 2018 15:28:21 -0800 Subject: [PATCH] split PS wrapper and payload (CVE-2018-16859) (#49142) * 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) --- changelogs/fragments/ps_sb_logging.yaml | 2 ++ .../executor/powershell/async_wrapper.ps1 | 7 ++-- .../executor/powershell/become_wrapper.ps1 | 10 ++++-- .../executor/powershell/bootstrap_wrapper.ps1 | 7 ++++ .../executor/powershell/exec_wrapper.ps1 | 7 ++-- .../executor/powershell/module_manifest.py | 6 ++-- lib/ansible/plugins/action/script.py | 7 ++-- lib/ansible/plugins/connection/psrp.py | 1 + lib/ansible/plugins/connection/winrm.py | 14 +++++--- lib/ansible/plugins/shell/powershell.py | 9 +++-- .../targets/win_become/tasks/main.yml | 33 ++++++++++++++----- .../targets/win_exec_wrapper/tasks/main.yml | 18 +++++++++- 12 files changed, 91 insertions(+), 30 deletions(-) create mode 100644 changelogs/fragments/ps_sb_logging.yaml 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/powershell/async_wrapper.ps1 b/lib/ansible/executor/powershell/async_wrapper.ps1 index f91216c15ff..545b4a5623c 100644 --- a/lib/ansible/executor/powershell/async_wrapper.ps1 +++ b/lib/ansible/executor/powershell/async_wrapper.ps1 @@ -39,8 +39,9 @@ $Payload.async_results_path = $results_path $Payload.actions = $Payload.actions[1..99] $payload_json = ConvertTo-Json -InputObject $Payload -Depth 99 -Compress +# $exec_wrapper = [System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String($Payload.exec_wrapper)) -$exec_wrapper = $exec_wrapper.Replace("`$json_raw = ''", "`$json_raw = @'`r`n$payload_json`r`n'@") +$exec_wrapper += "`0`0`0`0" + $payload_json $payload_bytes = [System.Text.Encoding]::UTF8.GetBytes($exec_wrapper) $pipe_name = "ansible-async-$jid-$([guid]::NewGuid())" @@ -77,7 +78,9 @@ $bootstrap_wrapper = { $pipe.Close() } $exec = [System.Text.Encoding]::UTF8.GetString($input_bytes) - $exec = [ScriptBlock]::Create($exec) + $exec_parts = $exec.Split(@("`0`0`0`0"), 2, [StringSplitOptions]::RemoveEmptyEntries) + Set-Variable -Name json_raw -Value $exec_parts[1] + $exec = [ScriptBlock]::Create($exec_parts[0]) &$exec } diff --git a/lib/ansible/executor/powershell/become_wrapper.ps1 b/lib/ansible/executor/powershell/become_wrapper.ps1 index a27ea908a6e..e585f94fff9 100644 --- a/lib/ansible/executor/powershell/become_wrapper.ps1 +++ b/lib/ansible/executor/powershell/become_wrapper.ps1 @@ -98,11 +98,14 @@ Write-AnsibleLog "INFO - parsed become input, user: '$username', type: '$logon_t # NB: CreateProcessWithTokenW commandline maxes out at 1024 chars, must # bootstrap via small wrapper which contains the exec_wrapper passed through the # stdin pipe. Cannot use 'powershell -' as the $ErrorActionPreference is always -# set to Stop and cannot be changed +# set to Stop and cannot be changed. Also need to split the payload from the wrapper to prevent potentially +# sensitive content from being logged by the scriptblock logger. $bootstrap_wrapper = { &chcp.com 65001 > $null $exec_wrapper_str = [System.Console]::In.ReadToEnd() - $exec_wrapper = [ScriptBlock]::Create($exec_wrapper_str) + $split_parts = $exec_wrapper_str.Split(@("`0`0`0`0"), 2, [StringSplitOptions]::RemoveEmptyEntries) + Set-Variable -Name json_raw -Value $split_parts[1] + $exec_wrapper = [ScriptBlock]::Create($split_parts[0]) &$exec_wrapper } $exec_command = [System.Convert]::ToBase64String([System.Text.Encoding]::Unicode.GetBytes($bootstrap_wrapper.ToString())) @@ -115,8 +118,9 @@ $Payload.actions = $Payload.actions[1..99] $Payload.encoded_output = $true $payload_json = ConvertTo-Json -InputObject $Payload -Depth 99 -Compress +# delimit the payload JSON from the wrapper to keep sensitive contents out of scriptblocks (which can be logged) $exec_wrapper = [System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String($Payload.exec_wrapper)) -$exec_wrapper = $exec_wrapper.Replace("`$json_raw = ''", "`$json_raw = @'`r`n$payload_json`r`n'@") +$exec_wrapper += "`0`0`0`0" + $payload_json try { Write-AnsibleLog "INFO - starting become process '$lp_command_line'" "become_wrapper" 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/executor/powershell/exec_wrapper.ps1 b/lib/ansible/executor/powershell/exec_wrapper.ps1 index d860264ef1f..c473c79ff45 100644 --- a/lib/ansible/executor/powershell/exec_wrapper.ps1 +++ b/lib/ansible/executor/powershell/exec_wrapper.ps1 @@ -159,9 +159,10 @@ $($ErrorRecord.InvocationInfo.PositionMessage) } .$wrapper_functions - # NB: do not adjust the following line - it is replaced when doing - # non-streamed input - $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 { $json_raw += [String]$input } end { diff --git a/lib/ansible/executor/powershell/module_manifest.py b/lib/ansible/executor/powershell/module_manifest.py index 9ad8261cf74..b12bac66b44 100644 --- a/lib/ansible/executor/powershell/module_manifest.py +++ b/lib/ansible/executor/powershell/module_manifest.py @@ -280,9 +280,7 @@ def _create_powershell_wrapper(b_module_data, module_args, environment, exec_manifest['csharp_utils'][name] = b64_data exec_manifest['csharp_utils_module'] = list(finder.cs_utils_module.keys()) - # FUTURE: smuggle this back as a dict instead of serializing here; - # the connection plugin may need to modify it b_json = to_bytes(json.dumps(exec_manifest)) - b_data = exec_wrapper.replace(b"$json_raw = ''", - b"$json_raw = @'\r\n%s\r\n'@" % b_json) + # delimit the payload JSON from the wrapper to keep sensitive contents out of scriptblocks (which can be logged) + b_data = exec_wrapper + b'\0\0\0\0' + b_json return b_data diff --git a/lib/ansible/plugins/action/script.py b/lib/ansible/plugins/action/script.py index a7a338e5b29..5b528bf10d2 100644 --- a/lib/ansible/plugins/action/script.py +++ b/lib/ansible/plugins/action/script.py @@ -127,14 +127,17 @@ class ActionModule(ActionBase): # PowerShell runs the script in a special wrapper to enable things # like become and environment args if self._connection._shell.SHELL_FAMILY == "powershell": - # FIXME: use a more public method to get the exec payload + # FUTURE: use a more public method to get the exec payload pc = self._play_context exec_data = ps_manifest._create_powershell_wrapper( to_bytes(script_cmd), {}, env_dict, self._task.async_val, pc.become, pc.become_method, pc.become_user, pc.become_pass, pc.become_flags, substyle="script" ) - script_cmd = "-" + # 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/psrp.py b/lib/ansible/plugins/connection/psrp.py index 459d45fc210..a4456d1003d 100644 --- a/lib/ansible/plugins/connection/psrp.py +++ b/lib/ansible/plugins/connection/psrp.py @@ -282,6 +282,7 @@ class Connection(ConnectionBase): # starting a new interpreter to save on time b_command = base64.b64decode(cmd.split(" ")[-1]) script = to_text(b_command, 'utf-16-le') + in_data = to_text(in_data, errors="surrogate_or_strict", nonstring="passthru") display.vvv("PSRP: EXEC %s" % script, host=self._psrp_host) else: # in other cases we want to execute the cmd as the script diff --git a/lib/ansible/plugins/connection/winrm.py b/lib/ansible/plugins/connection/winrm.py index 325665ac42b..d671b86c83e 100644 --- a/lib/ansible/plugins/connection/winrm.py +++ b/lib/ansible/plugins/connection/winrm.py @@ -103,6 +103,7 @@ import traceback import json import tempfile import subprocess +import xml.etree.ElementTree as ET HAVE_KERBEROS = False try: @@ -531,14 +532,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 551a9e2db59..7f874002b0b 100644 --- a/lib/ansible/plugins/shell/powershell.py +++ b/lib/ansible/plugins/shell/powershell.py @@ -55,6 +55,7 @@ import base64 import os import re import shlex +import pkgutil from ansible.errors import AnsibleError from ansible.module_utils._text import to_text @@ -208,9 +209,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 @@ -218,8 +221,10 @@ 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 0aab4374710..9033e6af146 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 @@ -82,7 +86,7 @@ vars: *admin_become_vars win_whoami: register: whoami_out - + - name: verify output assert: that: @@ -121,7 +125,7 @@ - whoami_out.label.account_name == 'Medium Mandatory Level' - whoami_out.label.sid == 'S-1-16-8192' - whoami_out.logon_type == 'Interactive' - + - name: test with module that will return non-zero exit code (https://github.com/ansible/ansible/issues/30468) vars: *become_vars setup: @@ -138,14 +142,14 @@ - '"Failed to become user " + become_test_username not in become_invalid_pass.msg' - '"LogonUser failed" not in become_invalid_pass.msg' - '"Win32ErrorCode 1326)" not in become_invalid_pass.msg' - + - name: test become with SYSTEM account win_whoami: become: yes become_method: runas become_user: SYSTEM register: whoami_out - + - name: verify output assert: that: @@ -162,7 +166,7 @@ become_method: runas become_user: NetworkService register: whoami_out - + - name: verify output assert: that: @@ -179,7 +183,7 @@ become_method: runas become_user: LocalService register: whoami_out - + - name: verify output assert: that: @@ -195,11 +199,12 @@ win_command: whoami async: 10 register: whoami_out - + - name: verify become + async worked assert: that: - whoami_out is successful + - become_test_username in whoami_out.stdout - name: test failure with string become invalid key vars: *become_vars @@ -313,6 +318,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 diff --git a/test/integration/targets/win_exec_wrapper/tasks/main.yml b/test/integration/targets/win_exec_wrapper/tasks/main.yml index b067168b80f..75d2dad1ceb 100644 --- a/test/integration/targets/win_exec_wrapper/tasks/main.yml +++ b/test/integration/targets/win_exec_wrapper/tasks/main.yml @@ -1,4 +1,8 @@ --- +- name: fetch current target date/time for log filtering + raw: '[datetime]::now | Out-String' + register: test_starttime + - name: test normal module execution test_fail: register: normal @@ -180,7 +184,7 @@ - set_fact: become_test_username: ansible_become_test - 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: @@ -248,3 +252,15 @@ that: - not common_functions_res is failed - common_functions_res.msg == "good" + +- name: get PS events containing module args or envvars created since test start + raw: | + $dt=[datetime]"{{ test_starttime.stdout|trim }}" + (Get-WinEvent -LogName Microsoft-Windows-Powershell/Operational | + ? { $_.TimeCreated -ge $dt -and $_.Message -match "test_fail|fail_module|hyphen-var" }).Count + register: ps_log_count + +- name: assert no PS events contain module args or envvars + assert: + that: + - ps_log_count.stdout | int == 0