Fix an instance of xss #677

greggles · 2015-10-09T16:18:17Z

Hi,

There's an xss issue in https://github.com/minkphp/Mink/blob/master/driver-testsuite/web-fixtures/issue130.php

That issue was highlighted in an email to full disclosure at http://seclists.org/fulldisclosure/2015/Oct/42

The Drupal project attempts to solve this xss issue with an .htaccess file, but that's not very reliable. Another solution is to keep Mink code outside of the webroot, but that's not always possible on all servers.

It would be ideal, in my opinion, to just filter the text.

I did a quick grep to look for other similar problems and it seems there are some:

 ✗ grep -r echo . | grep \$_
./driver-testsuite/web-fixtures/advanced_form_post.php:echo str_replace('>', '', var_export($_POST, true)) . "\n";
./driver-testsuite/web-fixtures/advanced_form_post.php:    echo $_FILES['about']['name'] . "\n";
./driver-testsuite/web-fixtures/advanced_form_post.php:    echo file_get_contents($_FILES['about']['tmp_name']);
./driver-testsuite/web-fixtures/basic_form_post.php:    <h1>Anket for <?php echo $_POST['first_name'] ?></h1>
./driver-testsuite/web-fixtures/basic_form_post.php:    <span id="first">Firstname: <?php echo $_POST['first_name'] ?></span>
./driver-testsuite/web-fixtures/basic_form_post.php:    <span id="last">Lastname: <?php echo $_POST['last_name'] ?></span>
./driver-testsuite/web-fixtures/basic_get_form.php:        <?php echo isset($_GET['q']) && $_GET['q'] ? $_GET['q'] : 'No search query' ?>
./driver-testsuite/web-fixtures/cookie_page2.php:    Previous cookie: <?php echo isset($_COOKIE['srvr_cookie']) ? $_COOKIE['srvr_cookie'] : 'NO'; ?>
./driver-testsuite/web-fixtures/issue130.php:        echo '<strong>'.htmlspecialchars($_SERVER['HTTP_REFERER'], ENT_QUOTES, 'UTF-8');).'</strong>';
./driver-testsuite/web-fixtures/issue140.php:    echo $_COOKIE["tc"];
./driver-testsuite/web-fixtures/print_cookies.php:    <?php echo str_replace('>', '', var_export($_COOKIE, true)); ?>

I didn't look into fixing any more of them.

aik099 · 2015-10-09T18:03:41Z

The Mink test suite passes, but since file is located in driver test suite, then each driver needs to use it and pass as well.

stof · 2015-10-09T18:04:47Z

Yeah, the Mink testsuite itself does not rely on these files at all.

greggles · 2015-10-09T19:15:07Z

So...if I patched the instances that seem clearly like XSS would that be OK to merge or are there other people we should ping to get their feedback on these changes (at least advise them it's coming)?

greggles · 2015-10-12T14:45:29Z

This is some rather interesting code ;)

I'm pretty sure the htmlspecialchars on the file contents will break some uses of that form, but I have a hard time imagining the purpose of that form. When trying to sanitize synthetic code it seems natural that the outputs will be rather synthetic as well - as long as the behavior is well known it seems fine for any consuming code to modify itself to be more synthetic as well.

stof · 2015-10-12T15:01:34Z

@greggles try running the MinkSelenium2Driver testsuite using this modified version of the testsuite (start the webserver in the modified version rather than the current version of fixtures) to see whether the driver testsuite still works

stof · 2015-10-12T15:02:18Z

driver-testsuite/web-fixtures/advanced_form_post.php

@@ -13,11 +13,15 @@
 }

 $_POST['agreement'] = isset($_POST['agreement']) ? 'on' : 'off';
+foreach ($_POST as $key => $value) {
+	unset($_POST[$key]);
+	$_POST[htmlspecialchars($key, ENT_QUOTES, 'UTF-8')] = htmlspecialchars($value, ENT_QUOTES, 'UTF-8');


rather than replacing $_POST, I would rather use a separate array if we go this way

greggles · 2015-10-12T16:25:33Z

Sadly I'm not currently setup to run MinkSelenium2Driver. Not sure I'll be able to do it.

stof · 2015-10-12T16:41:22Z

@greggles this is not complex: you just need to have java and firefox installed locally, and to download the selenium jar (well, you also need to clone the driver repo and run composer install in it, but you should be setup for that already)

aik099 · 2015-10-12T17:04:20Z

@stof, maybe we should explain on each driver repo page how to setup local development in detail. For Selenium it would be:

going to http://www.seleniumhq.org/download/
downloading the server JAR file
running it locally using java -jar downloaded_file.jar
cloning repo
installing composer dependencies
creating phpunit.xml from phpunit.xml.dist
putting correct local/remote urls to location of cloned repo on web server

stof · 2015-10-12T17:55:25Z

@aik099 this would be a good idea

aik099 · 2015-10-20T18:47:45Z

I've created corresponding tasks in each driver repo so we don't forget to do this at some point.

stof · 2016-01-26T20:22:23Z

driver-testsuite/web-fixtures/advanced_form_post.php

-ksort($_POST);
-echo str_replace('>', '', var_export($_POST, true)) . "\n";
+foreach ($_POST as $key => $value) {
+	$post_for_printing[htmlspecialchars($key, ENT_QUOTES, 'UTF-8')] = htmlspecialchars($value, ENT_QUOTES, 'UTF-8');


this is broken in case the value is an array (for a multiple select for instance)

barryvdh · 2016-02-24T09:05:25Z

Another solution for most cases: exclude the testsuite from the archives; as they aren't needed for other projects, right? See #691

aik099 · 2016-02-24T09:09:39Z

This would only help if archives are used for install this package.

barryvdh · 2016-02-24T09:11:31Z

Yes, which is most cases. Not saying you shouldn't fix this issue also :)

aik099 · 2016-02-24T09:15:22Z

Absolutely (about issue needs to be fixed). I saw people using Mink for crawling purposes in which case tests might end up in production as well.

Unfortunately @greggles seems to have abandoned this PR (I see no changes requested during code review being made).

aik099 · 2016-02-24T09:20:39Z

As for implementation to avoid massive code duplication I'm proposing to create a function like htmlspecialchars_array that would recursively process any data that comes in (including arrays) so that we don't need to:

repeat code in every fixture file
pass 2nd+ parameters to htmlspecialchars call

Then we can make this function available across test suite (e.g. via Composer's autoload-dev functionality).

barryvdh · 2016-02-24T09:22:38Z

But the tests are autoload only in dev, so that wouldn't work properly right? Imho package specific tests are not public API and should not be relied upon.

aik099 · 2016-02-24T09:45:08Z

Ah, you're right, that autoload-dev works on that repo only and doesn't, when repo is included as dependency on other repo.

barryvdh · 2016-02-24T11:00:37Z

Okay, so removing the driver-testsuite isn't an option apparently, so I suggest it either gets moved to a seperate issue soonish, or the XSS gets fixed (also soonish ;))

aik099 · 2016-02-24T11:02:19Z

As I've mentioned before issue fixing is the right way, but PR author has abandoned it. If you're willing you can create your PR, that will replace this one.

barryvdh · 2016-02-24T11:05:19Z

I have zero knowledge of Mink. I just find it odd that a XSS issue is known for months, but no action is taken by the maintainers of this repo, even if the person who reported it abandoned the issue.

stof · 2016-02-24T11:11:49Z

@barryvdh the question is why you are installing vendors inside the webroot though.

barryvdh · 2016-02-24T11:13:03Z

I'm not, but you need to ask Drupal that ;)
Coming from this Drupal issue: https://www.drupal.org/node/2585165

greggles · 2016-02-24T16:14:14Z

I just tried to address the feedback on the PR. Scrutinizer has said 2 new style issues were introduced, but when I look at their website it seems the issues are in files that were not modified in this PR, so I'm not sure what needs to be fixed (probably I can't read that output).

I don't use Mink. I was trying to fix a bug that affected Drupal 8 (which I also don't currently use).

It doesn't feel great to hear comments that I abandoned this, but it is accurate that I stopped working on it. I'd say the reason I stopped working was 2 fold:

the barriers to contribution are quite high (requires me to download multiple pieces of software I don't generally need)
my motivation was fairly low (given I don't use the software)

Guides for new contributors would help to lower the barrier. Seems there's been no progress on those? If you could automatically run the tests you're asking me to run that would also lower the barrier.

greggles · 2016-02-24T16:19:32Z

BTW, Drupal includes it in the webroot because that's how most people who download Drupal expect to use the file they've downloaded: Extract a tarball it into the webroot and installation is done. I know that's considered by "modern" php folks to be an outdated philosophy, but it's the reality that most people running PHP sites expect.

There aren't (yet) any good guides about how to modify an installation to move vendor outside the webroot.

aik099 · 2016-02-24T16:24:07Z

It doesn't feel great to hear comments that I abandoned this, but it is accurate that I stopped working on it.

Sorry about that, but when you realized that you won't be able to continue with a PR development I recommend informing maintainers so that they can reassign it to another contributor. In this case you haven't replied at all (for 5 months) and maintainers (including me) had no idea if PR is abandoned or just delayed.

aik099 · 2016-02-24T16:32:06Z

I just tried to address the feedback on the PR. Scrutinizer has said 2 new style issues were introduced, but when I look at their website it seems the issues are in files that were not modified in this PR, so I'm not sure what needs to be fixed (probably I can't read that output).

That may happen due the fact, that used standard have changed since last inspection was run and therefore errors on non-changed files were raised. You can safely ignore those.

I don't use Mink. I was trying to fix a bug that affected Drupal 8 (which I also don't currently use).

I see.

the barriers to contribution are quite high (requires me to download multiple pieces of software I don't generally need)

my motivation was fairly low (given I don't use the software)

As a maintainer I'm a bit biased, but I do understand that it's hard to get new contributors on board due specifics of installing the software.

Guides for new contributors would help to lower the barrier. Seems there's been no progress on those?

Unfortunately. I guess we (as maintainers) are expecting for somebody else to change the guidelines as they see fit to make contribution easier.

If you could automatically run the tests you're asking me to run that would also lower the barrier.

I can. Please made necessary changes and at least make sure there are no PHP syntax errors.

greggles · 2016-02-24T16:43:01Z

I fixed the syntax error - sorry about that.

I don't currently plan to work more on this PR.

aik099 · 2016-02-27T11:19:10Z

Closing in favor of #694 , where I've applied all code from this PR and improved it further to reduce duplication in escaping code.

barryvdh · 2016-03-09T12:47:45Z

@aik099 Thanks for finishing this!

aik099 · 2016-03-09T14:00:44Z

You're welcome. At least now people having vendor folder inside DocumentRoot won't have any XSS-related problems.

Fix an instance of xss

cc2ec2a

Fix a few more instances of xss

48f6d69

stof reviewed Oct 12, 2015
View reviewed changes

Create a temp instead of modifying the global

b644fa3

stof reviewed Jan 26, 2016
View reviewed changes

barryvdh mentioned this pull request Feb 24, 2016

Export-ignore test files #691

Merged

aik099 added the response-needed label Feb 24, 2016

greggles added 2 commits February 24, 2016 09:01

Merge remote-tracking branch 'upstream/master'

3405973

Attempt to address feedback from @stof

d12098b

Syntax error

fddf0ca

aik099 removed the response-needed label Feb 24, 2016

aik099 mentioned this pull request Feb 27, 2016

Fixes XSS in the driver test suite #694

Merged

aik099 closed this Feb 27, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix an instance of xss #677

Fix an instance of xss #677

greggles commented Oct 9, 2015

aik099 commented Oct 9, 2015

stof commented Oct 9, 2015

greggles commented Oct 9, 2015

greggles commented Oct 12, 2015

stof commented Oct 12, 2015

stof Oct 12, 2015

greggles commented Oct 12, 2015

stof commented Oct 12, 2015

aik099 commented Oct 12, 2015

stof commented Oct 12, 2015

aik099 commented Oct 20, 2015

stof Jan 26, 2016

barryvdh commented Feb 24, 2016

aik099 commented Feb 24, 2016

barryvdh commented Feb 24, 2016

aik099 commented Feb 24, 2016

aik099 commented Feb 24, 2016

barryvdh commented Feb 24, 2016

aik099 commented Feb 24, 2016

barryvdh commented Feb 24, 2016

aik099 commented Feb 24, 2016

barryvdh commented Feb 24, 2016

stof commented Feb 24, 2016

barryvdh commented Feb 24, 2016

greggles commented Feb 24, 2016

greggles commented Feb 24, 2016

aik099 commented Feb 24, 2016

aik099 commented Feb 24, 2016

greggles commented Feb 24, 2016

aik099 commented Feb 27, 2016

barryvdh commented Mar 9, 2016

aik099 commented Mar 9, 2016

Fix an instance of xss #677

Fix an instance of xss #677

Conversation

greggles commented Oct 9, 2015

aik099 commented Oct 9, 2015

stof commented Oct 9, 2015

greggles commented Oct 9, 2015

greggles commented Oct 12, 2015

stof commented Oct 12, 2015

stof Oct 12, 2015

Choose a reason for hiding this comment

greggles commented Oct 12, 2015

stof commented Oct 12, 2015

aik099 commented Oct 12, 2015

stof commented Oct 12, 2015

aik099 commented Oct 20, 2015

stof Jan 26, 2016

Choose a reason for hiding this comment

barryvdh commented Feb 24, 2016

aik099 commented Feb 24, 2016

barryvdh commented Feb 24, 2016

aik099 commented Feb 24, 2016

aik099 commented Feb 24, 2016

barryvdh commented Feb 24, 2016

aik099 commented Feb 24, 2016

barryvdh commented Feb 24, 2016

aik099 commented Feb 24, 2016

barryvdh commented Feb 24, 2016

stof commented Feb 24, 2016

barryvdh commented Feb 24, 2016

greggles commented Feb 24, 2016

greggles commented Feb 24, 2016

aik099 commented Feb 24, 2016

aik099 commented Feb 24, 2016

greggles commented Feb 24, 2016

aik099 commented Feb 27, 2016

barryvdh commented Mar 9, 2016

aik099 commented Mar 9, 2016