Problem/Motivation

PHP files should not have execution permissions to avoid security problems.

Proposed resolution

Use the find command on the Drupal 8 HEAD to find if there is any PHP or static file (PNG,SVG,GIF,JPG) outside the core with execution permissions. The PHP files should have their execution permissions revoked and should remain as 644.

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Original report by @alexpott

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ParisLiakos’s picture

diff --git a/core/assets/vendor/jquery.ui/themes/base/images/ui-bg_flat_0_aaaaaa_40x100.png b/core/assets/vendor/jquery.ui/themes/base/images/ui-bg_flat_0_aaaaaa_40x100.png
...
diff --git a/core/assets/vendor/jquery.ui/themes/base/images/ui-bg_glass_55_fbf9ee_1x400.png b/core/assets/vendor/jquery.ui/themes/base/images/ui-bg_glass_55_fbf9ee_1x400.png
...
diff --git a/core/assets/vendor/jquery.ui/themes/base/images/ui-bg_glass_65_ffffff_1x400.png b/core/assets/vendor/jquery.ui/themes/base/images/ui-bg_glass_65_ffffff_1x400.png
...
diff --git a/core/assets/vendor/jquery.ui/themes/base/images/ui-bg_glass_75_dadada_1x400.png b/core/assets/vendor/jquery.ui/themes/base/images/ui-bg_glass_75_dadada_1x400.png
...
diff --git a/core/assets/vendor/jquery.ui/themes/base/images/ui-bg_glass_75_e6e6e6_1x400.png b/core/assets/vendor/jquery.ui/themes/base/images/ui-bg_glass_75_e6e6e6_1x400.png
...
diff --git a/core/assets/vendor/jquery.ui/themes/base/images/ui-bg_highlight-soft_75_cccccc_1x100.png b/core/assets/vendor/jquery.ui/themes/base/images/ui-bg_highlight-soft_75_cccccc_1x100.png
...
diff --git a/core/assets/vendor/jquery.ui/themes/base/images/ui-icons_222222_256x240.png b/core/assets/vendor/jquery.ui/themes/base/images/ui-icons_222222_256x240.png
...
diff --git a/core/assets/vendor/jquery.ui/themes/base/images/ui-icons_2e83ff_256x240.png b/core/assets/vendor/jquery.ui/themes/base/images/ui-icons_2e83ff_256x240.png
...
diff --git a/core/assets/vendor/jquery.ui/themes/base/images/ui-icons_454545_256x240.png b/core/assets/vendor/jquery.ui/themes/base/images/ui-icons_454545_256x240.png
...
diff --git a/core/assets/vendor/jquery.ui/themes/base/images/ui-icons_888888_256x240.png b/core/assets/vendor/jquery.ui/themes/base/images/ui-icons_888888_256x240.png
...
diff --git a/core/assets/vendor/jquery.ui/themes/base/images/ui-icons_cd0a0a_256x240.png b/core/assets/vendor/jquery.ui/themes/base/images/ui-icons_cd0a0a_256x240.png

we shouldnt touch vendor, right?

jibran’s picture

removed core/vendor/ and i think svgs can be executable.

Status: Needs review » Needs work

The last submitted patch, 2: file-permission-once-more.patch, failed testing.

Status: Needs work » Needs review
Samshel’s picture

I'm at Drupal con #LatinAmerica2015.

Checked the files permissions listed on comment #2's patch before applying it on head version of Drupal 8. All the files had Execute permissions.

Applied patch posted on comment #2. Files no longer have Execute permissions.

Patch worked correctly.

ParisLiakos’s picture

i think svgs can be executable.

nah. why would one need to execute an svg ;)

icampana’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +RTBC

I'm also at Drupal Con #LatinAmerica2015

I used the find command on Linux to find if the issue was still present on the latest Drupal 8 version, checked HEAD and still existed. In case you need to make that sort of revision again, we are excluding the core folder as pointed out by ParisLiakos, the command I used is:

find -executable -name '*.php' | grep -v "core/vendor"
The result is:

$ find  -executable -name '*.php' | grep -v "core/vendor"
./core/modules/system/src/Tests/Database/DeleteTruncateTest.php
./core/modules/ban/src/BanIpManagerInterface.php
./core/modules/block/src/BlockRepository.php
./core/modules/block/tests/src/Unit/BlockRepositoryTest.php

After applying the patch in number #2 and re-running the command the issue seems to be gone, so I think it's ready to be commited.

xavier.cabrera’s picture

Issue tags: +LatinAmerica2015

I am also in DrupalCon LatinAmerica2015 and I also confirmed the RTBC works.

alejandrovaras’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

We should be fixed the svgs that are not in vendor as well.

icampana’s picture

Issue tags: -RTBC
icampana’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.05 KB

I included regular files, so that even the static files (svg, gif, jpg, css, etc) don't get included if they have those permissions, the updated command is the following:

find ./ -regextype posix-awk -regex "(.*.php|.*.jpg|.*.png|.*.gif|.*.css|.*.svg*)" -executable -type f | grep -v "core/vendor"

You can change the permissions on the fly by using a call to xargs:
find ./ -regextype posix-awk -regex "(.*.php|.*.jpg|.*.png|.*.gif|.*.css|.*.svg*)" -executable -type f | grep -v "core/vendor" | xargs chmod a-x

I include the updated patch for review.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

yes, that seems to do the trick
Thanks!

jibran’s picture

nah. why would one need to execute an svg ;)

O ya my bad :) svg can have JS but it doesn't have to be executable.

  • webchick committed f1652eb on 8.0.x
    Issue #2401919 by icampana, alexpott, jibran, ParisLiakos, Samshel,...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Nice catch, thanks for the fixes, and for all the thorough research!

Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.