Problem

  • Currently the configuration management has a lot of code related to signing configuration files in order to verify they have not been changed since written by the system. This code is potentially unnecessary and increases the complexity of the system.

Goal

  • Remove file signing to make the system architecture less complicated and reduce points of failure.

Details

  • When the file-based configuration system was originally proposed, files were going to be saved within the files directory at a predictable path. In an attempt to protect against files being overwritten behind the system's back as well as taking advantage of "defense in breadth". Since then, we have moved to having the files saved in a non-predictable path - a randomly generated directory name that is a sha-256 hash of various system information. Given that the path to your configuration is now completely hidden, it was brought up by various people that the file signing might not be necessary anymore.

Proposed resolution

  1. Remove all file signing and checking functionality.

Notes

  • I am most interested in the security implications of this change as this is not my area of expertise.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Issue tags: +Needs security review

Tagging for security review.

bojanz’s picture

I think removing the signing would be a good idea.

dawehner’s picture

This would indeed help a lot to simply some of the SignedFileStorage code. It would even make sense to rename the class to FileStorage, just because it's simply not signed anymore.

There are some questions which are opened here:

* What should be done with ConfigFileStorageSignatureException, there is already a ConfigFileStorageException so there is no need for it anymore? It's a bit confusing, because i can't find any usage of this Exception
* Can the verify method be dropped, i don't see any usage therefore anymore

gdd’s picture

All this is correct. The class would be renamed, the exceptions would go away, the verify method would be dropped.

dawehner’s picture

Status: Active » Needs review
FileSize
6.73 KB

Here is a first version, and at least the config tests are running fine. This patch was created against the 8.x-file-config branch.

Status: Needs review » Needs work

The last submitted patch, 1444620-remove-signed.patch, failed testing.

gdd’s picture

I don't think we are ready for a patch yet, given that there has been no serious discussion of the security implications of this at all yet.

yched’s picture

Regardless of security concerns, I thought maybe the signatures could help on configuration reload - they provide an easy way to tell which config files have changed, so that hook_config_reload() (or whatever the hook) only has to deal with actual changes, rather than having to deep compare every single item to see if it's different.

sun’s picture

Issue tags: +Configuration system
Anonymous’s picture

re #8, that's a fine idea.

we could definitely skip checking contents and just go with mismatched sigs to mean 'candidate for reload'.

Dave Reid’s picture

I'm still concerned that since these directories (while random) are still writable by the webserver and while I don't yet have a "this is my specific concern" I feel that for non-apache systems can we guarantee that the files directory is not 'browse-able' (and therefore vulnerable to leaking the config directory name and/or its files)? I do think having signatures available just for comparing old files vs new is good, but how much overhead and messy code are we talking about for verifying signatures? The code potentially removed in #6 doesn't immediately show me that we'd get a massive benefit.

sun’s picture

Although hashing (!= signing) might make sense, I'd highly prefer to remove and don't have to care for each and every complexity that isn't really required.

Hashing itself is also highly debatable. Mere white-space (un)intentionally added to a config object will show a difference (typical binary comparison and CRC flaws), although you can avoid that by creating a separate hash file based on the raw data when saving it. However, you don't strictly need a separate hash file to compute and compare hashes; you can simply read in the raw data objects, compare them, and forget them. Furthermore, the hash only exists for the file storage, but not for the db store (yet). Lastly, a hash-based comparison still contradicts the goal of allowing developers to easily go into a config file and add or edit a value. (There's a lot debatable there...)

We're going to be very busy for the next months, with all the follow-up issues and API changes that still need to happen, until the last bits of variable_* are removed. If we'll be faster than predicted, fine, we can think about optimizations. But until then, I think it would be wise to drop every bit of code that's undecided baggage and burden throughout the rewrite process.

jmcclelland’s picture

I don't see any signficant security benefit to signing and verifying configuration files. It seems to add a significant amount of complexity in exchange for only marginally better security.

If I have write access to the config directory and I am able to write a configuration change to a file in that directory - it seems plausible that I will also have read-access to the necessary files needed to generate a valid signature.

Requiring the valid signature only seems to make it a more daunting programming task for the attacker, but won't actually stop an attacker from modifying these files in a way that will cause them to be loaded.

I would prefer a simpler configuration implementation and a simpler direction for the site admin about how to secure your site (don't let rogue processes write to or read from your config directory). Remember - right now, if a rogue process can simply read your settings.php file most sites will be effectively compromised. Given that requirement for security, it seems reasonable to expect admins to be able to protect write access to the config directory as well.

I'm in favor of simple steps to add marginally better security (like a suggestion on the lambic's suggestion on the thread describing the config changes to allow this config directory to be placed outside of the web root), but the signing approach seems like a lot of complexity for limited pay-off.

gdd’s picture

The config system will not work unless there is a place to store config that is writeable by the web server. While you will be able to store config outside webroot, that is not a solution for all cases and we need to have a good least-common-denominator answer. Additionally a remote attacker who can write new config can not necessarily write a new signature. The key needed to generate a valid signature is stored in settings.php, and if your attacker has access to that then writing new config is the least of your worries. An example of how this might happen is an attacker being able to upload files into the config directory (they can write there but can't read the key needed to generate signatures.)

Now whether this is a significant enough worry to make up for the complexity of the code to manage the signatures is up for debate, but it does offer some protection.

philippejadin’s picture

Two comments :

- is'nt it easy to use brute force attack the directry name to access config directory? Trying every combination and doing an http get on /sites/default/files/config_[**key**] would maybe not take that long (as the weebserver is not rate limited to serve 404 for every wrong combination)

- are we really sure we want to put sensitive information in those config files? Views definitions, fields definitions, image styles, block positions... are not sensitive information imho. Those who want to protect this type of definitions will have to learn how to harden their server anyway. On the other hand, storing DB credential in those xml files would be a bad idea (settings.php is a better place).

The proposal then : store sensitive informations in settings.php, the rest in xml, and be done with it.

David_Rothstein’s picture

From Dave Reid's comment in #11:

I'm still concerned that since these directories (while random) are still writable by the webserver and while I don't yet have a "this is my specific concern" I feel that for non-apache systems can we guarantee that the files directory is not 'browse-able' (and therefore vulnerable to leaking the config directory name and/or its files)?

This seems like a valid concern to me also.

On the other hand, if the config directory name is leaked the concern in that case isn't limited to what happens if the directory is writeable (which is what the file signing protects against), but also that fact that discovering it means you can read it too, right? It's not totally clear to me how much sensitive information is expected in this directory, but given that the config system is supposed to replace variables, and modules store all sorts of sensitive data in the variable table currently (e.g., passwords/keys for external services that your Drupal site needs to talk to), it seems like that's a definite possibility.

So basically what I'm saying is that it's a concern, but file signing isn't necessarily a complete protection against that issue anyway.

Just a thought - would adding a dummy "index.php" file to the Drupal files directory help prevent it from being browseable in more cases? (Haven't tested that.)

David_Rothstein’s picture

- is'nt it easy to use brute force attack the directry name to access config directory? Trying every combination and doing an http get on /sites/default/files/config_[**key**] would maybe not take that long (as the weebserver is not rate limited to serve 404 for every wrong combination)

Since the key is constructed using a randomly-generated sha-256 hash, the number of possible keys is absolutely enormous. I'm pretty sure we're talking about many many many times the age of the universe for someone to brute-force it...

Anonymous’s picture

just to put it on record, though i'm aware no one else seems to care: lets not write the sensitive data to files during the normal course of operation of the site. instead, lets only write when explicitly called for by an export operation. and only read when we're doing an import/sync operation.

really. i just have NFI why we persist with this idea, when it so clearly introduces extra security concerns and complexity.

/me goes back to waiting for a decision he doesn't agree with so we can get on with implementation

bojanz’s picture

just to put it on record, though i'm aware no one else seems to care: lets not write the sensitive data to files during the normal course of operation of the site. instead, lets only write when explicitly called for by an export operation. and only read when we're doing an import/sync operation.

I couldn't agree more with this. It matches the discussion we had at the DrupalCon sprint.

Dave Reid’s picture

I don't remember ever discussing secure data should not be written to CMI. But we have discussed that CMI should eventually replace the variables table which does contain secure data for a lot of modules. If that's the decision that's made then it needs to be 100% clear to all module authors that they need to follow suit, because I just know regardless it's going to add work for the security team for modules that do things wrong. That's not something specific to CMI, but are we'll likely require SAs and new releases for modules that store secure data to CMI.

Anonymous’s picture

#20 - i didn't express myself clearly. i'm not just talking about 'sensitive' config data here. i don't think we should write any config to disk during the normal course of operation of a site.

writing out the files should be an export operation only. this will remove extra complexity we're about to build in around the reload/sync process, and will greatly reduce the security issues. but, meh.

if we're going to continue with 'all your configs will be on disk, all the time' by default, and not require a directory outside of the webroot, there's just no good way to avoid having simple mistakes lead to security issues.

we'll need to a) make it easy to encrypt values in the config and explain why it's important and b) make people aware of the importance of validating their webserver setup, as it is the only thing (apart from obscurity, yay) between their config and the interwebs.

gdd’s picture

Even if we turn off file writethrough by default (and I'm reasonably sure we will) I still think we have to be concerned about the files sitting around. People will deploy them and forget to remove them. and some people will still want writethrough on all the time. I also don't think we can prevent sensitive data from ever being used in the files. API keys are one extremely common example of data that will live here.

#11: index.php would prevent the directory being browsable, but it wouldn't stop surfing to a specific file. Given that the file names are public knowledge, and figuring out what modules a site is running is pretty trivial, I don't see how an index.php would help us at all. Also, as you point out, this discussion is only about preventing problems with files being changed/added. Browsing/surging to files is unrelated to the signing.

So while I see some unease about this, I don't see any concrete worries other than beejeebus' problems with writethrough of files which is most likely going away anyways. Anyone?

David_Rothstein’s picture

index.php would prevent the directory being browsable, but it wouldn't stop surfing to a specific file. Given that the file names are public knowledge, and figuring out what modules a site is running is pretty trivial, I don't see how an index.php would help us at all.

My thought was that index.php would go in sites/default/files itself; thus, you would not be able to discover the identity of sites/default/files/config_XXXXXXXX via browsing, so you wouldn't know the full path of any config files within it (even if you know the file name). Without knowing the full path of the file, you can't surf to it or write to it, right?

gdd’s picture

That would only affect sites with directory indexes enabled right? Given that Drupal's default .htaccess and web.config both ship with this turned off, I don't see that we would gain much, and this still doesn't really have much to do with the signing in the first place.

pounard’s picture

Agree with heyrocker, I guess that the index.php trick would only make sense on misconfigured environments and for which the HTTPd behave the same as Apache/mod_php (for CGI based environment it wouldnt make any sense since URI will be cleaned up and checked prior to request being send to the gateway, and they don't know how to run PHP by themselves).

sun’s picture

FWIW, I agree with all arguments and proposals raised by @beejeebus so far. Make the writeToDisk operation explicit, and let's also investigate introducing an optional, official private filesystem outside of the public document root for D8.

Furthermore, the current "reload" (sync) patch skips the file signature validation for new configuration (for technical reasons I forgot), so the actual reason for having the signing is invalidated.

perusio’s picture

Just want to say that in Nginx you can mark a location `internal`, meaning that it can only be accessed from within the server. No outside access is possible (it returns a 404 by default). So there's no need for any private filesystem. What's needed is just a well specified location.

Could be sites/default/files/site-config. Then just do:

location ^~ sites/default/files/site-config/ {
     internal;
} 

This forbids any access to files on this location and all subdirectories.

At least for Nginx there's no need for any private filesystem whatsoever.

I also want to raise the issue of performance when we consider private filesystems. They require X-Sendfile like mechanisms to be efficient. And at least in Apache X-Sendfile is not included in most common configurations furnished by the most popular Linux distros, for example, AFAIK.

EDIT: This in the eventuality of having a need for reading the files from within the code using HTTP methods.

sun’s picture

Status: Needs work » Needs review
FileSize
17.08 KB

Attached patch performs a full removal. Code is located in the config-sign-1444620-sun branch of cmi.

gdd’s picture

I agree that we should do this, and the patch looks good to my eyes. I have attached a new one, which just contains a few cleanups and is rerolled against HEAD. If it passes I think this is RTBC.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the additional clean-ups, @heyrocker.

I'd like to fast-forward with #1447686: Allow importing and synchronizing configuration when files are updated, which equally attempts to introduce (a variant of) FileStorage, so I'm hope we can kick off some more progress on the underlying API refactoring that's badly needed by committing this patch, and also moving on over there.

Note: This does not necessarily mean that we want or need to close this issue. We've discussed many pros/cons on configuration system security in here. It merely means that the only conclusion, right now, is that the file signing in its current form is not an appropriate security mechanism for the configuration system. Whether we need a new one (or not) can and should be discussed further. Either in here or in a new issue.

catch’s picture

Status: Reviewed & tested by the community » Active

This makes sense to me as well now the file path is random so I've committed/pushed the patch to 8.x. Agreed we should leave this open (or open a new issue) so this doesn't get forgotten though - we might want to add something back later on, so moving to active for now.

sun’s picture

webchick’s picture

Issue tags: +revisit before beta

Tagging so we (hopefully) don't forget.

Does this still need to be active in that case?

cosmicdreams’s picture

from #1605324: Configuration system cleanup and rearchitecture

I had this question:

+++ b/core/lib/Drupal/Core/Config/FileStorage.phpundefined
@@ -91,10 +64,10 @@ class FileStorage {
+      throw new FileStorageException('Failed to write configuration file: ' . $this->getFilePath($name));

Does revealing the file path (with name?) here cause any security concerns?

pounard’s picture

Exceptions don't care about security, their messages should never be displayed to the end user in a production environment IMHO.

gdd’s picture

While that is true, there is no doubt that there may be cases in which they are are in fact displayed. Is there a downside to removing the path and leaving the filename? If it gets revealed it is definitely a very big security problem.

sun’s picture

I don't see an issue with removing the path to the file and only outputting the filename.

The only time when that might get a bit hairy is when FileStorage does not operate on the default config directory but tries to fiddle with default config of extensions instead. However, those cases are very rare, and the mere origin of the exception should be sufficient to figure out the directory it is trying to operate on.

pounard’s picture

While that is true, there is no doubt that there may be cases in which they are are in fact displayed. Is there a downside to removing the path and leaving the filename? If it gets revealed it is definitely a very big security problem.

Normal production sites should always be error reporting disabled, and the default error page should never display the technical exception type nor message if error reporting is disabled. If exceptions do reach the final UI in production mode, it's a bug of the upper layers and of the error reporting, not a bug of this API IMHO. We should let exceptions pass and ensure the error listeners (PHP error handlers and kernel listeners) to behave correctly, but this is another issue that fits better with WSCCI, kernel, and other framework UI improvements.

philippejadin’s picture

... and 6 monts later people will google for "Drupal Failed to write configuration file:"
and will find some configurations keys to hack on.

I would definitely not write the config path to screen.

pounard’s picture

That's exactly the reason why a production site should not display technical errors on screen, but for the sake of debugging the exception must carry details for the developers to be able to debug.

greggles’s picture

I agree we should not include the full file path in the error message.

pounard’s picture

I'm not sure that hidding debugging information that aims to be delivered to the developer's eyes will make it more secure, the problem will still exist for a lot of other stuff, since exceptions can be thrown by any business layer, at anytime, even PHP and some extension themselves on which we don't have any control over. PHP errors or warnings will reveal themselves errors such as file not found, or syntax or PHP warning with the full path clearly readable thus making this minor security measure (not saying the file path in the exception message) totally inneficient. Revealing file path, failed queries, and other various technical information will continue to happen anyway.

This post on stackoverflow seems to lean towards saying that the real security measure is to never display those technical exceptions to any user except to the developer. My opinion is that the error level variable actually does it very well, since Drupal will have its own exception handler set at a very early bootstrap phase. Exceptions should go to into a logging facility (being catched either by the kernel, either by the error handler in last resort). I don't think we should obfuscate the exception messages for the sake of security: we need this information for debugging, and we may even need it more when something is failiing on a production site and we cannot reproduce the error elsewhere: details (including file path) logging is important for site maintainers.

But in the other hand, this post from stackexchange (which is PDO specific) says that information leakage should be reduced everywhere. While I agree with them, I also agree with the original answer of the PHP core dev which says that the error logging should be managed correctly. I took this post more lightly because it's about leaking passwords, not just file paths.

This one page is also very revelant to the problem of information leakage, and says that our exceptions won't be the only one failing on us for this kind of information leakage (even other layers than PHP can do too).

The only solution seems to use a proper global error handler, and Drupal does.

EDIT: From this post, I made my point, so I leave the final decision to the majority and what the security team think about it.

webchick’s picture

We need to reduce information leakage everywhere, because the configuration directory will be the same on dev as it is on live, and so if you forget to set up custom .htpasswd protection on dev.example.com (or if it ever fails), attackers will have everything they need to get at your production config files as well.

greggles’s picture

Drupal does have error handling, but it is not foolproof. There are times in the past where people would rightly assume that Drupal's error handler catches everything, but it doesn't: #1576300: trim() expects parameter 1 to be string, array given in request_path() (line 2732 of bootstrap.inc).

In the case of the docroot an accidental disclosure is a mistake that can be combined with other issues to cause a problem. In the case of the config directory it is a problem in itself. There are times when the size of the exposure makes a bit of defense in depth worthwhile and I think this is such a case.

greggles’s picture

All that said, I agree that for debugging reasons it would be ideal to display the path - if there's some way we can achieve that then that seems great.

David_Rothstein’s picture

Another complication is that even if your site is configured correctly (no error reporting to the screen), administrators with "access site reports" permission can still see this in the logs. But Drupal considers that a low-level administrative permission, not a high-level one.

Also, even if it's a high-level administrator viewing it, there are still good reasons never to display sensitive information on the screen (we don't display a user's password even to that user, right?).

That said, I think @pounard has a point that there are so many other ways this kind of thing can be disclosed. For example, the full code being discussed above is this:

    if (!file_put_contents($this->getFilePath(), $data)) {
      throw new FileStorageException('Failed to write configuration file: ' . $this->getFilePath());
    }

Before the exception is even thrown, the file_put_contents() call itself will likely log a warning (with the full path) if the file isn't writable, won't it?

In addition, as a result of #1464944: Installation of configuration system fails in some cases, I believe there are cases where we're now going to print the config directory on the status report page also (mea culpa on that one)...

All in all, it may not make sense to protect the config directory name in some places if we can't protect it everywhere, and overall this is starting to feel like whack-a-mole, and like security by obscurity. Perhaps we'll have to accept that the name of this directory can't be kept entirely secret, and rethink other things accordingly.

pounard’s picture

I wasn't going this far, even if I'm still heavily against obfuscating the directory name automatically (making it configurable would be best because it'd allow to set the config files out of the public webroot and allow easy scripting the same way) but this is not really the point in this issue. Anyway, this leaves intact the fact that no absolute path should ever go onto the end user screen, because it's also tied to the operating system security, but the best we can do here is to have a robust error handler. If this opens new questions it could be a good occasion to open security related issues. As a developer, I really need to see those pathes in the debug information and error log, else it will be hell to find out who's the guilty file and or module.

gdd’s picture

As a note, it is configurable by setting a value in settings.php at installtime, just like database settings. It's only through the default install that the obfuscated directory name is created. I believe this setting is currently relative to webroot but that is a bug that will be fixed before launch.

pounard’s picture

Nice

gdd’s picture

Status: Active » Closed (fixed)

I think, over time, I have come to agree with David_Rothstein and pounard. There are lots of places this information can be exposed, and we could spend a ton of time trying to find them all and never come up with an answer that is complete. I'm going to set this back to fixed and if anyone wants feels like it needs more discussion, feel free to reopen.

David_Rothstein’s picture

Seven months later, I think it does need more discussion :)

However, any further work on securing this isn't going to result in adding file signing back (the removal of which was the original point of this issue) so I think we can discuss it at #1914018: hook_requirements() for un-protected configuration directories instead.

David_Rothstein’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

Issue summary: View changes
Issue tags: -revisit before beta