function cache_clear_all($cid = NULL, $table = NULL, $wildcard = FALSE) {

and

db_query("DELETE FROM {". $table ."}");

When passing on the wrong value for $table the wrong table can be wiped out accidentally. This can easily occur when making a typo (like I just did) or when there's a bug in a third-party module that allows users to enter a value for $table indirectly (hardly plausible, but possible).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dpearcefl’s picture

Status: Active » Postponed (maintainer needs more info)

Does this issue exist in current D6?

dpearcefl’s picture

Status: Postponed (maintainer needs more info) » Active
pounard’s picture

Title: cache_clear_all uses direct input as table name to delete from » cache_clear_all('*', 'block', TRUE); will TRUNCATE the {block} table without additional checks
Version: 6.x-dev » 7.22
Priority: Normal » Critical

Today I tried:

drush php-eval "cache_clear_all('*', 'block', TRUE);"

and as a fun side effect it TRUNCAT'ed the 'block' table.

The fun part is that I did it on a production site. The other fun part is that all known cache bins are configured to use a MongoDB backend, but unknown ones still fallback on DrupalDatabaseCache backend per default.

The very fun part is that everything went very well and I got no errors.

Hopefuly, we have backups, it has been restored in 10 minutes, but think about it: any poorly coded module could use a variable as second cache_clear_all() parameter and potentially TRUNCATE *any* database table without further checks.

Just try with:

drush php-eval "cache_clear_all('*', 'node', TRUE);"

For additional fun.

What happens is that cache_clear_all() will load any cache object loosly and lazily without further checks, and call the clear() method on it. The DrupalDatabaseCache::clear() method does not do any further security checks and will just run a db_truncate() no questions asked.

Escalating this to critical, this is a very dangerous behavior: cache_clear_all() MUST NOT arbitrary TRUNCATE any other table than a valid cache bin.

Drupal 8 handles cache very differently, and cache bins are supposed to be registered in the kernel (at least I hope) but Drupal 7 cache handling it way too lazy about this. At least some checks should be done before the TRUNCATE statement in order to ensure we are really hitting a cache table.

alexpott’s picture

What's wrong with drush cc?

pounard’s picture

Nothing is wrong with drush cc, that's actually not the point of this issue. The API method can do permanent damage to data where it shouldn't. This should be more robust.

pounard’s picture

There is a bunch of pathes to explore to fix this, I can see two obvious that would work:
- Force modules to register their bins with a hook, deny any cache operation on an unknow bin [breaks BC]
- Enforce the $bin parameter to start with 'cache_' by appending this prefix if not set in the cache_* helpers.

The second one was actually one of the first D8 improvements in the very begining even before SF adoption, I think is a good candidate for D7.

catch’s picture

There's nothing stopping a module registering a cache bin in Drupal 7 that doesn't start with cache_* so that'd also be a bc break.

pounard’s picture

@catch In the latest 3 years, I never saw any module not using the 'cache_' prefix, except maybe one CTools table. Is there any other solution you could see?

markpavlitski’s picture

Version: 7.22 » 7.x-dev
Status: Active » Needs review
FileSize
1.6 KB

We can use the database schema to confirm if a cache bin represents a genuine cache table.

This patch checks that all of the common cache table fields exist in the table before truncating.

I've only added a check for the truncate operation, since the missing fields will cause other cache operations to throw a PDO exception anyway.

Status: Needs review » Needs work

The last submitted patch, drupal-check-cache-bin-238250-9.patch, failed testing.

pounard’s picture

This approach could actually work, I'm not comfortable with live database schema checks, but considering than the cache clear with no conditions is not supposed to happen often on a live site, why not. Let's see why it fails.

pounard’s picture

Those are all PHP notices in the isValidBin() function.

Damien Tournoud’s picture

Version: 7.x-dev » 8.x-dev
Priority: Critical » Normal

Shooting your own foot doesn't qualify as a critical bug of the gun. Let's see how we can improve the situation for Drupal 8.

pounard’s picture

The status was here to get some attention, indeed it's not that critical, but it remains very dangerous.

Damien Tournoud’s picture

@pounard: That's exactly why we have higher level tools. Alex is totally spot on in #4: that's why drush cc exists in the first place.

pounard’s picture

Version: 8.x-dev » 7.x-dev

My opinion is that only hard registration of bins can cleanly avoid that.

Today in 8.x nothing prevents you from fetching an arbitrary bin from Drupal\Core\Cache\CacheFactory::get() method, but the container hides the factory and will only provide you registered services such as "cache.binname", which are explictely registered via the Bundles or the *.services.yml file: in normal 8.x runtime (even in Drush if it uses the core correctly) reaching that code path can't happen if the API has been used the way it is designed for.

Switching back this issue as 7.x only because 7.x API allows this code path to be reached very easily, and is even the appropriate way of using the API.

pounard’s picture

#15 is right, indeed, but in the end nothing prevents a module from passing a variable there, which at some point could have been dynamically built. Drush is not a Drupal core provided tool nor a requirement to be used, so it is definitely out of this issue scope.

markpavlitski’s picture

Status: Needs work » Needs review
FileSize
1.79 KB

@pounard This patch checks for cache and cache_* tables first, so only runs a schema check for edge cases. It should also fix the notices which caused #9 to fail.

pounard’s picture

I do like the if ($this->bin == 'cache' || substr($this->bin, 6) == 'cache_') { first line! The schema check using the hook_schema() will populate the schema incremental cache, and might end up in a minor memory usage raise on the pages where the cache_clear_all() method would be triggered with those parameters, but I'm not really worried, it's not supposed to happen during a normal runtime. May I add I prefer this to straight db_field_exists() calls.

I think this patch is good enough for core and will make this safer, the only problem it can cause is for poorly coded modules: those wont don't use the cache_ prefix won't work anymore, but I'd say that:
1) I don't think there are a lot of them out there
2) They need fixing, and patching them will be easy

In my opinion, for this kind of issue, security wins over compatibility, especially when it will explicitely break only modules that don't respect the core naming conventions.

markpavlitski’s picture

@pounard It's worth noting that this will still work with modules using a non-standard table name, i.e. {mymodule_cache}, as long as they base their schema on the system cache schema. If not, their cache table probably won't work anyway.

markpavlitski’s picture

@pounard It's also worth noting that the potential memory increase only occurs in edge cases, i.e. when attempting to cache_clear_all() on an invalid bin or on a bin using a non-standard format.

pounard’s picture

Status: Needs review » Reviewed & tested by the community

Indeed, I spoke too quickly! I'm quite fond of this solution, it's straight-forward, induces aboslutely no performance penalty when using modules that respect the naming conventions, and fallbacks smartly when using modules violating the conventions, keeping BC.

I think this is RTBC, very nice, thanks!*

catch, Damien Tournoud, alexpott, I think we have a solid patch here, opinions?

EDIT: Just one additional note: I think we should document why this exists, anyone who read the code without any hint may find this useless and remove it. Maybe adding the issue link too.

markpavlitski’s picture

@pounard Agreed. This is the patch from #18 with additional documentation referencing this issue page.

pounard’s picture

Thanks, I think that #23 is RTBC. EDIT: There's probably a few coding standards nitpicks to apply, but I don't see any of it myself, the patch seems fine as-is.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
+++ b/includes/cache.incundefined
@@ -505,7 +505,13 @@ class DrupalDatabaseCache implements DrupalCacheInterface {
+          if ($this->isValidBin()) {
+            db_truncate($this->bin)->execute();
+          }

Shouldn't we throw a runtime exception here if isValidBin not met?

Patch looks sane to me... but we need to test the isValidBin function...

markpavlitski’s picture

@alexpott I'll add some tests to the next patch.

Other DrupalDatabaseCache functions, namely get() and set(), fail silently rather than producing an error. Do we definitely want to throw an exception here?

Edit: They fail silently when a database issue occurs, rather than user/caller error, so probably we do want to throw an error here.

David_Rothstein’s picture

The substr() call looks to me like it has the wrong parameters, and there's no need to "see issue X" in documentation (if there's any thing extra that needs to be said, just add it in the documentation)...

Otherwise, I agree that silently failing here doesn't seem right (@markpavlitski: the code comment says "other cache operations throw a PDO error in this situation", so is it wrong?)

Honestly, I wonder if the simplest way to deal with this issue isn't just to change db_truncate() to something like db_delete($this->bin)->where('cid = cid')->execute()? Then we get the PDO exception thrown automatically... A little bit of a hack, but in some ways it's cleaner than loading up the whole schema and checking it.

catch’s picture

A db_delete() is going to be a lot slower than a db_truncate() if you have a lot of rows, this is why db_truncate() uses native TRUNCATE if it's available.

markpavlitski’s picture

@David_Rothstein You're right about the substr() call, I'll correct this.

Using db_delete() could introduce a serious performance penalty over db_truncate, especially on larger tables such as {cache_form}.

The PDO exception comment refers to the other cache_clear_all() operations, such as:

db_delete($this->bin)
            ->condition('cid', array_splice($cid, 0, 1000), 'IN')
            ->execute();

In the situation mentioned by the OP, where {block} was used as the bin name, the cid field doesn't exist so would throw a PDO exception.

pounard’s picture

@#27 TRUNCATE operation with MySQL also offers the courtesy of freeing the table disk space, which DELETE won't necesarily do. It is also known to be faster in scenarios where the table has a high volume of data. I think TRUNCATE remains good to use here, cache tables are higly mutant and can potentially (think of cache_page and cache_block which are emptied every 3 hours) grow *a lot*, and a table on which you don't OPTIMIZE manually on MySQL is a table which may never reclaim disk space.

markpavlitski’s picture

Status: Needs work » Needs review
FileSize
3.6 KB

This patch throws an exception rather than failing silently, clarifies the documentation, and adds test coverage.

Damien Tournoud’s picture

TRUNCATE is probably a bad idea here, because (1) it's a DDL statement and has such has to lock *the whole table* to execute, and (2) it's awfully slower then a DELETE on mostly empty tables; but discussing this is not in the scope of this issue.

alexpott’s picture

+++ b/includes/cache.incundefined
@@ -505,7 +505,16 @@ class DrupalDatabaseCache implements DrupalCacheInterface {
+            throw new Exception(t('Invalid or missing cache bin specified: %bin', array('%bin' => $this->bin)));

this should throw a http://php.net/manual/en/class.runtimeexception.php

markpavlitski’s picture

Priority: Normal » Critical

@alexpott I can't see a RuntimeException being thrown anywhere else in the D7 codebase, only in D8. Can you confirm?

alexpott’s picture

Priority: Critical » Minor

Oh ok... had my Drupal 8 head on :) ... it's no biggie so leave as is

alexpott’s picture

Priority: Minor » Normal

Didn't mean to make this minor... it's just not critical :)

markpavlitski’s picture

Oops, didn't mean to change priority... Thanks for clarifying!

pounard’s picture

Status: Needs review » Reviewed & tested by the community

Switching back #31 to RTBC.

pounard’s picture

Status: Reviewed & tested by the community » Needs work

Oups didn't see the debate about Exception vs RuntimeException sorry.

markpavlitski’s picture

Status: Needs work » Needs review

@pounard I think that was agreed- Exception is fine for D7 (see #35).

pounard’s picture

Status: Needs review » Reviewed & tested by the community

Oh OK, so back RTBC for me.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.23 release notes

Alright, let's do this. Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/f019275

I removed the @see pointing to this issue on commit (since there's still no need for that), and also removed t() from the test assertion messages.

pounard’s picture

Great! Thank you very much for this.

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests, -7.23 release notes

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