-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ConfigParser should never write broken configurations #69909
Comments
>>> from configparser import ConfigParser
>>> from io import StringIO
>>> from configparser import ConfigParser
>>> c = ConfigParser()
>>> c.add_section('foo]\nbar=baz\n[bar')
>>> fd = StringIO()
>>> c.write(fd)
>>> print(fd.getvalue())
[foo]
bar=baz
[bar] User input should always be validated. At least a ValueError should be raised if add_section() is called with a string containing anything like ']\x00\n[' or any other non-printable string. As this will always create a broken configuration or might lead to ini-injections. Otherwise ConfigParser cannot be used to write new config files without having deeper knowledge about the implementation. See also: |
I would have expected something like ValueError('A section must not contain any of ...') or at least that the characters are simply stripped. |
This strikes me as an overt bug. .add_section should add one new empty section, not a section with an item and a second section. Since a section name cannot contain \n, I would agree that passing a string containing \n should raise ValueError("Section names cannot contain newlines."). Since anything else without ']' is valid and since even that can be accommodated with a custom section name re, that is the only check that should be done. |
Terry: "Since anything else without ']' is valid (...)" A Python script can be used to generate a configuration read by another application. This application can more more strict on the configuration format than Python, so I would prefer to deny '\n', '[' and ']' characters in section names. I'm not sure that it's ok to modify Python < 3.6 since it can break applications relying on this ugly "feature". I propose to only modify Python 3.6. If you need strict ConfigParser, you can inherit from the class to override add_section() to add checks on the section name. @spaceone: Are you interested to work on a patch? |
I added also a rejection of '\r' and '\x00': It's only my opinion but I would prefer to reject all of these non printable characters: But as the _parse currently detects them as valid and your opinion is probably a very different one I will not add these characters. |
Newline (\n) and possibly \x00, if it necessarily causes an actual problem, are the only characters that we might reject by default. Rejecting anything else is unwarrented editorializing about what people 'should' use. As you said, people who want something stricter can replace .add_section. (This would be most useful in a group or corporate setting which might, for instance, want to severely limit the character set allowed.) In particular, I showed in bpo-20923 how to replace .SECTCRE to make "[Test[2]_foo]" yield "Test[2]_foo". The OP for that issue filed it after seeing an application that used such section names and they are not at all unreasonable. Python should be able to continue writing .ini files for that application as well as any others. Victor, your point that even a minimal change could break working code is a good one. It suggests to me that we should maybe do nothing. This issue is motivated by a theoretical principle "User input should always be validated" that is not a Python design principle (what is valid, who decides), not by actual problems in the field. |
Isn't is an actual problem in the field? We had a vulnerability in our code due to this as we only sanitized the config values and didn't recognized that add_section() does no validation of input. |
Can you explain how passing \n createda vulnerability (to who, doing what) that flagging \n would prevent? Your opening example (nicely presented, by the way) shows that passing \n allows one to do with one call what would otherwise take (in the case of the example) three calls. But the result is identical, so the shortcut seems harmless. |
The issue would be if the section name came from user input. Then an attacker could craft a section name that would result in inserting arbitrary names and values into the config file. |
We all know that blindly inserting external data into a database can be a bad idea. But raising ValueError if the data contains \n barely scratches the surface of a real defense. The external data should be checked before passing it to .add_section or as part of a derived method in a subclass. I already suggested the possibility of allowing only a restricted set of letter characters. Such a check comes after defending against the possibility of someone submitting 'a'*1000000 as, in this case, a section name. configparser is permissive by design, not by accident. The un-abbreviated verbose re for ConfigParser.SECTCRE say so. |
I view this as similar to the corresponding issue with email headers, where we fixed a similar security issue. The special danger of \n is that it allows you to create a *new* header, or in this case section, with an arbitrary value, possibly overriding an existing section and thus changing the behavior of the program in an exploitable way. This is *far* easier to exploit than the ability to introduce arbitrary data into the section name itself. Good security involves concentric rings of defense, and one should almost always be more secure by default when it has a small usability impact. In this case, there is no legitimate use for \n in a section name, so the only usability impact would be if some weird program out there was actually making use of this for some reason, against all reasonable logic :). Which is why we are suggesting changing it only in 3.6. \x00 is problematic (though somewhat less so) for the same reason, as many file readers will treat it as equivalent to end of line and allow a similar exploit. \r, \f, and \x1c-\x1e should also be blocked, but otherwise we should probably ignore non-printables for backward compatibility reasons (there we move further into the usability impact area). |
Of course both of you have reasonable arguments. Terry, I completely agree with your argument "that blindly inserting external input into a database is bad idea". But in the real world it happens that there are many applications out which doesn't validate what they pass into .add_section(). (Do you want me to give you a list of python projects which are either broken or vulnerable?). In my opinion this is dangerous, as well as not validating HTTP/Mail/MIME headers for such characters and so on. As I understand when I read the documentation is that config parser is loosely based on M$ INI files and as the name says it is for configuration files. Usually(!) configuration files are human readable files editable with an editor. Disallowing non-printable characters would have been the best option in the first release of config parser. And regarding bpo-20923: I think it would be a great feature to include the code change instead of changing the documentation. In my research about add_section() I found some projects which uses URI's as section name. As you know the WWW is evolving and actually http://[::1]/ is a valid URI nowadays. If this would be changed these implementations will not have to overwrite SECTRE in the future and they also won't run into that bug one day. I adapted my commit to only disallow \r \n and \x00. [ ] are allowed for customization of SECTRE. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: