Improve the security model for logging listener()

[ncoghlan]

BPO	15452
Nosy	@vsajip, @ncoghlan, @tiran
Files	ast-lookup-eval.diff: lookup_eval added to ast module

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-12-31.11:50:30.705>
created_at = <Date 2012-07-26.01:01:34.931>
labels = ['type-security', 'library']
title = 'Improve the security model for logging listener()'
updated_at = <Date 2016-12-31.11:50:30.704>
user = 'https://github.com/ncoghlan'

bugs.python.org fields:

activity = <Date 2016-12-31.11:50:30.704>
actor = 'vinay.sajip'
assignee = 'none'
closed = True
closed_date = <Date 2016-12-31.11:50:30.705>
closer = 'vinay.sajip'
components = ['Library (Lib)']
creation = <Date 2012-07-26.01:01:34.931>
creator = 'ncoghlan'
dependencies = []
files = ['27531']
hgrepos = ['154']
issue_num = 15452
keywords = ['patch']
message_count = 12.0
messages = ['166448', '166468', '166484', '166495', '166714', '166797', '166845', '171807', '171808', '213095', '275218', '275229']
nosy_count = 5.0
nosy_names = ['vinay.sajip', 'ncoghlan', 'christian.heimes', 'Arfrever', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'needs patch'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue15452'
versions = ['Python 3.5']

[ncoghlan]

The current implementation of PEP-391 relies on eval, which is substantially more permissive than the expected syntax described in the spec. This means the listen() feature provides an attack vector for injection of untrusted code.

While the documentation has been updated with a cautionary note to this effect, longer term, the use of eval() should be replaced with:

1. ast.literal_eval()
2. refactoring the str.format attribute and item lookup code into something suitable for reuse in other contexts (perhaps exposed via the ast module as "ast.lookup_eval()")

[vsajip]

It's not actually the PEP-391 implementation - dictConfig() - that uses eval(). Rather, it's the older fileConfig() API which was part of the original logging package when added to Python 2.3. The use of eval() by fileConfig() was documented at that time, IIRC.

I have no problem in principle with updating fileConfig() - which uses eval() in just one private function - to use ast.literal_eval(), but it may break existing, innocuous code which can't be handled by ast.literal_eval().

[vsajip]

Initial evaluation indicates that ast.literal_eval doesn't cut the mustard: it doesn't do any name lookups, so you can't for example successfully evaluate something like 'handlers.WatchedFileHandler' or even 'FileHandler'.

However, a limited evaluator which goes further than ast.literal_eval will probably work. One such is shown in this Gist:

https://gist.github.com/3182304

It supports a reasonable subset of Python expressions and also could be useful in other contexts than logging configuration.

[vsajip]

Having reflected on this further, ISTM that limiting the scope of evaluation is not the correct answer. For example, a malicious user could still send a bogus configuration which, for example, just turns the verbosity of all loggers off, or configures a huge number of bogus loggers. This would certainly be allowed even by a limited-evaluation scheme if a user legitimately wanted to do so; but if a malicious user sends the exact same “legal” configuration, it is still a security exploit because the consequences may be undesirable to the victim.

But how is the listener to know whether or not the configuration is coming from a legitimate source (a client process controlled by the same user who is running the process which uses listen()) or a malicious user (a client process controlled by some other user)? The simplest answer would appear to be a shared secret: When listen() is called, it is passed a text passphrase, which is also known to legitimate clients. When handling a configuration request via the socket, the configuration is checked to see if it contains the passphrase. If it does, the request is processed; otherwise, it is ignored.

In the fileConfig() input data, the passphrase could be provided via a passphrase=secret entry in the [default] section. In the dictConfig() input data, the passphrase could be provided against the passphrase key in the dict which is passed to dictConfig(). The checking would be done in the request handler code before calling fileConfig() or dictConfig(). If the passphrase argument to the listen() call is None (the default, preserving the current behaviour) no passphrase checking would be done.

[ncoghlan]

I know ast.literal_eval() isn't enough - that's why I suggested the addition of ast.lookup_eval() in the opening post.

As far as your other suggestion goes, don't reinvent crypto badly - if you want to provide authentication support in listener(), provide a hook that allows the application to decide whether or not to accept the configuration before it gets applied. They can then choose there own authentication mechanism based on their own needs, and handle any appropriate security updates. Some will choose a simple shared secret, some may choose to require a cryptographic signature, some may pass the entire payload in an encrypted form.

However, this isn't an either/or situation - we can, and should, do both (i.e. provide a hook that allows the application to preauthenticate the configuration before it is applied, as well as replacing the use of eval() with something more limited like str.format lookup syntax). The right security mindset is to set up defence in depth, not trust one particular layer of defence to handle all possible varieties of attack.

[vsajip]

As far as your other suggestion goes, don't reinvent crypto badly -
if you want to provide authentication support in listener(), provide a
hook that allows the application to decide whether or not to accept
the configuration before it gets applied.

Well, that's fine. My earlier suggestion keeps the API change to a minimum, but I suppose there's no real need to be so minimal.

I suppose the basic approach would be to pass to listen() an optional verify callable (defaulting to None) which, if provided, would be called with the bytes received over the socket. That allows for e.g. signed or encrypted data. The value returned from the verify() call would be processed as the current received value is (allowing the verifier to transform the input, e.g. by decrypting it).

[ncoghlan]

Yep, that's exactly the kind of hook I had in mind. That way the user can decide for themselves what level of scrutiny they want to apply.

[python-dev]

New changeset 26c3d170fd56 by Vinay Sajip in branch 'default':
Issue bpo-15452: Added verify option for logging configuration socket listener.
http://hg.python.org/cpython/rev/26c3d170fd56

[vsajip]

I've updated logging as discussed in this issue, except for the removal of the two calls to eval() in logging.config. I propose to resolve that as follows:

1. Add the Evaluator implemented in the Gist I linked to to ast.py.

2. Expose a function 'ast.lookup_eval(source, context, allow_import)' which basically just does a

   return Evaluator(context, allow_import).evaluate(source, '<lookup_eval>')

3. Add docs and tests to ast.rst and test_ast.py.

4. Update logging.config to call ast.lookup_eval() instead of eval().

Please comment if you see any problems with this, otherwise I will go
ahead and implement this change within the next week or so.

[python-dev]

New changeset fe1804387687 by R David Murray in branch 'default':
whatsnew: logging.config.listen *verify* (bpo-15452).
http://hg.python.org/cpython/rev/fe1804387687

[tiran]

Can this ticket be closed?

[vsajip]

Can this ticket be closed?

I suppose so - I didn't implement the addition of lookup_eval() to the ast module as I thought it might be a slight overkill. Given that the calls to eval() from fileConfig() have been there from when logging was added to the stdlib, and as this ticket has been quiet since 2012, I suppose there's no real concern about the eval() being a security issue. If there is such a concern, then my proposal to add lookup_eval() to the ast module should be considered (it didn't get any review comments when I proposed it).