As a result of #1914018: hook_requirements() for un-protected configuration directories I believe file_scan_directory() need to exclude the config_* folder in all cases. The nomask param need to be extended to remove all config_* folders..

$options += array(
  'nomask' => '/^CVS$/',
  'callback' => 0,
  'recurse' => TRUE,
  'key' => 'uri',
  'min_depth' => 0,
);
Files: 
CommentFileSizeAuthor
#21 core-config-exclude-doc-1914914-21.patch825 bytespurdy_nc
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,485 pass(es). View
#19 core-config-exclude-doc-1914914-19.patch826 bytespurdy_nc
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,296 pass(es). View
#17 core-config-exclude-doc-1914914-17.patch813 bytespurdy_nc
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,061 pass(es). View
#14 core-config-exclude-doc-1914914-14.patch764 bytespurdy_nc
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,755 pass(es). View
#9 1914914-documentation.patch740 bytesmeba
PASSED: [[SimpleTest]]: [MySQL] 58,513 pass(es). View

Comments

heyrocker’s picture

The config folders can be renamed to something else, so removing config_* folders would not suffice. It would also open up the possibility of stripping out folders that begin with 'config_' and are not related to core. A better option (if it is determined this is necessary at all) would be to add the directories from the $config_directories global.

I'm still not convinced this is necessary though. There may be legitimate reasons to list this directory (for instance when building a deployment tool.) I think it is a better option to leave this to contributed module authors to manage.

heyrocker’s picture

Issue tags: +Configuration system
greggles’s picture

chx’s picture

Title: Exclude config_* in file_scan_directory() to prevent security breach » Document file_scan_directory() best practices
Component: file system » documentation
Priority: Major » Normal
Issue tags: -Security improvements, -needs security review +Novice

We need to amend the doxygen to something like "scanning the whole public files directory with file_scan_directory can lead to severe perfomance problems. if you must, use your own subidrectory but it's better not use this function on user uploaded content at all -- rather use the file managed API to keep a record of your files". That's all is needed here and that's a useful addition anyways.

heyrocker’s picture

Retagging because I'd still like some more opinions.

hass’s picture

Why have you renamed the title? Please read the issue where I came from.

If admin can rename the config_* folder we have more troubles than I expected as no module maintainer can solve this issue on it's own to keep the config dir secured and hidden from their users prying eyes. Just think about ICME where everyone is able to read all config files.

This config files in the file system become more and more troubles. Core need to provide something to maintainers of file management modules.

heyrocker’s picture

Did you actually read what I said above? Any file management module can get the current config directory from the $config_directories global and hide it themselves if they feel it is necessary for their use case. I believe this is the appropriate measure. chx is on the security team and he agrees. I have asked other members of the security team to comment if they feel there is a problem with this.

jhodgdon’s picture

So... What are we needing to add to the documentation? I'm confused.

meba’s picture

Status: Active » Needs review
FileSize
740 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,513 pass(es). View

Something like this

jhodgdon’s picture

Status: Needs review » Needs work

Um. I think it would be fairly obvious that scanning the entire public files directory would take some time? Why do we need to say this?

Also, if you want to say to use the Managed Files API, it needs to be a link or have a @see, or better yet some explanation of why this is better or what it buys you, or which function to use to scan for files using that API.

And there are problems with punctuation and capitalization in the last line. Should be "module-related files" and "Managed Files API".

hass’s picture

Title: Document file_scan_directory() best practices » file_scan_directory() does not exclude config folder by default
Component: documentation » file system
Issue summary: View changes
jhodgdon’s picture

Component: file system » documentation

Normally, documentation changes are left in the documentation component.

purdy_nc’s picture

Assigned: Unassigned » purdy_nc
purdy_nc’s picture

FileSize
764 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,755 pass(es). View

Added documentation to file_scan_directory() to remind people that config_ directories could be included under the right conditions. I also noted how they could make sure their code is more secure by using the right parameters.

purdy_nc’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks!

This patch has a few typographical issues:

+ * scanned. This will not exclude config_ directories by default, so secure
+ * usage of this API can be assured by either limiting the initial base
+ * directory to a specific folder, limiting the recursion or adding
+ * "^config_" to the nomask option.

a) assured ==> ensured

b) needs a comma after "recursion" in the 3rd line (we use serial commas in the Drupal project by convention)... but see (c) below.

c) And also, as noted above, this is not really accurate. Limiting recursion will not actually help with security, but only with the performance issues noted above. And adding "config" specifically is not going to work for someone who has used a different config directory. So I think we should take out the recursion part, and not talk about "config_" specifically, instead say something more generic about "configuration directories" and "use the nomask option to skip configuration directories".

purdy_nc’s picture

Status: Needs work » Needs review
FileSize
813 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,061 pass(es). View

Awesome, thanks for the great feedback. I'm trying again with another patch (attached).

jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good! You need a comma before "and" here:

+ * information. Also, the base directory, recurse and min_depth options can 
+ * affect performance by limiting how much of the filesystem has to be
+ * traversed.

And... Maybe it would be clearer to say something like "you can use the ... options to improve performance..."?

purdy_nc’s picture

Status: Needs work » Needs review
FileSize
826 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,296 pass(es). View

I'm getting there! :)

Ok, another patch file attached. Thanks for the feedback/review! :)

jhodgdon’s picture

Status: Needs review » Needs work

Thanks -- text looks good!

There is an extra space at the end of one of the lines though. Most programming editors can be configured to remove or highlight spaces at the ends of lines -- if you are going to continue working on Drupal patches (and I hope you will!), you might want to look for that setting.

purdy_nc’s picture

Status: Needs work » Needs review
FileSize
825 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,485 pass(es). View

D'oh! Ok, removed. Thanks for catching that.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -needs security review

Thanks!

jhodgdon’s picture

Thanks again everyone! Committed to 8.x.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

  • Commit 7fdb267 on 8.x by jhodgdon:
    Issue #1914914 by purdy_nc, meba, chx, heyrocker: Add to docs of...

Status: Fixed » Closed (fixed)

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