bpo-29606: urllib throwing an exception on any URLs that contain one of '\r\n' for the FTP protocol. #1216

corona10 · 2017-04-20T18:52:50Z

bpo-29606: urllib throwing an exception on any URLs that contain one of '\r\n' for the FTP protocol.

mention-bot · 2017-04-20T18:52:54Z

@corona10, thanks for your PR! By analyzing the history of the files in this pull request, we identified @orsenthil, @ncoghlan and @Yhg1s to be potential reviewers.

corona10 · 2017-04-21T01:28:19Z

@orenmn
For reviewer,
If you have times, Can you take a look?

orenmn · 2017-04-21T05:40:30Z

I am sorry, but I am quite busy these days. Also, I am not familiar with the implementation of urllib.
anyway, I took a quick look, and these are my thoughts:

shouldn't we also raise an error for
'FTP://foo:bar%0d%[email protected]/file.png',
'fTp://foo:bar%0d%[email protected]/file.png',
etc.?
maybe you could use elif url[:i].lower() == 'ftp':?
ISTM it would be better to add two tests instead of one, e.g. check that both
'ftp://foo:bar%[email protected]/file.png' and
'ftp://foo:bar%[email protected]/file.png' raise an error.

corona10 · 2017-04-21T08:08:42Z

@orenmn
Thank you for your kind reviews.
Yes, You are right. We need to handling upper and mixing cases!
I also add more tests for this issues.

Please take a look.

One more thing, If this patch is landed, I think that backporting patches are might be needed for Python 3.x.
WDYT?

@ncoghlan
Can you take a look also?

orenmn · 2017-04-21T08:22:06Z

As I mentioned, I am quite busy these days, and not familiar with the relevant code, so I don't think I can help with this patch any more..
good luck :)

orsenthil · 2017-04-21T14:16:58Z

@corona10 - I will review this one.

corona10 · 2017-04-21T15:20:07Z

@orsenthil Great!

orsenthil · 2017-04-22T06:16:46Z

Sorry, closing was a mistake.

corona10 · 2017-04-22T07:08:12Z

@orsenthil I rebase it. PTAL

corona10 · 2017-04-26T00:34:24Z

@orsenthil If you have a time can you take a look?

orsenthil · 2017-04-26T00:37:04Z

@corona10 - I did, but give me some time. I am in travel, and I will definitely act on this soon.

ecbftw · 2017-04-30T17:58:42Z

Hi Everyone,

I'm the guy that worked out how to exploit these bugs with firewalls. I just figured I would chime in and provide some opinions.

For one, any URL parsing code that is not FTP-specific should not have input validation checks added. After all, other protocols might be able to handle CR/LF/NUL just fine through an appropriate encoding mechanism. I don't think explicit checks just after decoding are necessarily the best place for this (though I don't see a way to exploit the issue with the proposed patch). In my mind, the fact that CR/LF are an FTP-specific issue and should be dealt with right before FTP commands are dropped on the wire.

For the sake of having more complete test cases, please also consider adding %0d%0a to directory names in your test URLs. E.g.:

'fTp://example.net/foo%0d%0a/file.png',

Finally, I would encourage you to reject any NUL bytes in CWD commands, since these are never valid in any filesystem that I know of (UNIX or Windows).

HTH,
tim

ecbftw · 2017-05-01T17:14:16Z

I may need to revise my advice here... @giampaolo pointed out that RFC-2640 provides a mechanism for escaping newline sequences in FTP. It's not yet clear if this is the best approach (even though it may be the most correct approach), since many FTP servers may not support that RFC. To quote the RFC:

   When a <CR> character is encountered as part of a pathname it MUST be
   padded with a <NUL> character prior to sending the command. On
   receipt of a pathname containing a <CR><NUL> sequence the <NUL>
   character MUST be stripped away. This approach is described in the
   Telnet protocol [RFC854] on pages 11 and 12. For example, to store a
   pathname foo<CR><LF>boo.bar the pathname would become
   foo<CR><NUL><LF>boo.bar prior to sending the command STOR
   <SP>foo<CR><NUL><LF>boo.bar<CRLF>. Upon receipt of the altered
   pathname the <NUL> character following the <CR> would be stripped
   away to form the original pathname.

…one of '\n' for the FTP protocol

corona10 · 2017-06-17T01:58:14Z

@serhiy-storchaka
There were discussions about which combination of CR LF NUL should be deny. And I agree with the @ecbftw's opinion which denying very specific situations can not prepare for new types of injections about FTP. However, following to RFC-2640, it is not yet clear that all FTP server is now support it. Therefore, I refers to the patches of OpenJDK which deny only LF.

OpenJDK: http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/rev/81ddd5fc5a4e
Discussion: #1214

vadmium · 2017-06-17T02:35:40Z

@orsenthil, the discussion here is rather fragmented, so I may have missed something, but I still stand by my view that changing urlsplit is the wrong way: https://bugs.python.org/issue29606#msg292413.

vstinner · 2017-06-23T09:56:39Z

Lib/urllib/request.py

@@ -1502,6 +1502,9 @@ def ftp_open(self, req):
        host = req.host
        if not host:
            raise URLError('ftp error: no host given')
+        target_host = unquote(host)
+        if '\n' in target_host:
+            raise URLError("illegal ftp host")


I suggest to keep the "ftp error" prefix for the error message, so:

ftp error: illegal host

vstinner · 2017-06-23T09:57:07Z

Lib/urllib/request.py

@@ -1502,6 +1502,9 @@ def ftp_open(self, req):
        host = req.host
        if not host:
            raise URLError('ftp error: no host given')
+        target_host = unquote(host)
+        if '\n' in target_host:


Should we also blacklist \r?

vstinner · 2017-06-23T09:58:05Z

Lib/test/test_urllib.py

+            'FTP://foo:bar%0d%[email protected]/file.png',
+            'ftp://foo:bar%[email protected]/file.png',
+            'fTp://foo:bar%[email protected]/file.png',
+            'FTP://foo:bar%[email protected]/file.png'


I don't think that it's interesting to test different cases, I suggest to only test %0d%0a and %0a, so only keep two tests using ftp:// prefix.

vstinner

I tested this code in master, and it doesn't work: URLopener.open_ftp() is used, and this code is not patched to check for \n (%0a).

vstinner · 2017-06-23T10:09:30Z

Lib/test/test_urllib.py

+            with self.assertRaises(urllib.error.URLError) as e:
+                urlopen(host)
+            self.assertFalse(e.exception.filename)
+            self.assertTrue(e.exception.reason)


The test is invalid: it must check that the error contains "illegal host". In my case, the test only succeded because of another URLError.

For example, the test pass if the connection fails with <urlopen error ftp error ConnectionRefusedError(111, 'Connection refused')>

As I mentioned above, the tests should also include injection attempts on directories and filenames. After all, these portions of a URL illicit different FTP commands at different times.

corona10 · 2017-06-23T15:22:58Z

@ecbftw
Can you proceed this issue?
I think that you are the more right person to solve this issue.

ecbftw · 2017-06-23T15:26:47Z

I don't have time to develop a patch for this. I'm an extremely busy guy. Most security folks are. But I can lend my insight.

vstinner · 2017-07-21T10:30:18Z

Since @corona10 abandonned his PR, I wrote a new one: PR #2800.

the-knights-who-say-ni added the CLA signed label Apr 20, 2017

corona10 force-pushed the bpo-29606 branch 2 times, most recently from 004805b to b035bed Compare April 21, 2017 01:02

corona10 force-pushed the bpo-29606 branch from b035bed to 8ce8a3b Compare April 21, 2017 08:04

corona10 force-pushed the bpo-29606 branch from 8ce8a3b to 050d5f8 Compare April 21, 2017 08:23

corona10 mentioned this pull request Apr 21, 2017

bpo-30119: Fix ftplib to handle the ftp user's commands. #1214

Merged

orsenthil self-assigned this Apr 21, 2017

orsenthil closed this Apr 22, 2017

orsenthil reopened this Apr 22, 2017

corona10 force-pushed the bpo-29606 branch 2 times, most recently from 12439c6 to 6eb73f0 Compare April 22, 2017 07:07

corona10 force-pushed the bpo-29606 branch from 6eb73f0 to e23a8e1 Compare April 22, 2017 07:44

corona10 force-pushed the bpo-29606 branch 3 times, most recently from 399bfd0 to fa97d08 Compare April 26, 2017 07:12

orsenthil removed their assignment Jun 14, 2017

orsenthil requested a review from berkerpeksag June 14, 2017 20:15

orsenthil requested review from vadmium and serhiy-storchaka June 14, 2017 20:16

bpo-29606: urllib throwing an URL exception on any URLs that contain …

99c0dd2

…one of '\n' for the FTP protocol

corona10 force-pushed the bpo-29606 branch from fa97d08 to 99c0dd2 Compare June 16, 2017 14:38

vstinner reviewed Jun 23, 2017

View reviewed changes

vstinner requested changes Jun 23, 2017

View reviewed changes

vstinner reviewed Jun 23, 2017

View reviewed changes

corona10 closed this Jun 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-29606: urllib throwing an exception on any URLs that contain one of '\r\n' for the FTP protocol. #1216

bpo-29606: urllib throwing an exception on any URLs that contain one of '\r\n' for the FTP protocol. #1216

corona10 commented Apr 20, 2017

mention-bot commented Apr 20, 2017

corona10 commented Apr 21, 2017

orenmn commented Apr 21, 2017

corona10 commented Apr 21, 2017 •

edited

Loading

orenmn commented Apr 21, 2017

orsenthil commented Apr 21, 2017

corona10 commented Apr 21, 2017

orsenthil commented Apr 22, 2017

corona10 commented Apr 22, 2017

corona10 commented Apr 26, 2017

orsenthil commented Apr 26, 2017

ecbftw commented Apr 30, 2017

ecbftw commented May 1, 2017

corona10 commented Jun 17, 2017

vadmium commented Jun 17, 2017

vstinner Jun 23, 2017

vstinner Jun 23, 2017

vstinner Jun 23, 2017

vstinner left a comment

vstinner Jun 23, 2017

vstinner Jun 23, 2017

ecbftw Jun 23, 2017

corona10 commented Jun 23, 2017

ecbftw commented Jun 23, 2017

vstinner commented Jul 21, 2017

bpo-29606: urllib throwing an exception on any URLs that contain one of '\r\n' for the FTP protocol. #1216

bpo-29606: urllib throwing an exception on any URLs that contain one of '\r\n' for the FTP protocol. #1216

Conversation

corona10 commented Apr 20, 2017

mention-bot commented Apr 20, 2017

corona10 commented Apr 21, 2017

orenmn commented Apr 21, 2017

corona10 commented Apr 21, 2017 • edited Loading

orenmn commented Apr 21, 2017

orsenthil commented Apr 21, 2017

corona10 commented Apr 21, 2017

orsenthil commented Apr 22, 2017

corona10 commented Apr 22, 2017

corona10 commented Apr 26, 2017

orsenthil commented Apr 26, 2017

ecbftw commented Apr 30, 2017

ecbftw commented May 1, 2017

corona10 commented Jun 17, 2017

vadmium commented Jun 17, 2017

vstinner Jun 23, 2017

Choose a reason for hiding this comment

vstinner Jun 23, 2017

Choose a reason for hiding this comment

vstinner Jun 23, 2017

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

vstinner Jun 23, 2017

Choose a reason for hiding this comment

vstinner Jun 23, 2017

Choose a reason for hiding this comment

ecbftw Jun 23, 2017

Choose a reason for hiding this comment

corona10 commented Jun 23, 2017

ecbftw commented Jun 23, 2017

vstinner commented Jul 21, 2017

corona10 commented Apr 21, 2017 •

edited

Loading