Do only follow HTTP and HTTPS redirects #11032
Conversation
We do not want to follow redirects to other protocols since they might allow an adversary to bypass network restrictions. (i.e. a redirect to ftp:// might be used to access files of a FTP server which might be in a secure zone and not be reachable from the net but from the ownCloud server) See owncloud/core#11032 for the change in ownCloud
|
Upstream change to SabreDAV: sabre-io/http#14 |
|
Todo:
|
We do not want to follow redirects to other protocols since they might allow an adversary to bypass network restrictions. (i.e. a redirect to ftp:// might be used to access files of a FTP server which might be in a secure zone and not be reachable from the net but from the ownCloud server) See owncloud/core#11032 for the change in ownCloud Fix unit test Add workaround for HHVM
| 'notification' => 'progress' | ||
| ); | ||
| $ctx = stream_context_create( | ||
| $contextArray |
There was a problem hiding this comment.
Are you sure this is correct? The manual says resource stream_context_create ([ array $options [, array $params ]] ) and the previous patch had null as the first parameter.
LukasReschke
force-pushed
the
harden-redirect
branch
3 times, most recently
from
92c9d11 to
3fccd44
Compare
|
🚀 Test Passed. 🚀 |
|
@craigpg @MTRichards Upgrading to gold. Critical one - needs a fix. |
| $ctx = stream_context_create(null, array('notification' =>'progress')); | ||
| $contextArray = array( | ||
| 'http' => array( | ||
| 'follow_location' => false, // Do not follow the location since we can't limit the protocol |
There was a problem hiding this comment.
I'm worried about lost of functionality here, some websites might redirect their download links to the real file.
Not sure how often people use this feature though, and also how often such redirect cases might happen.
There was a problem hiding this comment.
I will take a look at how we can use getURLContent here - on it.
There was a problem hiding this comment.
I added a workaround around this PHP limitation, please review a870895 - thanks!
There was a problem hiding this comment.
To clarify: Using getURLContent is here not easily possible since we have no hard dependency on cURL and maintaining the callback would be major pain.
|
Yes. Change looks good to me. 👍 Please backport @LukasReschke |
LukasReschke
force-pushed
the
harden-redirect
branch
from
b917667 to
a870895
Compare
|
🚀 Test Passed. 🚀 |
|
Code looks good 👍 |
| if($headerArray !== false && array_key_exists('Location', $headerArray)) { | ||
| $locationHeader = $headerArray['Location']; | ||
| if(substr($locationHeader, 0, 8) === 'https://' || substr($locationHeader, 0, 7) === 'http://') { | ||
| return self::getFinalLocationOfURL($headerArray['Location']); |
There was a problem hiding this comment.
In general, calling a function has a certain overhead and as such recursion should be avoided when possible. Not that it matters here, but there also is a recursion limit of 100 in some environments. This can probably simply be replaced with a do while loop (untested):
$h = array('Location' => $location);
do
{
$h = get_headers($headers['Location'], 1);
}
while (isset($h['Location']) && (strpos($h['Location'], 'http://') === 0 || strpos($h['Location'], 'https://') === 0));
Note that isset($h['Location']) implies that $h is not false.
| $headerArray = $this->getHeaders($location, 1); | ||
|
|
||
| if($headerArray !== false && isset($headerArray['Location'])) { | ||
| while(true) { |
There was a problem hiding this comment.
It is easier to understand when you have the break condition in the while loop instead of using an endless loop with break. Hence I made it a do-while loop in my version.
|
Looks pretty much acceptable to me. |
|
Thanks @bantu - I'll address your other comments later. |
|
🚀 Test PASSed. 🚀 |
|
🚀 Test PASSed. 🚀 |
LukasReschke
force-pushed
the
harden-redirect
branch
2 times, most recently
from
26c5edf to
ed2fd68
Compare
|
Did the adjustments that I felt were reasonable. |
We do not want to follow redirects to other protocols since they might allow an adversary to bypass network restrictions. (i.e. a redirect to ftp:// might be used to access files of a FTP server which might be in a secure zone and not be reachable from the net but from the ownCloud server) Get final redirect manually using get_headers() Migrate to HTTPHelper class and add unit tests
LukasReschke
force-pushed
the
harden-redirect
branch
from
ed2fd68 to
6eeb905
Compare
|
👍 |
|
🚀 Test PASSed. 🚀 |
|
🚀 Test PASSed. 🚀 |
|
🚀 Test PASSed. 🚀 |
Do only follow HTTP and HTTPS redirects
|
Stable7: cb3bc5a Stable6 and stable5 PR is upcoming... |
|
Stable6: #11248 |
Backport of #11032 to stable5
|
And the crippled stable5 one: #11249 |
|
The inspection completed: No issues found |
We do not want to follow redirects to other protocols since they might allow an adversary to bypass network restrictions. (i.e. a redirect to ftp:// might be used to access files of a FTP server which might be in a secure zone and not be reachable from the net but from the ownCloud server)