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)

12 files changed, 91 insertions, 30 deletions

diff --git a/changelogs/fragments/ps_sb_logging.yaml b/changelogs/fragments/ps_sb_logging.yaml
new file mode 100644
index 0000000000..78241df449
--- /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 f91216c15f..545b4a5623 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 a27ea908a6..e585f94fff 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 0000000000..cfad82703d
--- /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 d860264ef1..c473c79ff4 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 9ad8261cf7..b12bac66b4 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 a7a338e5b2..5b528bf10d 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 459d45fc21..a4456d1003 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 325665ac42..d671b86c83 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 551a9e2db5..7f874002b0 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 0aab437471..9033e6af14 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 b067168b80..75d2dad1ce 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