Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mitogen broken after ansible security patch for "unsafe variables" #1034

Closed
upekkha opened this issue Dec 11, 2023 · 16 comments
Closed

Mitogen broken after ansible security patch for "unsafe variables" #1034

upekkha opened this issue Dec 11, 2023 · 16 comments
Assignees
Labels
affects-0.3 Issues related to 0.3.X Mitogen releases bug Code feature that hinders desired execution outcome

Comments

@upekkha
Copy link
Contributor

upekkha commented Dec 11, 2023

Ansible has released a security patch for CVE-2023-5764 in all maintained versions, namely 2.16.1, 2.15.7 and 2.14.12. This patch breaks mitogen, as seen in the example of a simple package installation task:

ansible somehost -m package -a 'pkg=vim' -D -C -vvv
[...]
The full traceback is:
Traceback (most recent call last):
  File "/path/to/ansible/lib/ansible/executor/task_executor.py", line 165, in run
    res = self._execute()
          ^^^^^^^^^^^^^^^
  File "/path/to/ansible/lib/ansible/executor/task_executor.py", line 641, in _execute
    result = self._handler.run(task_vars=vars_copy)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/mitogen/ansible_mitogen/mixins.py", line 146, in run
    return super(ActionModuleMixin, self).run(tmp, task_vars)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/ansible/lib/ansible/plugins/action/package.py", line 85, in run
    result.update(self._execute_module(module_name=module, module_args=new_module_args, task_vars=task_vars, wrap_async=self._task.async_val))
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/mitogen/ansible_mitogen/mixins.py", line 386, in _execute_module
    result = ansible_mitogen.planner.invoke(
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/mitogen/ansible_mitogen/planner.py", line 609, in invoke
    response = invocation.connection.get_chain().call(
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/mitogen/ansible_mitogen/connection.py", line 466, in call
    return self._rethrow(recv)
           ^^^^^^^^^^^^^^^^^^^
  File "/path/to/mitogen/ansible_mitogen/connection.py", line 452, in _rethrow
    return recv.get().unpickle()
           ^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/mitogen/mitogen/core.py", line 977, in unpickle
    raise obj
mitogen.core.CallError: mitogen.core.StreamError: cannot unpickle 'ansible.utils.unsafe_proxy'/'AnsibleUnsafeText'
  File "<stdin>", line 3698, in _dispatch_one
  File "<stdin>", line 3681, in _parse_request
  File "<stdin>", line 966, in unpickle
  File "<stdin>", line 757, in find_class
  File "<stdin>", line 867, in _find_global

somehost | FAILED! => {
    "msg": "Unexpected failure during module execution: mitogen.core.StreamError: cannot unpickle 'ansible.utils.unsafe_proxy'/'AnsibleUnsafeText'\n  File \"<stdin>\", line 3698, in _dispatch_one\n  File \"<stdin>\", line 3681, in _parse_request\n  File \"<stdin>\", line 966, in unpickle\n  File \"<stdin>\", line 757, in find_class\n  File \"<stdin>\", line 867, in _find_global\n",
    "stdout": ""
}

For example in 2.16.1, the culprit is ansible/ansible@270b39f. The problem persists on the current stable-2.16 branch and is not fixed by unreleased commits like "Additional unsafe fixes" ansible/ansible#82376.

Considering that Ansible 2.13 is end-of-life since November and all maintained versions (2.14-2.16) are currently broken, we are left in the undesirable situation, where no supported Ansible version remains compatible with mitogen.

@upekkha upekkha added affects-0.3 Issues related to 0.3.X Mitogen releases bug Code feature that hinders desired execution outcome labels Dec 11, 2023
@AKrumov
Copy link

AKrumov commented Dec 12, 2023

Here is workaround how to make mitogen work with ansible v2.16.2:

Open file: mitogen/mitogen/core.py
Go to line: 845 and replace def _find_global with this:

    def _unpickle_ansible_unsafe_text(self, serialized_obj):
        return serialized_obj

    def _find_global(self, module, func):
        """
        Return the class implementing `module_name.class_name` or raise
        `StreamError` if the module is not whitelisted.
        """
        print(module, __name__)
        if module == __name__:
            if func == '_unpickle_call_error' or func == 'CallError':
                return _unpickle_call_error
            elif func == '_unpickle_sender':
                return self._unpickle_sender
            elif func == '_unpickle_context':
                return self._unpickle_context
            elif func == 'Blob':
                return Blob
            elif func == 'Secret':
                return Secret
            elif func == 'Kwargs':
                return Kwargs
        elif module == 'ansible.utils.unsafe_proxy' and func == 'AnsibleUnsafeText':
            return self._unpickle_ansible_unsafe_text
        elif module == '_codecs' and func == 'encode':
            return self._unpickle_bytes
        elif module == '__builtin__' and func == 'bytes':
            return BytesType
        raise StreamError('cannot unpickle %r/%r', module, func)

Open file: mitogen/ansible_mitogen/loaders.py
Go to line: 52 and replace it with:

ANSIBLE_VERSION_MAX = (2, 16)

@upekkha
Copy link
Contributor Author

upekkha commented Dec 12, 2023

Hey @AKrumov

Thanks for looking into this. I can confirm that your patch is working for me as well.

@FlorianLaunay
Copy link

Many thanks @AKrumov! It's working for me too!

I'm sad to see this project is abandoned.

innovationfleet added a commit to innovationfleet/mitogen that referenced this issue Dec 20, 2023
@baryluk
Copy link

baryluk commented Jan 3, 2024

As part of my fixes for Mitogen 2.16 and Python 3.12, in #1033 (comment) I solved this in my patch, slightly differently:

In mitogen/core.py:

+AnsibleUnsafeText = None
+
+def lazy_AnsibleUnsafeText():
+    global AnsibleUnsafeText
+    if AnsibleUnsafeText is not None:
+        return AnsibleUnsafeText
+    mod = __import__("ansible.utils.unsafe_proxy", fromlist=("AnsibleUnsafeText",))
+    AnsibleUnsafeText = getattr(mod, "AnsibleUnsafeText")
+    assert type(AnsibleUnsafeText) is type, f"AnsibleUnsafeText {AnsibleUnsafeText} is not a type"
+    assert callable(AnsibleUnsafeText), f"AnsibleUnsafeText {AnsibleUnsafeText} is not callable"
+    return AnsibleUnsafeText
+
+
@@ -860,6 +893,8 @@ class Message(object):
                 return Secret
             elif func == 'Kwargs':
                 return Kwargs
+        elif module == 'ansible.utils.unsafe_proxy' and func == 'AnsibleUnsafeText':
+            return lazy_AnsibleUnsafeText()
         elif module == '_codecs' and func == 'encode':
             return self._unpickle_bytes
         elif module == '__builtin__' and func == 'bytes':

@opoplawski
Copy link
Contributor

Thank you for the workarounds. Could we get some comments on the relative merits of the two versions? Are we simply subverting the CVE fix or actually incorporating it into mitogen?

@baryluk
Copy link

baryluk commented Jan 11, 2024

I do not know what the Unsafe proxy actually protects against, but my random guess is that it somehow intercepts calls to logging and print, and does not allow them to be printed or logged to output of ansible (i.e. when running with -v), but conversion to str should work.

It is not a serious security risk, unless you make your log files publicly visible, or something (i.e. when running from AWX / Tower, but people that should not have access to original secrets).

DChalcraft added a commit to DChalcraft/mitogen that referenced this issue Jan 22, 2024
AnatomicJC added a commit to AnatomicJC/mitogen that referenced this issue Jan 22, 2024
@amarao
Copy link

amarao commented Feb 29, 2024

@AKrumov, would mind if I use your patch for serverscom.mitogen? I already do autopatch for ANSIBLE_VERSION, seems like it's time to start adding more patches.

@gilesw
Copy link

gilesw commented Mar 5, 2024

I'm not helping here but I'm using the patch and it fixes the issue but my plays now have lots of lines of:-

mitogen.core mitogen.core
mitogen.core mitogen.core
mitogen.core mitogen.core
mitogen.core mitogen.core
mitogen.core mitogen.core
mitogen.core mitogen.core
mitogen.core mitogen.core
mitogen.core mitogen.core
_codecs mitogen.core

@opoplawski
Copy link
Contributor

Drop the print() statement in the patch. It presumably was just for debugging purposes.

@moreati moreati self-assigned this Mar 18, 2024
@AKrumov
Copy link

AKrumov commented Mar 28, 2024

@AKrumov, would mind if I use your patch for serverscom.mitogen? I already do autopatch for ANSIBLE_VERSION, seems like it's time to start adding more patches.

Absolutely! Go ahead and use the patch for serverscom.mitogen. Sorry for getting back to you late. If you need anything else or have more questions, just let me know!

@moreati
Copy link
Member

moreati commented Mar 29, 2024

I believe the changes in #1017 will fix this, based on branch https://github.com/moreati/mitogen/tree/2.14. Please try it and let me know how it goes. If all is well this will go into master, and then release 0.3.6. Sorry for the long wait, and thanks for helping with interim workarounds.

@nerijus
Copy link
Contributor

nerijus commented Apr 3, 2024

It works, even with ansible 2.16.5 and this patch:

--- a/ansible_mitogen/loaders.py
+++ b/ansible_mitogen/loaders.py
@@ -49,7 +49,7 @@ __all__ = [
 
 
 ANSIBLE_VERSION_MIN = (2, 10)
-ANSIBLE_VERSION_MAX = (2, 14)
+ANSIBLE_VERSION_MAX = (2, 16)
 
 NEW_VERSION_MSG = (
     "Your Ansible version (%s) is too recent. The most recent version\n"

Could you please add support for 2.16 too?

@roman-vynar
Copy link

@nerijus just put 99 and not bother:

--- a/ansible_mitogen/loaders.py
+++ b/ansible_mitogen/loaders.py
@@ -49,7 +49,7 @@ __all__ = [
 
 
 ANSIBLE_VERSION_MIN = (2, 10)
-ANSIBLE_VERSION_MAX = (2, 13)
+ANSIBLE_VERSION_MAX = (2, 99)

Mitogen has always worked with the most recent ansible :) Not even sure why this is checked.

@nerijus
Copy link
Contributor

nerijus commented Apr 3, 2024

No, not always :)

@moreati
Copy link
Member

moreati commented Apr 4, 2024

Could you please add support for 2.16 too?

Not in the next release (0.3.6). I'm aiming for smaller, more frequent releases. 2.15 and 2.16 will be in subsequent release(s).

@moreati
Copy link
Member

moreati commented Apr 4, 2024

Mitogen 0.3.6 is now out.

@moreati moreati closed this as completed Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-0.3 Issues related to 0.3.X Mitogen releases bug Code feature that hinders desired execution outcome
Projects
None yet
Development

No branches or pull requests

10 participants