Skip to content

Commit

Permalink
[security] core: undirectional routing wasn't respected in some cases
Browse files Browse the repository at this point in the history
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 5924af1
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 14 deletions.
2 changes: 1 addition & 1 deletion mitogen/core.py
Expand Up @@ -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,
Expand Down
48 changes: 35 additions & 13 deletions tests/router_test.py
@@ -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
Expand Down Expand Up @@ -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
Expand Down

4 comments on commit 5924af1

@greysteil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

@dw dw commented on 5924af1 Aug 20, 2019 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greysteil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

@dw dw commented on 5924af1 Aug 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.