I know that in #334416, the temporary solution to prevent one site from being able to get the credentials for a different site was to move the credentials to environment variables set in the Apache VHost configuration, but this solution in some cases can be more insecure.

Consider that if the credentials are in the Apache environment, a single phpinfo page on the site will publicly devulge the credentials. Drupal's built-in phpinfo page at "admin/reports/status/php" is an example, though it is only accessible to users with "administer site configuration" permission.

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Is it a requirement or an option that the credentials are in the Apache configuration?

adrian’s picture

adrian commented

it's not that critical an issue in my mind.

a) the information is specified in the apache configuration so that it is impossible to leak the database credentials between sites. Site A with php access can no longer peek inside the settings.php file of Site B in a multi-site configuration.

b) displaying the information to site administrators does not mean they can make use of them. Unless they have PHP access they can not make a connection to the database, because the grant is incredibly paranoid.

c) I was unaware that page even existed, but it is simple enough to construct the virtualhost record for either the platform or site to explicitly disallow access to it.

anarcat’s picture

anarcat commented
Priority: Critical » Normal

I believe that if people can execute a phpinfo(), they can execute arbitrary code and therefore read whatever Drupal uses to store its database credentials: they can read settings.php, read the global $db_url, etc.

In short, once you have PHP access, you own the site. So while I share your concern about admin/reports/status/php, I think the issue is much larger than that, and not dependent on the environment technique we use right now, which resolves another much more severe security issue, which is that sites can read and write each other's database when Aegir is installed out of the box.

The tradeoff is clearly in favor of the mod_env at this point.

I would also think that the issue here is with Drupal itself: phpinfo() may yield confidential information, no matter what your setup is. It should be protected by a stricter permission..

drumm’s picture

drumm
he/him
Location NY, US
commented

What specifically would phpinfo() provide that is a security risk? Follow http://drupal.org/node/101494 if there is something notable. I don't think Drupal core will change this as phpinfo() does provide valuable debugging information.

Sounds like C in #2 can fix this issue.

And/or settings.php (or whatever uses the environment valriables) could unset them as they are used. Of course they would still be there if a bare phpinfo() file was present, but then we are back to already having arbitrary server access.

Anonymous’s picture

Anonymous (not verified) commented
Status: Active » Closed (won't fix)

The masked credentials prevent admin users on a site from divulging the creds of *another* site in multisite.

I don't consider the ability of an admin user to see his/her own site's database creds to be a security risk. If you consider it to be, don't give your user admin access. Access to the phpinfo() and seeing the creds is probably only one in a sea of many other risks you run by giving such admin access.

I'm going to be bold and close this as the security issue is dubious and dwarfed by the hole we fix with the masked credentials. And no-one has come up with anything better in nearly a year.

I don't even agree with blocking /admin/reports/status/php because that page could still be useful.

anarcat’s picture

anarcat commented
Status: Closed (won't fix) » Needs work

So there is a way to fix this, I believe.

anarcat’s picture

anarcat commented
Version: 6.x-0.4-alpha3 » 6.x-1.0-rc2
Status: Needs work » Fixed
Issue tags: +Security

And here it is: http://drupalcode.org/project/provision.git/commitdiff/6c27429f3bdf9e768...

Commit merged in stable branch too.

That one was painful. I actually had to go through php5's source code to figure out how apache_setenv() worked, to find the right combination.

omega8cc’s picture

omega8cc commented
Status: Fixed » Needs work

I think this should be here only for Apache inside some if(apache){write_it} as it will be never used in Nginx (even if it will not cause any issues it is useless for Nginx).

anarcat’s picture

anarcat commented
Status: Needs work » Fixed

Good catch - i'll just check for the existence of the function.

Too bad the template doesn't have access to the provision_db_cloaking settings (or does it?) as it makes the template more difficult to read...

Status: Fixed » Closed (fixed)
Issue tags: -Security

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

Issue tags: +Security

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.

  • Commit 442f945 on dev-ssl-ip-allocation-refactor, dev-1205458-move_sites_out_of_platforms, 7.x-3.x, dev-subdir-multiserver, 6.x-2.x-backports, dev-helmo-3.x by anarcat: 
    #730424 - only use apache_setenv if it exists, fixes nginx support
  • Commit df83d0f on 6.x-1.x, dev-ssl-ip-allocation-refactor, dev-1205458-move_sites_out_of_platforms, 7.x-3.x, dev-subdir-multiserver, 6.x-2.x-backports, dev-helmo-3.x by anarcat:
    #730424 - only use apache_setenv if it exists, fixes nginx support
  • Commit dde4a0c on debian, dev-ssl-ip-allocation-refactor, dev-1205458-move_sites_out_of_platforms, 7.x-3.x, dev-subdir-multiserver, 6.x-2.x-backports, dev-helmo-3.x by anarcat:
    #730424 - only use apache_setenv if it exists, fixes nginx support