php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Doc Bug #74634 read_exif_data() documented as not taking URLs but allows URLs
Submitted: 2017-05-22 23:55 UTC Modified: 2017-08-22 23:35 UTC
From: rskansing at gmail dot com Assigned: kalle (profile)
Status: Closed Package: EXIF related
PHP Version: 7.1.5 OS: Linux
Private report: No CVE-ID: None
 [2017-05-22 23:55 UTC] rskansing at gmail dot com
Description:
------------
# Description

exif_read_date doc page found at http://php.net/manual/en/function.exif-read-data.php says the filename parameter can not be a URL. There is no mention of ability to use wrappers.

This expectation that a URL can not be used leads to a potential SSRF and probably other vectors.

# Fix
Include information about wrappers supported in exif_*  docs.  




Test script:
---------------
<?php
read_exif_data('https://admin:admin@192.168.0.1:12345'); // $_POST['filename']);


Expected result:
----------------
PHP Warning:  exif_read_data(): Unable to open file


Actual result:
--------------
SSRF served inside the network.

# nc -l -p 12345 

GET / HTTP/1.0
Authorization: Basic YWRtaW46YWRtaW4=
Host: 127.0.0.1:12345
Connection: close


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2017-06-08 10:44 UTC] leigh@php.net
The documentation (http://php.net/exif_read_data) specifically states:

filename
The name of the image file being read. This cannot be an URL.

Looking at the history of exif.c it seems that exif_read_data has always (going back 15+ years) called exif_read_file, and exif_read_file has always used php_stream_open_wrapper

So which is correct, the documentation or the current behaviour?
 [2017-06-19 21:37 UTC] stas@php.net
I'm not sure why this specific function would not support streams where all others, like exif_imagetype or getimagesize, do.
 [2017-07-02 20:37 UTC] stas@php.net
-Summary: Unexpected behavior leads to blind SSRF +Summary: read_exif_data() documented as not taking URLs but allows URLs
 [2017-07-30 11:58 UTC] rskansing at gmail dot com
Any update on this issue? Seems straight forward to fix the docs, as it seems to be consistent behavior across the similar methods as mentioned.

But will it just be fixed silently?
 [2017-08-22 16:45 UTC] cmb@php.net
FTR: most functions support general stream wrappers if a filename
is expected, and I assume that is not explicitly documented for
many of those.

I have documented that for this particular function in
<http://svn.php.net/viewvc?view=revision&revision=342914>, but not
for others.
 [2017-08-22 17:12 UTC] kalle@php.net
-Status: Open +Status: Closed -Type: Security +Type: Documentation Problem -Assigned To: +Assigned To: cmb
 [2017-08-22 17:12 UTC] kalle@php.net
It was me who was not entirely good at documenting this behavior I implemented in 7.2, I blame my long time absence from working with the docs.

The exif extension supports streams like other functions that support streams in PHP do, in the same way. Any value passed should always be validated like anything else :)

Thanks Christoph for fixing this, didn't see it myself as it was wrongly labeled as a security issue.
 [2017-08-22 22:13 UTC] cmb@php.net
-Assigned To: cmb +Assigned To: kalle
 [2017-08-22 22:13 UTC] cmb@php.net
> It was me who was not entirely good at documenting this behavior
> I implemented in 7.2, […]

Actually, this ticket is not about the new feature that
`exif_read_data` accepts a stream, but rather about the
long-standing feature that the function accepts not only filenames
(in the strict sense), but also general URL wrappers (such as
http://example.com/foo?bar). The OP claims that the failure to
explicitly document that the function supports general URL
wrappers, can cause developers to write vulnerable code, which
*might* be the case (I cannot assess that, though).
 [2017-08-22 22:41 UTC] rskansing at gmail dot com
I am not sure why the security mark was removed on this issue? 

a developer could previously have used this method in a unsafe way due to the need for further validation/sanitization of input before passing it toe the method compared to what the doc previously said.
 [2017-08-22 22:54 UTC] rskansing at gmail dot com
>  The OP claims that the failure to
> explicitly document that the function supports general URL
> wrappers, can cause developers to write vulnerable code, which
> *might* be the case (I cannot assess that, though).

If it was only the lack of mentioning It wouldnt be a issue.

What makes it a minor issue is that it explicitly said the opposite.

"This cannot be an URL."
 [2017-08-22 23:17 UTC] kalle@php.net
-Package: Documentation problem +Package: EXIF related
 [2017-08-22 23:17 UTC] kalle@php.net
I removed the security flag as its a fairly common security issue, nothing secret about it here (issues marked as security are hidden on our bug tracker)
 [2017-08-22 23:35 UTC] rskansing at gmail dot com
Ah okay thanks. I was not sure how the security mark worked
 [2020-02-07 06:06 UTC] phpdocbot@php.net
Automatic comment on behalf of cmb
Revision: http://git.php.net/?p=doc/en.git;a=commit;h=f9958280aff969ad0644ccf8bdd1c3d2ab4c9e37
Log: Fix #74634: read_exif_data() documented as not taking URLs but allows URLs
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Tue Apr 23 20:01:29 2024 UTC