On Wed, Apr 10, 2019 at 11:00 AM Ivan Pozdeev via Python-Dev < python-dev@python.org> wrote:
On 10.04.2019 7:30, Karthikeyan wrote:
Thanks Gregory. I think it's a good tradeoff to ensure this validation only for URLs of http scheme.
I also agree handling newline is little problematic over the years and the discussion over the level at which validation should occur also prolongs some of the patches. https://bugs.python.org/issue35906 is another similar case where splitlines is used but it's better to raise an error and the proposed fix could be used there too. Victor seemed to wrote a similar PR like linked one for other urllib functions only to fix similar attack in ftplib to reject newlines that was eventually fixed only in ftplib
* https://bugs.python.org/issue30713 * https://bugs.python.org/issue29606
Search also brings multiple issues with one duplicate over another that makes these attacks scattered over the tracker and some edge case missing. Slightly off topic, the last time I reported a cookie related issue where the policy can be overriden by third party library I was asked to fix it in stdlib itself since adding fixes to libraries causes maintenance burden to downstream libraries to keep up upstream. With urllib being a heavily used module across ecosystem it's good to have a fix landing in stdlib that secures downstream libraries encouraging users to upgrade Python too.
Validation should occur whenever user data crosses a trust boundary -- i.e. when the library starts to assume that an extracted chunk now contains something valid.
https://tools.ietf.org/html/rfc3986 defines valid syntax (incl. valid characters) for every part of a URL -- _of any scheme_ (FYI, \r\n are invalid everywhere and the test code for `data:' that Karthikeyan referred to is raw data to compare to rather than a part of a URL). It also obsoletes all the RFCs that the current code is written against.
AFAICS, urllib.split* fns (obsoleted as public in 3.8) are used by both urllib and urllib2 to parse URLs. They can be made to each validate the chunk that they split off. urlparse can validate the entire URL altogether.
Also, all modules ought to use the same code (urlparse looks like the best candidate) to parse URLs -- this will minimize the attack surface.
I think I can look into this later this week.
My PR as of last night cites that RFC and does validation in http.client while constructing the protocol request payload. Doing it within split functions was an initial hack that looked like it might work but didn't feel right as that isn't what people expect of those functions and that turned out to be the case as I tested things due to our mess of codepaths for opening URLs, but they all end with http.client so yay! I did *not* look at any of the async http client code paths. (legacy asyncore or new asyncio). If there is an issue there, those deserve to have their own bugs filed. As for third party PyPI libraries such as urllib3, they are on their own to fix bugs. If they happen to use a code path that a stdlib fix helps, good for them, but honestly they are much better off making and shipping their own update to avoid the bug. Users can get it much sooner as it's a mere pip install -U away rather than a python runtime upgrade.
Fixing this is going to break code that relies on the current code accepting invalid URLs. But the docs have never said that e.g. in urlopen, anything apart from a (valid) URL is accepted (in particular, this implies that the user is responsible for escaping stuff properly before passing it). So I would say that we are within our right here and whoever is relying on those quirks is and has always been on unsupported territory.
yep. even http.client.HTTPConnection.request names the function parameter "url" so anyone embedding whitespace newlines and http protocol strings within that is well outside of supported territory (as one example in our own test_xmlrpc was taking advantage of to test a malformed request). I suggest following up on https://bugs.python.org/issue30458 rather than in this thread. the thread did its job, it directed our eyeballs at the problems. :) -gps
Determining which of those quirks are exploitable and which are not to fix just the former is an incomparably larger, more error-prone and avoidable work. If anything, the history of the issue referenced to by previous posters clearly shows that this is too much to ask from the Python team.
I also see other undocumented behavior like accepting '<URL:<url>>' (also obsoleted as public in 3.8) which I would like to but it's of no harm.
--
Regards, Ivan
_______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/greg%40krypto.org