[issue25723] ConfigParser should never write broken configurations

[SpaceOne]

SpaceOne added the comment:

Of course both of you have reasonable arguments.
For compatibility with overridden SECTRE attributes it should not raise 
ValueError for characters like [ and ]. (too bad that SECTRE is a public 
attribute otherwise it could also be used to validate the name 
(SECTRE.match('[%s]')). What if somebody changed SECTRE to multiline? Then even 
rejecting '\n' would break compatibility.
But: How often does this happen? In open source projects it seems none. A 
nullege.com and google search exposed that no project does this.

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.
What's the goal of python here? Giving programmers nice utilities which have 
security considerations in its software design by default or giving everything 
up to the programmer which is forced to never trust the stdlib and always have 
to read the source code it uses?

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.
>From my experience it is good to restrict things from the beginning and make 
>them overrideable to be more relaxed if this is really needed.

And regarding issue20923: 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.
https://github.com/spaceone/cpython/commit/a0cdb85e4c7c4dd71a87b1f6c4d9d92ece2dde15

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25723] ConfigParser should never write broken configurations

[R. David Murray]

R. David Murray added the comment:

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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25723] ConfigParser should never write broken configurations

[Terry J. Reedy]

Terry J. Reedy added the comment:

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 #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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25723] ConfigParser should never write broken configurations

[SpaceOne]

SpaceOne added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25723] ConfigParser should never write broken configurations

[Terry J. Reedy]

Terry J. Reedy added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25723] ConfigParser should never write broken configurations

[R. David Murray]

R. David Murray added the comment:

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.

--
nosy: +r.david.murray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25723] ConfigParser should never write broken configurations

[Terry J. Reedy]

Terry J. Reedy added the comment:

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'*100 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.
  (?P[^]]+) # very permissive!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25723] ConfigParser should never write broken configurations

[STINNER Victor]

STINNER Victor added the comment:

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?

--
nosy: +haypo
versions:  -Python 2.7, Python 3.5

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25723] ConfigParser should never write broken configurations

[Terry J. Reedy]

Terry J. Reedy added the comment:

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.

--
nosy: +terry.reedy

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25723] ConfigParser should never write broken configurations

[R. David Murray]

Changes by R. David Murray :


--
nosy: +lukasz.langa
versions:  -Python 3.2, Python 3.3, Python 3.4

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25723] ConfigParser should never write broken configurations

[SpaceOne]

New submission from SpaceOne:

>>> 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:
http://bugs.python.org/issue23301
http://bugs.python.org/issue20923

--
components: Library (Lib)
messages: 255270
nosy: spaceone
priority: normal
severity: normal
status: open
title: ConfigParser should never write broken configurations
type: behavior
versions: Python 2.7, Python 3.2, Python 3.3, Python 3.4, Python 3.5, Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25723] ConfigParser should never write broken configurations

[SpaceOne]

SpaceOne added the comment:

I would have expected something like ValueError('A section must not contain any 
of ...') or at least that the characters are simply stripped.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com