Skip to content
Permalink
Browse files

[security] core: undirectional routing wasn't respected in some cases

When creating a context using Router.method(via=somechild),
unidirectional mode was set on the new child correctly, however if the
child were to call Router.method(), due to a typing mistake the new
child would start without it.

This doesn't impact the Ansible extension, as only forked tasks are
started directly by children, and they are not responsible for routing
messages.

Add test so it can't happen again.
  • Loading branch information...
dw committed Aug 8, 2019
1 parent 436a4b3 commit 5924af1566763e48c42028399ea0cd95c457b3dc
Showing with 36 additions and 14 deletions.
  1. +1 −1 mitogen/core.py
  2. +35 −13 tests/router_test.py
@@ -3623,7 +3623,7 @@ def _setup_master(self):
self.broker = Broker(activate_compat=False)
self.router = Router(self.broker)
self.router.debug = self.config.get('debug', False)
self.router.undirectional = self.config['unidirectional']
self.router.unidirectional = self.config['unidirectional']
self.router.add_handler(
fn=self._on_shutdown_msg,
handle=SHUTDOWN,
@@ -1,9 +1,11 @@
import sys
import time
import zlib

import unittest2

import testlib
import mitogen.core
import mitogen.master
import mitogen.parent
import mitogen.utils
@@ -341,22 +343,42 @@ def test_previously_alive_context_returns_dead(self):
))


def test_siblings_cant_talk(router):
l1 = router.local()
l2 = router.local()
logs = testlib.LogCapturer()
logs.start()

try:
l2.call(ping_context, l1)
except mitogen.core.CallError:
e = sys.exc_info()[1]

msg = mitogen.core.Router.unidirectional_msg % (
l2.context_id,
l1.context_id,
)
assert msg in str(e)
assert 'routing mode prevents forward of ' in logs.stop()


@mitogen.core.takes_econtext
def test_siblings_cant_talk_remote(econtext):
mitogen.parent.upgrade_router(econtext)
test_siblings_cant_talk(econtext.router)


class UnidirectionalTest(testlib.RouterMixin, testlib.TestCase):
def test_siblings_cant_talk(self):
def test_siblings_cant_talk_master(self):
self.router.unidirectional = True
l1 = self.router.local()
l2 = self.router.local()
logs = testlib.LogCapturer()
logs.start()
e = self.assertRaises(mitogen.core.CallError,
lambda: l2.call(ping_context, l1))
test_siblings_cant_talk(self.router)

msg = self.router.unidirectional_msg % (
l2.context_id,
l1.context_id,
)
self.assertTrue(msg in str(e))
self.assertTrue('routing mode prevents forward of ' in logs.stop())
def test_siblings_cant_talk_parent(self):
# ensure 'unidirectional' attribute is respected for contexts started
# by children.
self.router.unidirectional = True
parent = self.router.local()
parent.call(test_siblings_cant_talk_remote)

def test_auth_id_can_talk(self):
self.router.unidirectional = True

4 comments on commit 5924af1

@greysteil

This comment has been minimized.

Copy link

replied Aug 20, 2019

Thanks for the fix on this @dw.

I'm on the security team at GitHub and we were notified that a CVE was published for this vulnerability yesterday. I'd love to know:

  1. did you publish that CVE or did someone else?
  2. did you consider creating a Security Advisory in GitHub for this vulnerability? (They're new, if you've never heard of them...!)

Any feedback you can give on how GitHub can help in dealing with security-related issues would be very appreciated. You can reach me on greysteil@github.com if you'd rather not leave a public comment.

@dw

This comment has been minimized.

Copy link
Owner Author

replied Aug 20, 2019

@greysteil

This comment has been minimized.

Copy link

replied Aug 20, 2019

Ack, that sucks, and isn't the first time we've heard that feedback.

CVE issuance is a big marketing tool for some companies, and in some cases employees are incentivised to get them assigned and skip parts of the process (like informing the maintainer...).

One thing we're planning at GitHub is to become a CVE-issuing entity (a CNA) ourselves, which should allow us to help improve the process and guard against situations like this. In the meantime I'll ping our vulnerability curation team and see what we can do about removing / flagging this in our database.

@dw

This comment has been minimized.

Copy link
Owner Author

replied Aug 20, 2019

It's possible the package gets more attention because it is in multiple downstream package repositories nowadays, I hadn't considered people might be watching the changelog in this way. I assumed it might have been an enthusiastic user who filed it, not sure. No harm done in any case, just a curious amount of unfortunate noise to round off many months of hard work :)

Please sign in to comment.
You can’t perform that action at this time.