Port Security Review to Drupal 8

Regarding the architecture and implementation of security checks, a limitation of the D7 design has been its reliance on anonymous arrays. While not required, I'd recommend that the design adapt to a more object-orientated architecture, but I leave the feasibility of that up to those carrying out the work. For inspiration see the checks of https://www.drupal.org/project/site_audit.

Noting GSOC project in summray.

There's now a 8.x-1.x branch for hosting the code so eventually it'll also live here on drupal.org

I use http://www.acronymfinder.com/ when an acronym is unknown to me. Also, @coltrane has updated the issue summary before with the link to the GSOC (Google Summer of Code) page and even Google uses GSOC in the URL and elsewhere to refer to it.

One of the only Drupal-specific acronyms you cannot find out about outside of the Drupal Community is 'IS' which stands for Issue Summary.

I've taken out the proposal link as it wasn't accessible to the public. Also updated the text considering the abbreviation issue mentioned above. Additional changes to this issue will be made throughout the project, and a bigger update will come this week to actualize the tasks and timeline.

Added detailed plan.

Just a little update for those who don't want to get into the code to know where I am:
I've finished implementing the new architecture and connecting it to the UI, the Base URL check is in the code as an example, it doesn't check your base URL setting yet, but serves as an example for how checks should be implemented. I've already tested the API by writing an other module that defined a check and it worked well.
So until the first push to drupal.org I have to:

• Implement logging functions
• Finish Base URL check
• Write tests
• Document the API

Made a checklist.

The new API documentation can be found here.

Drush interface port is ready.

Added "Drupal permissions" check to the list of checks (I've just ported it and didn't find it here).
Also removed the checklist, it might have been a little unprofessional.

Corrected "code is so far at", as there is already an 8.x-1.x branch with most of the code in it.

Updated blog link and check porting progress.

Ported Executable PHP check.

Added check PHP Access to the "not to be ported" list. Looks like the PHP module died and no one wants to resurrect it. Confirmation on my find would be nice.
Could make a check that tests whether the module is enabled tho.

Ported Text formats and Temporary files.

Ported Database errors, Failed logins and Views access.

Finished porting the checks. However still lacking some parts of the documentation (Drush usage, alterables), and the runCli() for File system permissions and Executable PHP, so still at Port existing checks.

Remaining tasks:

• Write a few more tests
• Add result icons on Run & Review page
• Clean up
• Wait for the community's review

Hi @banviktor.

Thanks for your excellent work on this!

I can install the module on `8.0.x` branch of drupal 8 on 18 July 2015
I can verify that output report from the GUI `/admin/reports/security-review` and `drush security-review` report the same information.

Anything else you would like tested?

~Geoff

Hi @banviktor.

I went through the tests that failed the security review for me:

1. Base URL in settings.php
2. Drupal installation files and directories (except required) are not writable by the server.
3. Untrusted users are not allowed to input dangerous HTML tags.

For (1) I set the $base_url in settings.php as instructed in the Details of the report. Well done.

For (2) on the Details report (/admin/reports/security-review/help/security_review/file_perms) it says:

In addition to inspecting existing directories, this test attempts to create and write to your file system. Look in your security_review module directory on the server for files named file_write_test.YYYYMMDDHHMMSS and for a file called IGNOREME.txt which gets a timestamp appended to it if it is writeable.

So, I checked the `security_review` directory and there was no `file_write_test.YYYYMMDDHHMMSS` that is good presumably the test could not write that file.
The IGNOREME.txt file was/is there; I think that is fine and what would be bad is if it were IGNOREME{timestamp}.txt, but I was not sure if that is correct. So, I think either make the language clear on the `details` (/admin/reports/security-review/help/security_review/file_perms) page, or split out the tests into separate line items on the security report so that the user knows what to fix and how to fix it. Does that make sense?

For (3)

1. I took away the `authenticated` users ability to use the wysiwyg field and that fixed it.
2. Visiting the path `/admin/reports/security-review/settings` and unchecking 'Authenticated user` as an `Untrusted role` also fixed it.
3. Taking away the <img> tag on /admin/config/content/formats/manage/basic_html also fixed it

I think these are all legitimate solutions and which is right for a user depends on who there `Authenticated users` are for their site. Maybe we could layout some circumstances in which one of the above solutions is correct, i.e. if your `Authenticated users` are internal staff and know not to put javascript in the image tag then we can trust them.

I'm not sure just thinking aloud.

~Geoff

Hi @serundeputy

Thank you for testing the module.

(2) IGNOREME.txt
The timestamp gets appended inside the file's contents (if it's writable). Changing the text would need a separate issue as this text was directly copied from the D7 version.

(3) The check result depends on which roles are set as untrusted on the Settings tab, so I think providing scenarios on this page is unnecessary.

Private files feature is not enabled.
That means it's fine. Only warnings and errors are considered to be vulnerabilites.

30s check runs.
Could you tell me about your test environment?
OS, filesystem? 30s check runs are normal on NTFS filesystems (sadly), but a progress bar would be a nice feature (the old version had one), so I will add that later.

Execution time:

geoff@geoff default (8.0.x) $ time drush security-review
Only safe extensions are allowed for uploaded files and images.                                                    [success]
Dangerous tags were not found in any submitted content (fields).                                                   [success]
Base URL is set in settings.php.                                                                                   [success]
Untrusted roles do not have administrative or trusted Drupal permissions.                                          [success]
Error reporting set to log only.                                                                                   [success]
PHP files in the Drupal files directory cannot be executed.                                                        [success]
Drupal installation files and directories (except required) are not writable by the server.                        [success]
No sensitive temporary files were found.                                                                           [success]
Untrusted users are not allowed to input dangerous HTML tags.                                                      [success]
Views are access controlled.                                                                                       [success]

real	0m33.528s
user	0m1.815s
sys	0m1.072s

Test Environment:

ProductName:	Mac OS X
ProductVersion:	10.10.4
BuildVersion:	14E46

File System:	Journaled HFS+

Thank you @serundeputy for the additional information

The slowness is most likely caused by determining which files are writable in your Drupal installation directory. If you click on Details next to "Drupal installation files and directories (except required) are not writable by the server." it probably takes about 30 seconds to show the results.
This slowness does not exist on ext filesystems (haven't tried all of them though). In the D7 version the difference wasn't noticeable because there were only ~1500 files while D8 consists of ~16500 files.

For more information on this, read my week 6 blog post here

great! thanks for the write up and all of your hard work!

Hi haven't looked through the complete module but here is one thing that I noticed :-

In your ChecklistController (may in other places as well), you are using static methods like '::csrfToken()', '::currentUser()'. I think it is better if you use DI instead of this.

Suggestion :-
You are making heavy use of SecurityReview class. So why don't you make it service ? I know most of its methods are static but still think in service way.

Hi @joshi.rohit100,

Thank you for your findings. It is also weird that I used ControllerBase for ToggleController but not for HelpController and ChecklistController. I will address these as soon as I can.

The lack of DI in controllers is already resolved on my GitHub. For services I was thinking Checklist, Security and SecurityReview as the Symfony Book says "As a rule, a PHP object is a service if it is used globally in your application.".

Checklist, Security and SecurityReview are services on this branch on my GitHub. I don't feel like I gained much by this modification. Any feedback is welcome.

Services and DI are part of the d.o code repository too.
Base URL check has been replaced by Trusted hosts which checks for $base_url and $settings['trusted_host_patterns']. See Issue #18 on GitHub.

Forgot to mention that some uninformative check results have been hidden, such as "Private files feature is not enabled". (@serundeputy)

Just tried it out. Works great! thanks.

~Geoff

Thanks all for your contributions :)

We are happy to contribute testing patch, quality assurance, documentation, and agile project management services if needed

Related pages

• Drupal 8.0.0 will be released on November 19, 2015
• News about "Porting modules and themes to Drupal 8 "
• "Drupal Module Upgrader" module. Which can do some of the port automatically for you, covering many API hooks.
• Documentation about "Converting 7.x modules to 8.x"

Updated title so it's easier to read. Using aggregator, drupal.org dashboard and search.

@banviktor: How would you feel about using plugins for the checks? It seems like it'd be a good use of plugins - we'd be able to eliminate hook_security_review_checks() and can move metadata about the checks (getNamespace(), getTitle(), getMachineTitle(), etc) into the plugin definition like other D8 plugins (ex. blocks, field formatters, etc).

I'd be happy to write the code (I'll make a new issue) but I just wanted to see what you thought first?

Hi @dsnopek,

Using plugins came up as an idea during the last few weeks of GSoC, so the time was limited then. I forgot to mention there are a few issues related to the port I did on my github issues page.
So I'm more than happy if this gets implemented :) Thank you @dsnopek !

@banviktor: Ah, cool! I didn't think to check the issues in the GitHub repo. Now that the module is fully working on D8 and the code is in the Drupal.org repo, how about closing this issue and moving the GitHub issues into this queue? Then we can continue collaborating on it in one place. I'd be happy to be the one to actually create the new issues here!

Yes, this issue doesn't really make sense now. Feel free to close this and create new ones :) Thank you!

Ok, closed! I made three issues on D.o for the four GitHub issues:

#2623148: Make security checks into plugins

#2623150: Remove SKIPPED and HIDE constants

#2623152: Merge Check::evaluate() and Check::evaluatePlain()