Mig5 was kind enough to send me some apache/nginx configs that mitigate SA-CORE-2018-002.

We should add these to our config.

CommentFileSizeAuthor
#15 2960825-15-mitigation-hosting-re-verify.patch1.01 KBhelmo
#15 2960825-mitigation-full.patch2.64 KBhelmo
#11 0001-Issue-2960825-Add-nginx-config-to-mitigate-SA-CORE-2.patch1.96 KBmemtkmcc
#9 2960825-mitigation-hosting-re-verify.patch866 byteshelmo
#6 2960825-mitigation-hosting_https.patch799 byteshelmo
#4 2960825-mitigation.patch1.44 KBJon Pugh
#3 2960825-mitigation.patch0 bytesJon Pugh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jon Pugh created an issue. See original summary.

Jon Pugh’s picture

Jon Pugh
he/him
Primary language English
Location Newburgh, NY
CreditAttribution: Jon Pugh commented
FileSize
2960825-mitigation.patch0 bytes
Jon Pugh’s picture

Jon Pugh
he/him
Primary language English
Location Newburgh, NY
CreditAttribution: Jon Pugh commented
FileSize
2960825-mitigation.patch1.44 KB

  • helmo committed 5b182c9 on 2960825-mitigation 
    Issue #2960825: Move mitigation
helmo’s picture

helmo CreditAttribution: helmo at Initfour websolutions commented
Status: Active » Needs review
FileSize
2960825-mitigation-hosting_https.patch799 bytes

After moving it to the vhost config this also needs a patch for hosting_https ...

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan at Colan Schwartz Consulting commented
Status: Needs review » Active

Looks reasonable, but I'm not sure of a good way to test. Would it be hard to add CI tests for both servers?

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan at Colan Schwartz Consulting commented
Status: Active » Needs review
helmo’s picture

helmo CreditAttribution: helmo at Initfour websolutions commented
FileSize
2960825-mitigation-hosting-re-verify.patch866 bytes

I'm open to suggestions for a CI test but don't have one ready.

Here's an extra patch for hosting module to re-verify all sites to update their vhost config.

memtkmcc’s picture

memtkmcc CreditAttribution: memtkmcc at Omega8.cc commented
Status: Needs review » Needs work

Nginx config has to be moved to the default config include, which is included in every vhost, since "set" directive is not allowed in this context:

Executing: sudo /etc/init.d/nginx reload
  nginx: [emerg] "set" directive is not allowed here in /etc/nginx/conf.d/aegir.conf:243
  nginx: configuration file /etc/nginx/nginx.conf test failed
memtkmcc’s picture

memtkmcc CreditAttribution: memtkmcc at Omega8.cc commented
Status: Needs work » Needs review
FileSize
0001-Issue-2960825-Add-nginx-config-to-mitigate-SA-CORE-2.patch1.96 KB

Here is a working patch for Nginx (attached).

memtkmcc’s picture

memtkmcc CreditAttribution: memtkmcc at Omega8.cc commented

Note that there is no need to re-verify sites hosted in Nginx based Aegir, since the updated (single) config is dynamically included in all vhosts.

helmo’s picture

helmo CreditAttribution: helmo at Initfour websolutions commented
FileSize
2960825-mitigation-full.patch2.64 KB

thanks @memtkmcc, I've committed that to the 2960825-mitigation branch.

For easy review, here's the full patch.

helmo’s picture

helmo CreditAttribution: helmo at Initfour websolutions commented

@memtkmcc so instead of the sites we should verify the server nodes?

helmo’s picture

helmo CreditAttribution: helmo at Initfour websolutions commented
FileSize
2960825-mitigation-full.patch2.64 KB
2960825-15-mitigation-hosting-re-verify.patch1.01 KB

I somehow managed to delete the patch from #13 .. here's that again.

And an updated re-verify patch to also do the server.

Jon Pugh’s picture

Jon Pugh
he/him
Primary language English
Location Newburgh, NY
CreditAttribution: Jon Pugh commented

Correct, Server verify saves the vhost include:

From http\Provision\Service\http\nginx.php:


  function init_server() {
    parent::init_server();
    $this->configs['server'][] = 'Provision_Config_Nginx_Server';
    $this->configs['server'][] = 'Provision_Config_Nginx_Inc_Server';
    $this->configs['site'][] = 'Provision_Config_Nginx_Site';

  • helmo committed 638b03d on 2960825-mitigation authored by memtkmcc 
    Issue #2960825 by memtkmcc: Add nginx config to mitigate SA-CORE-2018-...
  • helmo committed 7fdb80d on 2960825-mitigation
    Issue #2960825 by helmo: Cleanup moved code

As per https://www.drupal....
helmo’s picture

helmo CreditAttribution: helmo at Initfour websolutions commented
Status: Needs review » Fixed

This was released with 3.14.6 ... it would be nice to improve this (or verify that it already does) cover SA-CORE-2018-004. But lets create a new issue when there is someone available to work on it.

Status: Fixed » Closed (fixed)

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

Jon Pugh’s picture

Jon Pugh
he/him
Primary language English
Location Newburgh, NY
CreditAttribution: Jon Pugh commented
Assigned: Jon Pugh » helmo
Status: Closed (fixed) » Needs review

There's one more change still in this branch.

Can someone confirm this is not needed? https://git.drupalcode.org/project/provision/compare/7.x-3.x...2960825-m...

kienan’s picture

kienan CreditAttribution: kienan commented
Status: Needs review » Reviewed & tested by the community

The rules are still present in the nginx templates, so I think the trailing commit for apache should probably be merged in so the configurations are coherent between the two possible webservers. Tested w/ apache2.

  • helmo committed 3e18d79 on 7.x-3.x authored by Jon Pugh 
    Issue #2960825: Leftover Apache mitigation from...
helmo’s picture

helmo CreditAttribution: helmo at Initfour websolutions commented
Status: Reviewed & tested by the community » Fixed

I've now committed the changed Jon pointed at.

Status: Fixed » Closed (fixed)

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