php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #68089 NULL byte injection - cURL lib
Submitted: 2014-09-24 11:59 UTC Modified: 2014-10-14 17:41 UTC
From: research at g0blin dot co dot uk Assigned:
Status: Closed Package: *General Issues
PHP Version: 5.6.0 OS: Ubuntu 14.04 LTS
Private report: No CVE-ID:
 [2014-09-24 11:59 UTC] research at g0blin dot co dot uk
Description:
------------
It appears that when a user sets the value of the cURL option CURLOPT_URL, and uses the 'file://' schema, NULL bytes are not correctly stripped (as they are for http requests, for example). If user input is accepted as the CURLOPT_URL option, with the code expecting to add an extension on the end of the URL (i.e. .json) in order to force a download of a specific file type, this can be bypassed by supplying a NULL byte.

Also, if within the code, the user input is being checked for the schema by use of a strpos, this can also be subverted by use of a NULL byte.

This can result in disclosure of local files.

If this is accepted as a bug, I can request a CVE-ID to be put against this report, or you can provide me with a CVE-ID (once one has been obtained), for my records.

Test script:
---------------
<?php
    $url = "file:///etc/passwd%00http://google.com";
    if (strpos($url,"http://")!==FALSE) {
        $ch = curl_init();
        curl_setopt($ch, CURLOPT_URL, $url);
        curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
        echo curl_exec($ch);
    } else { echo "Error: provide a URL that uses the HTTP schema!"; }

Expected result:
----------------
The result expected would of been an error, stating an the provided path could not be found.

Actual result:
--------------
The actual result was the output of the file '/etc/passwd'.

Patches

fix-url-5.4 (last revision 2014-09-29 01:07 UTC) by stas@php.net)
fix-options-5.5 (last revision 2014-09-29 01:01 UTC) by stas@php.net)
fix-5.5 (last revision 2014-09-29 00:54 UTC) by stas@php.net)
bug68089.diff (last revision 2014-09-25 10:40 UTC) by johannes@php.net)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2014-09-24 12:04 UTC] research at g0blin dot co dot uk
While I understand that exploitation of this issue depends heavily on bad coding practices by the user, but as libcurl stops processing the path as soon as it finds a NULL byte (see https://github.com/bagder/curl/blob/master/lib/file.c, line 228), there is no need for these bytes to be accepted in path in the first place.
 [2014-09-25 10:40 UTC] johannes@php.net
The following patch has been added/updated:

Patch Name: bug68089.diff
Revision:   1411641611
URL:        https://bugs.php.net/patch-display.php?bug=68089&patch=bug68089.diff&revision=1411641611
 [2014-09-25 10:43 UTC] johannes@php.net
I think this is a curl issue. %00 is valid in URLs. We might add a mitigation for the file:// protocol, not sure about that.

There however is a related issue we certainly have to fix: curl_easy_setopt() is not binary safe, using
   $url = "file:///etc/passwd\0http://google.com";
shows that issue. This one is fixed in the attached patch.
 [2014-09-25 11:02 UTC] pajoye@php.net
One should actually use CURLOPT_PROTOCOLS to limit the protocol.

Arbitrary restricting valid URI (as Johannes) may be wrong.

We could enforce the given protocol only for anything remote, that would solve the local data access using "crafted" URI.
 [2014-09-25 11:48 UTC] research at g0blin dot co dot uk
Thanks for getting back to me. Will you be requesting a CVE-ID, or would you like me to do a write up and request a CVE-ID?
 [2014-09-25 12:04 UTC] pajoye@php.net
I discussed this issue with Bagder and for file: it could be done in curl. Patch:

https://gist.github.com/bagder/3b41a03b5feee6a59b0f

That will at least solve when one uses the next release. Or if distributions backport it.

Not sure we need a CVE for that.
 [2014-09-25 12:05 UTC] pajoye@php.net
@research at g0blin dot co dot uk

for the curl credit page/file, do you prefer your email or real name?
 [2014-09-25 12:10 UTC] pajoye@php.net
Do we really need a CVE here? Bagder or I thought it is not, would not mind tho' :) he is on Freenode's #curl 

Also we should follow up with curl as the fix will happen in curl and it is actually a curl issue.

We should also improve the documentation for the CURLOPT_PROTOCOLS usage to fix it  for current code.
 [2014-09-25 12:13 UTC] research at g0blin dot co dot uk
My email address will suffice, thanks.

With regards to a CVE-ID, I can see several other bugs like this that have CVE-IDs against them. As this could potentially be used to disclose arbitrary files by user input, do you not think it should have a CVE-ID?

Correct me if I'm wrong - I only started doing this kind of research recently after many years in development, but I have an example around somewhere that allows this exact issue to occur, due to a particular PHP script, which takes user input as a partial in a path, which is passed to curl_setopt. Again, bad programming practice, but it shouldn't of been possible IMO.

Happy to go with what you decide RE this. I'll likely do a small write-up in the near future, regardless of CVE assignment or not, as I found it a useful learning experience.

James H
 [2014-09-25 12:16 UTC] research at g0blin dot co dot uk
Point taken - this really is an issue with cURL, not PHP, so we do not need a CVE against PHP.
 [2014-09-25 12:46 UTC] johannes@php.net
Before this is overseen: There *is* a PHP issue, though it is not directly the reported one. Just put a \0 the in any option (like the URL instead of %00). This has to be fixed on our side. See attached patch (which is missing a test right now)
 [2014-09-25 13:41 UTC] research at g0blin dot co dot uk
I don't seem to be able to view the patch - which I'd like to, simply out of curiosity. Should I wait until this bug is set as public, or could you provide me with access to the patch diff?
 [2014-09-25 14:41 UTC] johannes@php.net
trivial patch:

diff --git a/ext/curl/interface.c b/ext/curl/interface.c
index d7cacf5..094c8ae 100644
--- a/ext/curl/interface.c
+++ b/ext/curl/interface.c
@@ -170,6 +170,11 @@ static int php_curl_option_str(php_curl *ch, zend_long option, const char *str,
 {
 	CURLcode error = CURLE_OK;
 
+	if (strlen(str) != len) {
+		php_error_docref(NULL TSRMLS_CC, E_WARNING, "Curl option %d contains invalid characters (\\0) ignoring");;
+		return FAILURE;
+	}
+
 #if LIBCURL_VERSION_NUM >= 0x071100
 	if (make_copy) {
 #endif
 [2014-09-25 14:41 UTC] johannes@php.net
Trivial and wrong - format string with %d but no parameter ... but shows the point
 [2014-09-29 00:44 UTC] stas@php.net
The patch applies for 5.5+, however I wonder if we need a fix for 5.4 too, at least for url ones?
 [2014-09-29 00:54 UTC] stas@php.net
The following patch has been added/updated:

Patch Name: fix-5.5
Revision:   1411952088
URL:        https://bugs.php.net/patch-display.php?bug=68089&patch=fix-5.5&revision=1411952088
 [2014-09-29 00:56 UTC] stas@php.net
The following patch has been added/updated:

Patch Name: fix-options-5.5
Revision:   1411952203
URL:        https://bugs.php.net/patch-display.php?bug=68089&patch=fix-options-5.5&revision=1411952203
 [2014-09-29 00:57 UTC] stas@php.net
The following patch has been added/updated:

Patch Name: fix-options-5.5
Revision:   1411952250
URL:        https://bugs.php.net/patch-display.php?bug=68089&patch=fix-options-5.5&revision=1411952250
 [2014-09-29 01:01 UTC] stas@php.net
The following patch has been added/updated:

Patch Name: fix-options-5.5
Revision:   1411952491
URL:        https://bugs.php.net/patch-display.php?bug=68089&patch=fix-options-5.5&revision=1411952491
 [2014-09-29 01:07 UTC] stas@php.net
The following patch has been added/updated:

Patch Name: fix-url-5.4
Revision:   1411952840
URL:        https://bugs.php.net/patch-display.php?bug=68089&patch=fix-url-5.4&revision=1411952840
 [2014-10-02 11:29 UTC] research at g0blin dot co dot uk
FYI, bagder released an update to the curl repo that prevents null byte escaping in file paths. I understand this does not fix the null byte escaping in php_curl_option_str, but just thought I'd put the change on this bug report.

https://github.com/bagder/curl/commit/53cbea22310f1509e98f5537ef3a83c6e600629f

Cheers
 [2014-10-14 17:42 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=ab0939e5e5449cba04b02fff3a5595f725bce0a0
Log: Fix bug #68089 - do not accept options with embedded \0
 [2014-10-14 17:42 UTC] stas@php.net
-Status: Open +Status: Closed
 [2014-10-14 17:44 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=d1e030db02f402efebfe2976482dd7e7ebe2956f
Log: Fix bug #68089 - do not accept options with embedded \0
 [2014-10-14 17:45 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=71b63fc701b16e326b6ee01369d746aed2c7643c
Log: Fix bug #68089 - do not accept options with embedded \0
 [2014-10-14 17:46 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=d1e030db02f402efebfe2976482dd7e7ebe2956f
Log: Fix bug #68089 - do not accept options with embedded \0
 [2014-10-14 17:53 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=81b67937e4561b23fa4e1cbbd6a7f1d444772bd6
Log: Fix bug #68089 - do not accept options with embedded \0
 [2014-10-14 17:53 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=71b63fc701b16e326b6ee01369d746aed2c7643c
Log: Fix bug #68089 - do not accept options with embedded \0
 [2014-10-14 17:53 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=d1e030db02f402efebfe2976482dd7e7ebe2956f
Log: Fix bug #68089 - do not accept options with embedded \0
 [2014-10-15 10:10 UTC] ab@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=81b67937e4561b23fa4e1cbbd6a7f1d444772bd6
Log: Fix bug #68089 - do not accept options with embedded \0
 [2014-10-15 10:11 UTC] ab@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=71b63fc701b16e326b6ee01369d746aed2c7643c
Log: Fix bug #68089 - do not accept options with embedded \0
 [2014-10-15 10:11 UTC] ab@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=d1e030db02f402efebfe2976482dd7e7ebe2956f
Log: Fix bug #68089 - do not accept options with embedded \0
 [2014-10-15 12:08 UTC] jpauli@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=cc1fe72bc99ae92d2e14275fd97f34960db24032
Log: Fix bug #68089 - do not accept options with embedded \0
 [2014-11-03 19:40 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=ab0939e5e5449cba04b02fff3a5595f725bce0a0
Log: Fix bug #68089 - do not accept options with embedded \0
 [2014-11-18 20:35 UTC] ab@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=ab0939e5e5449cba04b02fff3a5595f725bce0a0
Log: Fix bug #68089 - do not accept options with embedded \0
 [2016-07-20 11:40 UTC] davey@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=81b67937e4561b23fa4e1cbbd6a7f1d444772bd6
Log: Fix bug #68089 - do not accept options with embedded \0
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Fri Jun 23 06:01:39 2017 UTC