are module imports safe?

[Seth Arnold]

Hello, I'm conducting a very quick review of oslo.privsep as part of the
Main Inclusion Process for Ubuntu.

I'm curious how oslo.privsep prevents malicious imports:

    def _process_cmd(self, cmd, *args):
        if cmd == Message.PING:
            return (Message.PONG.value,)

        elif cmd == Message.CALL:
            name, f_args, f_kwargs = args
            func = importutils.import_class(name)

            if not self.context.is_entrypoint(func):
                msg = _('Invalid privsep function: %s not exported') % name
                raise NameError(msg)

            ret = func(*f_args, **f_kwargs)
            return (Message.RET.value, ret)

        raise ProtocolError(_('Unknown privsep cmd: %s') % cmd)

This calls importutils.import_class() using data that comes from an
untrusted source.

import_class() doesn't appear to do any filtering of any sort to prevent
specifying module paths that aren't allowed:

def import_class(import_str):
    """Returns a class from a string including module and class.
    .. versionadded:: 0.3
    """
    mod_str, _sep, class_str = import_str.rpartition('.')
    __import__(mod_str)
    try:
        return getattr(sys.modules[mod_str], class_str)
    except AttributeError:
        raise ImportError('Class %s cannot be found (%s)' %
                          (class_str,
                           traceback.format_exception(*sys.exc_info())))

A few versions of Python documentation don't describe any attempts to
filter __import__() to only trusted module loading paths:
https://docs.python.org/2/library/functions.html#__import__
https://docs.python.org/3.3/library/functions.html#__import__

Is it necessary to allow arbitrary module loading? This has caused no end
of trouble for the Java community. (See posts from Security Explorations
to full-disclosure for the last four or five years.)

Thanks

[Seth Arnold (seth-arnold)]

I added a manual subscription to hopefully make these visible. If that didn't do it, I'll just open them public.

Thanks

[Jeremy Stanley (fungi)]

Since this report concerns a possible security risk, an incomplete security advisory task has been added while the core security reviewers for the affected project or projects confirm the bug and discuss the scope of any vulnerability along with potential solutions.

[Jeremy Stanley (fungi)]

While oslo.privsep isn't handled directly by the OpenStack Vulnerability Management Team, I've triaged this as a potential vulnerability reported under embargo on behalf of the oslo.privsep developers and am happy to consult on the topic. Now that the Oslo core security review team has been subscribed as well, hopefully they will add any necessary subject matter experts for oslo.privsep to directly address the rationale for the current design.

It's worth pointing out from a general OpenStack-wide perspective however, that the focus of oslo.privsep is to at least incrementally improve over the existing widespread "rootwrap" implementation we have which simply whitelists commands to be run as root based on shipped or configured regular expressions (which most of the time just end up containing something akin to a wildcard match). That said, it's almost certainly not perfect and if there are ways to further shore up the design then I'm sure they're quite welcome.

[Joshua Harlow (harlowja)]

Subscribed angus, he should be able to answer any questions here.

It may be advantageous to have a token that the sending and consuming processes share and only if that token is valid will the import actually go through (otherwise it will be denied). I believe this is similar to what multiprocessing does. Though it is also my understanding that privsep communication channels are 'private' between the application that uses privsep and the privsep daemon (though a token passed and verified between each wouldn't seem to hurt).

I'll let angus answer more questions since he can probably do a better job :)

[Seth Arnold (seth-arnold)]

If the unprivileged portion of the code is compromised, presumably the token is still in the address space somewhere; it could be retrieved when making requests.

If it happens to be e.g. swift or glance that's been compromised, a user might have uploaded their own module via legitimate means and then request it to be loaded via the rpc mechanism here.

I'm leery of proposing solutions when I don't know the problem space very well, but I could imagine a few different possibilities:

- A handful of modules are loaded at process start time and only properly annotated functions from these modules could be run
- A whitelist of allowed module names could be checked
- A whilelist of allowed module paths could be checked

Thanks

[Angus Lees (gus)]

First of all, thanks for reviewing the code/design. More eyes are always
welcome.

My assumptions in privsep is that the privileged python module path
contains only trustworthy code:
- either it is limited to root's python module path by sudo's env cleaning
(in the start-by-exec method)
- or it is the initial environment as given by systemd/other (in the
start-with-privs-then-fork method)

(Or to phrase this differently: My assumption is that if arbitrary code can
be injected into the python module path, then we've already lost.)

Following this reasoning, `_process_cmd` assumes it is safe to import any
module in the privileged module path. If there is a way to trick
`import_class` into loading modules *outside* the module load path, then
there is indeed an issue here - and I would value your additional
verification of this.

Regarding harlowja's token comment: The client-side unix socket fd is the
token. If and only if you have access to this socket, you are allowed to
make calls - and the socket connection is established in a way that does
not make it available to any other process (without being same uid/root and
manipulating the process via debuggers or similar). Again, additional
verification of this is valuable.

 - Gus

On Fri, 30 Sep 2016 at 13:40 Seth Arnold <email address hidden> wrote:

If the unprivileged portion of the code is compromised, presumably the
token is still in the address space somewhere; it could be retrieved
when making requests.

If it happens to be e.g. swift or glance that's been compromised, a user
might have uploaded their own module via legitimate means and then
request it to be loaded via the rpc mechanism here.

I'm leery of proposing solutions when I don't know the problem space
very well, but I could imagine a few different possibilities:

- A handful of modules are loaded at process start time and only properly
annotated functions from these modules could be run
- A whitelist of allowed module names could be checked
- A whilelist of allowed module paths could be checked

Thanks

--
You received this bug notification because you are subscribed to the bug
report.
https://bugs.launchpad.net/bugs/1628348

Title:
  are module imports safe?

Status in oslo.privsep:
  New
Status in OpenStack Security Advisory:
  Incomplete

Bug description:
  This issue is being treated as a potential security risk under
  embargo. Please do not make any public mention of embargoed (private)
  security vulnerabilities before their coordinated publication by the
  OpenStack Vulnerability Management Team in the form of an official
  OpenStack Security Advisory. This includes discussion of the bug or
  associated fixes in public forums such as mailing lists, code review
  systems and bug trackers. Please also avoid private disclosure to
  other individuals not already approved for access to this information,
  and provide this same reminder to those who are made aware of the
  issue prior to publication. All discussion should remain confined to
  this private bug report, and any proposed fixes should be added to the
  bug as attachments.

  Hello, I'm conducting a very quick review of oslo.privsep as part of the
  Main In...

[Angus Lees (gus)]

So to respond more directly to your concerns in https://bugs.launchpad.net/oslo.privsep/+bug/1628348/comments/5

The unprivileged side cannot inject new code into the privileged side - only trigger modules that already exist on the privileged side to be imported.

The reason I didn't go with an explicit whitelist approach, is:-
 a) this requires the whitelist to be managed and
 b) I felt it added almost no additional security, since with monkeypatching it is easy to have one module affect other modules. Effectively the security boundary must be around the entire module path and not individual modules.

Regarding your original `__import__()` question, I'd be happy to add some sort of simple "is this regular word chars separated by dots" regex check on strings before attempting to import them if you feel that addresses your concerns. I am assuming at this point in the code that `__import__()` does its own input validation, and I don't recall now if I actually audited the relevant code (I think I did, but of course that doesn't mean the python function won't change in future).

[Seth Arnold (seth-arnold)]

I'm not good enough at Python to figure out a way to test the safety of __import__ here definitively. If you're confident that __import__() will respect the sys.modules path, then that's good enough for me.

Thanks

[Angus Lees (gus)]

Yes, it is my understanding that there is no way to (mis)use `__import__(single_arg)` to load something outside `sys.path`.

I was hoping that someone else would respond and give their analysis however, because if I state this then I'm just repeating the original assumptions I used to write the code in the first place, and we haven't actually gained anything from your audit, original question, nor this discussion :-/

[Jeremy Stanley (fungi)]

Since this report is based on ungrounded conjecture with no working exploit scenario, it makes more sense to continue this in public where we can get input from others in the community. If there are no objections, I'd like to end the current embargo and switch this report to public security early next week.

[Seth Arnold (seth-arnold)]

I'd forgotten this is still private; making it public makes sense to me.

Thanks

[Travis McPeak (travis-mcpeak)]

My only concern is "downstream" projects using this in an unsafe way. We may have unintended serious issues by making this public. Any easy way to trace how many are using this and how?

[Joshua Harlow (harlowja)]

Run https://github.com/openstack/oslo.tools/blob/master/clone_openstack.sh and grep for usage?

[Jeremy Stanley (fungi)]

Travis: I hope not too many (yet)? It's only existed for about a year, with the 1.0 release happening as recently as February. What would you recommend to mitigate such unsafe use? Additional documentation strongly warning against passing untrusted module names?

[Jeremy Stanley (fungi)]

Joshua: I think Travis was asking about identifying potential consumers of the library _outside_ the OpenStack ecosystem who may be using it in unanticipated (and unsafe) ways.

But anyway, the secrecy choice boils down to whether there's an actual bug to fix (or at least risky design that can be reimplemented without breaking API compatibility). If we just need to tell people what is or isn't a safe way to use this module then there is no benefit to keeping this report secret.

[Travis McPeak (travis-mcpeak)]

I'd like to consider both but our responsibility is to OpenStack. This actually seems like something Bandit might be useful to detect. If we open this up I can create a Bandit feature to warn about this.

[Jeremy Stanley (fungi)]

And as Joshua pointed out, finding OpenStack consumers of the library is pretty trivial. Shameless plug here for http://codesearch.openstack.org/?q=oslo.*privsep as my favorite.

[Angus Lees (gus)]

On Wed, 30 Nov 2016 at 12:41 Travis McPeak <email address hidden> wrote:

> My only concern is "downstream" projects using this in an unsafe way.
> We may have unintended serious issues by making this public. Any easy
> way to trace how many are using this and how?
>

I'm confused - why are you concerned who uses this library? If there's an
issue here, we should fix it.

 - Gus

[Angus Lees (gus)]

On Wed, 30 Nov 2016 at 13:14 Jeremy Stanley <email address hidden> wrote:

> Travis: I hope not too many (yet)? It's only existed for about a year,
> with the 1.0 release happening as recently as February. What would you
> recommend to mitigate such unsafe use? Additional documentation strongly
> warning against passing untrusted module names?
>

Still confused. Didn't we conclude that there is no issue with untrusted
module names?

privsep is meant to provide/enforce a privilege boundary, so if there is an
issue (and my reading of the above conversation is that no-one has found
any such issue), then writing "we strongly warn against exploiting this" is
not sufficient ;)

 - Gus

[Travis McPeak (travis-mcpeak)]

OK, I think I agree with Angus. I'm not seeing any way off the top of my head to do anything horrible with a malicious import value.

This is probably a hardening opportunity but I don't see a direct exploit path.

[Jeremy Stanley (fungi)]

Angus: I was basing Travis's hesitancy off your comment about "hoping that someone else would respond and give their analysis" (which I think we're far more likely to get if we open this bug up anyway). I agree it doesn't sound like there's anything dangerous about the chosen implementation, but maybe someone else will eventually point out something.

[Jeremy Stanley (fungi)]

I've switched this report from private security to public security now, as planned. I propose treating this as a class E report ( https://security.openstack.org/vmt-process.html#incident-report-taxonomy ) unless someone is able to convince the oslo.privsep developers there is something to be fixed here in which case it would be class D.

[Tristan Cacqueray (tristan-cacqueray)]

Agreed with comment #22, closing the ossa task now.

[Ben Nemec (bnemec)]

Setting to opinion as we're open to further discussion, but there are currently no actions to be taken on this.