One of the outstanding feature requests of site_audit is #2129415: Add security_review support - integration with Security Review. The submitter seems to have given up / lost interest, but I really would like to see this through and make site_audit more modular. I've been working on #2245239: Allow custom reports and checks to be added and actually have things working now. Going to post a patch in a moment, but let's discuss further!

Comments

FluxSauce’s picture

Status: Active » Needs review
FileSize
6.46 KB
PASSED: [[SimpleTest]]: [MySQL] 256 pass(es). View
FluxSauce’s picture

Issue summary: View changes
coltrane’s picture

@FluxSauce excellent, this looks pretty good! Some of the check results have specific array structures which aren't standardized so getResultFail() outputs some $values in the report as Array.

I'm working off your patch to see if I can leverage the check help functions for standardizing the structure so that Site Audit can use it consistently. My anonymous arrays are really becoming a pain in the ass here :(

FluxSauce’s picture

Thanks for your time again at DrupalCon Austin! http://cgit.drupalcode.org/site_audit/tree/README.md now includes comprehensive documentation on adding custom reports and checks, let me know if there's anything else I can do to help!

Jaesin’s picture

  $items['audit_security'] = array(
    'description' => dt('Render a Security Review with Site Audit.'),
    'aliases' => array('as'),

The "as" alias is already used for the audit_status command. May I suggest "asec".

It would also be nice to have the details text (from /admin/reports/security-review/help/security_review/...) available in the report.

Jaesin’s picture

I would also suggest limiting the number of files listed for "File system permissions" unless the --details option is passed.

codi’s picture

+1 for asec

Grimreaper’s picture

Status: Needs review » Needs work

Hello,

I want to test the patch but I have the following error Call to undefined function site_audit_common_options()

If I understand, site_audit is a drush extension ?

Is there a way to add something like

if (module_exists('site_audit')) {
  // The new drush command here.
}

but for drush. Is there a function like "module_exists" but for drush command ?

Even with site_audit downloaded in ~/.drush/site_audit, I can't use drush cc all and any other drush command so I commented the new drush command.

FluxSauce’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
PASSED: [[SimpleTest]]: [MySQL] 256 pass(es). View

Doing some Pacific Northwest Drupal Summit sprinting on this.

Since we last spoke, I've added a Security Report to site_audit, so I've updated the patch accordingly and added a couple changes based on this feedback. If you're going to test this out, please grab the latest version of site_audit first (latest release should work fine).

Really appreciate the feedback, thanks!

  • Jaesin, codi - implemented using that naming convention
  • Jaesin - Now only showing the list of writeable files if --detail is specified
  • Grimreaper - I should have checked to see if the command exists. With the restructure, it's just a hook_drush_command_alter() now, so that should sort it.

I'm particularly keen on moving this forward, especially in light of recent vulnerabilities. Let me know if you have any other questions or concerns!

Grimreaper’s picture

FileSize
6.18 KB
PASSED: [[SimpleTest]]: [MySQL] 256 pass(es). View

Hello,

I test the patch. You forgot to add the file security_review.site_audit.inc

So I use the one in patch in comment number 1. And I remove the class SiteAuditReportSecurity extends.

And It works fine !!!

Thank you very much.

I attached a new patch with the modifications.

FluxSauce’s picture

FileSize
1.43 KB
5.95 KB
PASSED: [[SimpleTest]]: [MySQL] 256 pass(es). View

Whoops, good catch #10 - thanks! There were a couple other improvements in there, I've re-rolled the patch - correctly this time and included an interdiff.

rocketeerbkw’s picture

Status: Needs review » Needs work

I used drush aa --skip=insights --html --detail --bootstrap > site-audit-report.html and got the latest code from both side audit and security review.

When running via Drupal I got "Drupal installation files and directories (except required) are not writable by the server." but running via Drush I got "Some files and directories in your install are writable by the server." Then it listed every file in my Drupal install. This must be because I'm running drush as my user account, which owns all the files.

For text formats check I got "Untrusted users are allowed to input dangerous HTML tags. -Array"

For drupal permissions I got "Untrusted roles have been granted administrative or trusted Drupal permissions. -Array"

When running via Drupal I got "There are Views that do not provide any access checks." but via drush there was no report for views.

FluxSauce’s picture

Hi rocketeerbkw, thanks for the bug report! Confirmed several; they can be fixed by executing security_review's internal help system for rendering. I'll figure out a clean way to make that happen. That will sort the Array and no report bugs.

For text formats check I got "Untrusted users are allowed to input dangerous HTML tags. -Array"

I can replicate this by going to /admin/config/content/formats/full_html and giving Anonymous users the ability to use full HTML.

For drupal permissions I got "Untrusted roles have been granted administrative or trusted Drupal permissions. -Array"

Replicated by visiting /admin/people/permissions and adding any permission with the label of "Warning: Give to trusted roles only; this permission has security implications." to the anonymous user.

When running via Drupal I got "Drupal installation files and directories (except required) are not writable by the server." but running via Drush I got "Some files and directories in your install are writable by the server." Then it listed every file in my Drupal install. This must be because I'm running drush as my user account, which owns all the files.

That is actually in security_review itself. If you use drush secrev --results you'll see the same result. I suspect that's a "works as designed", but I'd recommend opening a bug report on that separately.

heathdutton’s picture

@FluxSauce this is looking great! You probably have a better solution in the works, but for now I just convert the nested arrays to nested unordered lists (or indented strings). The results aren't the most beautiful, but works consistently:

Example of UL

heathdutton’s picture

FluxSauce’s picture

Assigned: Unassigned » FluxSauce
Status: Needs work » Needs review
FileSize
6.26 KB
PASSED: [[SimpleTest]]: [MySQL] 256 pass(es). View
1.73 KB
69.36 KB

I've got time again! Nice fix heathdutton, I've done a couple more tweaks and I'm pretty happy with it. It fully addresses all the outstanding functional issues now.

mparker17’s picture

I've tested this and it works well. My environment:

  • PHP 5.4.39
  • Drush 7.0.0
  • security_review-7.x-1.x at 3141b79
  • site_audit-7.x-1.x at 5b8f832
  • Run on a local copy of a Pantheon drops-7 site.

... I see the Menu Router, File system permissions, Text formats, Content, Error reporting, Private files, Allowed upload extensions, Drupal permissions, Executable PHP, Drupal base URL, and Temporary files checks, and the output from site_audit matches the output from the security_review UI on the same site.

***

There are a few coding standards errors and DrupalPractice warnings in the new file, security_review.site_audit.inc: you can see the output from these two commands in the attached text file, 2279283-16-coding-standards.txt. Note there are existing coding standards and DrupalPractice issues in the security_review.drush.inc file, BUT not in the code added in this patch (i.e.: this patch does not introduce problems in that file).

***

Once the coding standards issues are fixed, this is RTBC from me.

FluxSauce’s picture

FileSize
6.76 KB
PASSED: [[SimpleTest]]: [MySQL] 256 pass(es). View
2.55 KB

Hi Matt,

Thanks for the review, really appreciate it! Some of those coding standard concerns were legit and I've fixed them, but some are Drupal 8 I believe.

jon@Decker ~/projects/drupal-7.37 $ phpcs --standard=sites/all/modules/coder/coder_sniffer/Drupal --extensions='php,module,inc,install,test,profile,theme,js,css,info,txt' ~/projects/security_review/security_review.site_audit.inc
jon@Decker ~/projects/drupal-7.37 $ phpcs --version
PHP_CodeSniffer version 1.5.6 (stable) by Squiz (http://www.squiz.net)
jon@Decker ~/projects/drupal-7.37 $ grep 'version =' sites/all/modules/coder/coder.info
version = "7.x-2.5"
mparker17’s picture

Assigned: FluxSauce » Unassigned
Status: Needs review » Reviewed & tested by the community

Great!

Looks like the patch in #18 meets all Drupal 7 coding standards and best practices.

This is RTBC from me now.

AohRveTPV’s picture

If you want some superficial documentation standards feedback:


/**
 * Given a nested array, generate a unordered list, or text-only equivalent.
 *
 * @param $array
 * @param bool $html
 * @param int $indentation
 * @return string
 */

- Verb in summary sentence should be third-person singular present tense (i.e., "generates").
- Missing param type "array" for $array. It is obvious given the variable name but I think it is supposed to be indicated.
- Supposed to be blank line (beyond asterisk) between @param and @return. ("Some tags also trigger paragraph breaks: @param, @return, @see, @var.")
- Grammar: "a unordered" -> "an unordered"

See: https://www.drupal.org/coding-standards/docs

FluxSauce’s picture

FileSize
7.89 KB
PASSED: [[SimpleTest]]: [MySQL] 256 pass(es). View
1.48 KB

Thanks for the feedback, AohRveTPV; I had already addressed most of those, but I didn't get the method description - good catch! Updated, and I also added a paragraph to the README.txt. No functional changes.

FluxSauce’s picture

FileSize
7.89 KB
PASSED: [[SimpleTest]]: [MySQL] 256 pass(es). View
226 bytes

Caught a typo in the instructions.

dsnopek’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, @FluxSauce! This works great in my testing! Committed. :-)

  • dsnopek committed f235299 on 7.x-1.x authored by FluxSauce
    Issue #2279283 by FluxSauce, heathdutton, Grimreaper: Integrate with...
FluxSauce’s picture

Fantastic, thank you very much! Also, welcome to the project!

dsnopek’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Fixed » Patch (to be ported)

Thanks!

Actually, I just noticed that there's a D8 version of site_audit. We should probably port this to the 8.x branch of security_review! Updating issue status...

FluxSauce’s picture

You want help with that or you got it?

dsnopek’s picture

Help would be appreciated! :-) There's some other things in the queue I was planning to work on before circling back to digging into this.

fuzzy76’s picture

Neither 7.x-dev or a stable release since the commit to D7. Is a release with this far away?

dsnopek’s picture

It's been a really long time since the last release, and there's some good improvements in 7.x-1.x-dev... I'll discuss with the other maintainers!

jayroberts’s picture

I've rolled a version of @FluxSauce's patch against the 8.x-1.x branch of Security Review. This enables integration between Site Audit 8.x-2.x-dev
and Security Review 8.x-1.x-dev.