Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix file inclusion vulnerability #510

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ejcx
Copy link

@ejcx ejcx commented Aug 20, 2015

This file curl allows you to view any file on the system. For example, cloning this repository to the base PHP directory makes a vulnerable page look like this

http://example.com/simplecart-js/test/inc/get-raw-javascript.php?file=file:/etc/passwd

@ejcx
Copy link
Author

ejcx commented Aug 20, 2015

@alex-kovac 👍

@alex-kovac
Copy link
Contributor

feels it should go like ;) : @AlekseyKorzun 👍

@AlekseyKorzun
Copy link

Haha @alex-kovac

@AlekseyKorzun
Copy link

👍

@ejcx
Copy link
Author

ejcx commented Aug 21, 2015

Yes wrong person. Sorry @alex-kovac.... @AlekseyKorzun ... I wonder how long these people won't respond. This bug is in hundreds of sites.

@AlekseyKorzun
Copy link

@ejcx shoot them an email I think they are working on the next version 👯

@bcoles
Copy link

bcoles commented Jul 26, 2016

This issue affects the QUnit Test Suite bundled with simplecart-js, rather than simplecart-js itself, which may explain - but not excuse - the disinterest shown by the developers.

Concerned users can fix this issue by ensuring the test folder is not accessible via the web server.

Semantically, it's worth noting that this issue relates to file disclosure rather than file inclusion, as it does not lead directly to execution of local files.

Unfortunately, the test suite is also vulnerable to Server-Side Request Forgery (SSRF), which this patch PR does not protect against.

A more secure patch would be:

$urlParts = parse_url($url);
if ($urlParts['scheme'] !== "http" && $urlParts['scheme'] !== "https") exit;

This would limit the impact of SSRF attacks which make use of protocol schemes such as gopher, dict and nntp. However, it would not hinder HTTP(S) SSRF attacks.

Ideally, the test suite should ensure that the requested file is located on the same host on which it's executed, by verifying $urlParts['host'] and $urlParts['port'].

Alternatively, the README should make mention of the dangers associated with leaving the test suite in the web root directory.

Here's some more references regarding the dangers of SSRF.

@ejcx
Copy link
Author

ejcx commented Jul 26, 2016

Come on @bcoles . It's all semantics. I just want to see the bug fixed.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants