From 7f4befdea77045fa83b5f2b304bd5e16b219f74c Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Fri, 11 Oct 2019 09:17:10 -0500 Subject: [PATCH] Wrap CLI Passwords with AnsibleUnsafeText, ensure unsafe context is not lost during encode/decode (#63351) * Wrap .encode and .decode on AnsibleUnsafe objects * runme.sh needs to be executable * ci_complete * Update changelog with CVE --- .../fragments/dont-template-cli-passwords.yml | 12 +++++++++++ lib/ansible/cli/__init__.py | 10 +++------ lib/ansible/template/__init__.py | 2 +- lib/ansible/utils/unsafe_proxy.py | 8 +++++-- test/integration/targets/cli/aliases | 2 ++ test/integration/targets/cli/runme.sh | 7 +++++++ test/integration/targets/cli/setup.yml | 4 ++++ test/integration/targets/cli/test-cli.py | 21 +++++++++++++++++++ test/units/module_utils/test_text.py | 11 ++++++++++ test/units/playbook/test_base.py | 10 +++++++++ 10 files changed, 77 insertions(+), 10 deletions(-) create mode 100644 changelogs/fragments/dont-template-cli-passwords.yml create mode 100644 test/integration/targets/cli/aliases create mode 100755 test/integration/targets/cli/runme.sh create mode 100644 test/integration/targets/cli/setup.yml create mode 100644 test/integration/targets/cli/test-cli.py diff --git a/changelogs/fragments/dont-template-cli-passwords.yml b/changelogs/fragments/dont-template-cli-passwords.yml new file mode 100644 index 00000000000..5c8dbea7e19 --- /dev/null +++ b/changelogs/fragments/dont-template-cli-passwords.yml @@ -0,0 +1,12 @@ +bugfixes: +- > + **security issue** - Convert CLI provided passwords to text initially, to + prevent unsafe context being lost when converting from bytes->text during + post processing of PlayContext. This prevents CLI provided passwords from + being incorrectly templated (CVE-2019-14856) +- > + **security issue** - Update ``AnsibleUnsafeText`` and ``AnsibleUnsafeBytes`` + to maintain unsafe context by overriding ``.encode`` and ``.decode``. This + prevents future issues with ``to_text``, ``to_bytes``, or ``to_native`` + removing the unsafe wrapper when converting between string types + (CVE-2019-14856) diff --git a/lib/ansible/cli/__init__.py b/lib/ansible/cli/__init__.py index c725db0d003..ab1a180de4c 100644 --- a/lib/ansible/cli/__init__.py +++ b/lib/ansible/cli/__init__.py @@ -29,7 +29,7 @@ from ansible.release import __version__ from ansible.utils.collection_loader import AnsibleCollectionLoader, get_collection_name_from_path, set_collection_playbook_paths from ansible.utils.display import Display from ansible.utils.path import unfrackpath -from ansible.utils.unsafe_proxy import AnsibleUnsafeBytes +from ansible.utils.unsafe_proxy import to_unsafe_text from ansible.vars.manager import VariableManager try: @@ -240,8 +240,6 @@ class CLI(with_metaclass(ABCMeta, object)): if op['ask_pass']: sshpass = getpass.getpass(prompt="SSH password: ") become_prompt = "%s password[defaults to SSH password]: " % become_prompt_method - if sshpass: - sshpass = to_bytes(sshpass, errors='strict', nonstring='simplerepr') else: become_prompt = "%s password: " % become_prompt_method @@ -249,17 +247,15 @@ class CLI(with_metaclass(ABCMeta, object)): becomepass = getpass.getpass(prompt=become_prompt) if op['ask_pass'] and becomepass == '': becomepass = sshpass - if becomepass: - becomepass = to_bytes(becomepass) except EOFError: pass # we 'wrap' the passwords to prevent templating as # they can contain special chars and trigger it incorrectly if sshpass: - sshpass = AnsibleUnsafeBytes(sshpass) + sshpass = to_unsafe_text(sshpass) if becomepass: - becomepass = AnsibleUnsafeBytes(becomepass) + becomepass = to_unsafe_text(becomepass) return (sshpass, becomepass) diff --git a/lib/ansible/template/__init__.py b/lib/ansible/template/__init__.py index 1503c82ea7e..fa4b11afe11 100644 --- a/lib/ansible/template/__init__.py +++ b/lib/ansible/template/__init__.py @@ -272,7 +272,7 @@ class AnsibleContext(Context): for item in val: if self._is_unsafe(item): return True - elif isinstance(val, string_types) and hasattr(val, '__UNSAFE__'): + elif hasattr(val, '__UNSAFE__'): return True return False diff --git a/lib/ansible/utils/unsafe_proxy.py b/lib/ansible/utils/unsafe_proxy.py index 59c805d53e6..3c74ea83699 100644 --- a/lib/ansible/utils/unsafe_proxy.py +++ b/lib/ansible/utils/unsafe_proxy.py @@ -66,11 +66,15 @@ class AnsibleUnsafe(object): class AnsibleUnsafeBytes(binary_type, AnsibleUnsafe): - pass + def decode(self, *args, **kwargs): + """Wrapper method to ensure type conversions maintain unsafe context""" + return AnsibleUnsafeText(super(AnsibleUnsafeBytes, self).decode(*args, **kwargs)) class AnsibleUnsafeText(text_type, AnsibleUnsafe): - pass + def encode(self, *args, **kwargs): + """Wrapper method to ensure type conversions maintain unsafe context""" + return AnsibleUnsafeBytes(super(AnsibleUnsafeText, self).encode(*args, **kwargs)) class UnsafeProxy(object): diff --git a/test/integration/targets/cli/aliases b/test/integration/targets/cli/aliases new file mode 100644 index 00000000000..6b71e884a14 --- /dev/null +++ b/test/integration/targets/cli/aliases @@ -0,0 +1,2 @@ +needs/target/setup_pexpect +shippable/posix/group3 diff --git a/test/integration/targets/cli/runme.sh b/test/integration/targets/cli/runme.sh new file mode 100755 index 00000000000..d9e846256ff --- /dev/null +++ b/test/integration/targets/cli/runme.sh @@ -0,0 +1,7 @@ +#!/usr/bin/env bash + +set -eux + +ANSIBLE_ROLES_PATH=../ ansible-playbook setup.yml + +python test-cli.py diff --git a/test/integration/targets/cli/setup.yml b/test/integration/targets/cli/setup.yml new file mode 100644 index 00000000000..9f6ab117412 --- /dev/null +++ b/test/integration/targets/cli/setup.yml @@ -0,0 +1,4 @@ +- hosts: localhost + gather_facts: no + roles: + - setup_pexpect diff --git a/test/integration/targets/cli/test-cli.py b/test/integration/targets/cli/test-cli.py new file mode 100644 index 00000000000..9893d6652ed --- /dev/null +++ b/test/integration/targets/cli/test-cli.py @@ -0,0 +1,21 @@ +#!/usr/bin/env python +# Copyright (c) 2019 Matt Martz +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +# Make coding more python3-ish +from __future__ import (absolute_import, division, print_function) +__metaclass__ = type + +import os + +import pexpect + +os.environ['ANSIBLE_NOCOLOR'] = '1' +out = pexpect.run( + 'ansible localhost -m debug -a msg="{{ ansible_password }}" -k', + events={ + 'SSH password:': '{{ 1 + 2 }}\n' + } +) + +assert b'{{ 1 + 2 }}' in out diff --git a/test/units/module_utils/test_text.py b/test/units/module_utils/test_text.py index 492aa3e9443..49f299e4048 100644 --- a/test/units/module_utils/test_text.py +++ b/test/units/module_utils/test_text.py @@ -16,6 +16,7 @@ from ansible.module_utils.six import PY3 # Internal API while this is still being developed. Eventually move to # module_utils.common.text from ansible.module_utils._text import to_text, to_bytes, to_native +from ansible.utils.unsafe_proxy import AnsibleUnsafeBytes, AnsibleUnsafeText # Format: byte representation, text representation, encoding of byte representation @@ -51,3 +52,13 @@ def test_to_bytes(in_string, encoding, expected): def test_to_native(in_string, encoding, expected): """test happy path of encoding to native strings""" assert to_native(in_string, encoding) == expected + + +def test_to_text_unsafe(): + assert isinstance(to_text(AnsibleUnsafeBytes(b'foo')), AnsibleUnsafeText) + assert to_text(AnsibleUnsafeBytes(b'foo')) == AnsibleUnsafeText(u'foo') + + +def test_to_bytes_unsafe(): + assert isinstance(to_bytes(AnsibleUnsafeText(u'foo')), AnsibleUnsafeBytes) + assert to_bytes(AnsibleUnsafeText(u'foo')) == AnsibleUnsafeBytes(b'foo') diff --git a/test/units/playbook/test_base.py b/test/units/playbook/test_base.py index dc3509577e1..0b51271772f 100644 --- a/test/units/playbook/test_base.py +++ b/test/units/playbook/test_base.py @@ -26,6 +26,7 @@ from ansible.module_utils.six import string_types from ansible.playbook.attribute import FieldAttribute from ansible.template import Templar from ansible.playbook import base +from ansible.utils.unsafe_proxy import AnsibleUnsafeBytes, AnsibleUnsafeText from units.mock.loader import DictDataLoader @@ -620,3 +621,12 @@ class TestBaseSubClass(TestBase): ds = {'test_attr_method_missing': a_string} bsc = self._base_validate(ds) self.assertEquals(bsc.test_attr_method_missing, a_string) + + def test_get_validated_value_string_rewrap_unsafe(self): + attribute = FieldAttribute(isa='string') + value = AnsibleUnsafeText(u'bar') + templar = Templar(None) + bsc = self.ClassUnderTest() + result = bsc.get_validated_value('foo', attribute, value, templar) + self.assertIsInstance(result, AnsibleUnsafeText) + self.assertEquals(result, AnsibleUnsafeText(u'bar'))