This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Improve the security model for logging listener()
Type: security Stage: needs patch
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, christian.heimes, ncoghlan, python-dev, vinay.sajip
Priority: normal Keywords: patch

Created on 2012-07-26 01:01 by ncoghlan, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
ast-lookup-eval.diff vinay.sajip, 2012-10-11 15:49 lookup_eval added to ast module review
Repositories containing patches
http://hg.python.org/sandbox/vsajip#fix15452
Messages (12)
msg166448 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-07-26 01:01
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()")
msg166468 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2012-07-26 08:55
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().
msg166484 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2012-07-26 14:29
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.
msg166495 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2012-07-26 18:07
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.
msg166714 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-07-29 04:31
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.
msg166797 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2012-07-29 17:39
> 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).
msg166845 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-07-30 01:22
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.
msg171807 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-10-02 14:56
New changeset 26c3d170fd56 by Vinay Sajip in branch 'default':
Issue #15452: Added verify option for logging configuration socket listener.
http://hg.python.org/cpython/rev/26c3d170fd56
msg171808 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2012-10-02 15:07
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.
msg213095 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-03-10 22:11
New changeset fe1804387687 by R David Murray in branch 'default':
whatsnew: logging.config.listen *verify* (#15452).
http://hg.python.org/cpython/rev/fe1804387687
msg275218 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-09-08 23:51
Can this ticket be closed?
msg275229 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2016-09-09 01:05
> 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).
History
Date User Action Args
2022-04-11 14:57:33adminsetgithub: 59657
2016-12-31 11:50:30vinay.sajipsetstatus: open -> closed
resolution: fixed
2016-09-09 01:05:34vinay.sajipsetstatus: pending -> open

messages: + msg275229
2016-09-08 23:51:01christian.heimessetstatus: open -> pending
nosy: + christian.heimes
messages: + msg275218

2014-03-10 22:11:20python-devsetmessages: + msg213095
2013-12-28 08:22:25vinay.sajipsetversions: + Python 3.5, - Python 3.4
2012-10-11 15:49:31vinay.sajipsetfiles: + ast-lookup-eval.diff
keywords: + patch
2012-10-11 15:48:57vinay.sajipsethgrepos: + hgrepo154
2012-10-02 15:07:47vinay.sajipsetmessages: + msg171808
2012-10-02 14:56:40python-devsetnosy: + python-dev
messages: + msg171807
2012-07-30 01:22:31ncoghlansetmessages: + msg166845
title: Eliminate the use of eval() in the logging config implementation -> Improve the security model for logging listener()
2012-07-29 17:39:37vinay.sajipsetmessages: + msg166797
2012-07-29 04:32:01ncoghlansetmessages: + msg166714
2012-07-26 18:07:37vinay.sajipsetmessages: + msg166495
2012-07-26 14:29:42vinay.sajipsetmessages: + msg166484
2012-07-26 08:55:42vinay.sajipsetmessages: + msg166468
2012-07-26 03:27:29Arfreversetnosy: + Arfrever
2012-07-26 01:01:34ncoghlancreate