Some taxonomy names should be disallowed

[Denis-de-Bernardy]

I haven't tested, but can't we can get all sorts of weird bugs (some of which could have potential side effects in the security department) if a term taxonomy is called users, userlogins, posts, etc.?

the reason are lines such as:

wp_cache_add($term->term_id, $term, $term->taxonomy);

[dd32]

Rather than being dis-allowed, The group should be prefixed with something, such as tax-$taxonomy

Milestone: In my opinion, This is not a "high-risk" security issue. My reasoning for this, is due to that it takes a malicious plugin or theme running on your blog in order for an issue to arrise. Due to it having little impact upon end users (Rather, The onus falls on Developers right now to use a non-conflicting taxonomy name, And any malicious plugin could access/overwrite users directly) and not being a regression from a previous version, It belongs in the next major release.

[hakre]

outch, as what i've seen today, this will allow to overwrite posts in cache. I'll raise the severity and priority one step in the hope that we can improve things before we fall over our own feets.

BUT: I can not find that line in core: wp_cache_add($term->term_id, $term, $term->taxonomy); please reference at least one location for this to be reviewed.

[rooodini]

BUT: I can not find that line in core: wp_cache_add($term->term_id, $term, $term->taxonomy); please reference at least one location for this to be reviewed.

FWIW there's an equivalent call in the update_term_cache function of wp-includes/taxonomy.php (line 2167 of revision 14032):
wp_cache_add($term->term_id, $term, $term_taxonomy);

[shidouhikari]

I agree with dd32, a theme or plugin would need to create a taxonomy with a conflicting name.

That has no security severity, since we already rely on theme/plugin being trusted. If we'd consider this issue a security flaw, then what about theme/plugin being able to edit global $post from any action and filter, or add filters to get_option()? With these 2 little features anything in a site can be changed from a code running in modules.

Only security risk I see here is if a theme/plugin is installed, hides data in cache using this method, and then it's found as risky and removed from wp-content, but its altered data remains in cache until cache becomes outdated and forced to be updated from database. But if that's the case, whoever removes it will know about cache and a simple wp_cache_flush() will solve the rest.

The real problem here is if a theme designer that knows little about Core and never saw cache in his life decides to create a taxonomy "users". But terms have different fields than other objects, so the whole site would probably break still during development, just with pageload-living cache, still during development. He would find it out easily and learn about cache or much probably just try giving his taxonomy another name and see everything go back to normal. "Well, IDK why that happened, but it seems a taxonomy named 'users' breaks everything, I'll just use a less common name and move on".

using a prefix like "tax-".$term->taxonomy in every cache code solves it all.

[ocean90]

Replying to hakre:

BUT: I can not find that line in core: wp_cache_add($term->term_id, $term, $term->taxonomy); please reference at least one location for this to be reviewed.

In 3.0.* it's ​wp_cache_add($term->term_id, $term, $term_taxonomy).

[nacin]

This is silly.

Let's switch this to either a generic 'taxonomy' bucket or prefix this with 'taxonomy-'.

[wonderboymusic]

this is no longer a problem if we follow through with #21760

[wonderboymusic]

Attached the patch for #21760 here as well - they have 2 different subject lines, but they accomplish the exact same thing - this ticket is earlier, #21760 is more thorough, so leaving them both open but marking this as "has-patch" as well. If someone feels like dupe/closing one of them, have at it.

[bpetty]

Patch doesn't apply cleanly to SVN trunk (not even against r21952, which was latest SVN trunk when this patch was submitted oddly enough).

Anyway, as noted in ticket:21760#comment:22, the patch isn't trivial, no unit tests, and considering all of the above, I'm punting this from 3.5.

[wonderboymusic]

21760.diff​ from #21760 does this / this ticket is essentially a dupe, but the subject of #21760 is new caches, not new cache name.

[wonderboymusic]

For inwestigation, #21760 is probably the focus

[wonderboymusic]

#12929 was marked as a duplicate.

[nacin]

I prefer the direction of #21760, so let's close this.

[boonebgorges]

Many tickets have duplicate chains that lead to this one: #12929, #23213, #31154 et al. The current ticket was closed because it was decided to address the cache key conflict elsewhere. But registering a taxonomy using a reserved term causes many more problems than just cache-clash, such as rewrite conflicts. See eg #36997.

I'd like to reopen this ticket for consideration. There are a few ways forward.

1. A blanket moratorium on using reserved terms for taxonomies/CPTs. This is easy to implement, but may be overly harsh, and will definitely have back compat concerns. See @danielbachhuber's comment here: https://core.trac.wordpress.org/ticket/31155#comment:1
2. Do some digging to figure out what the specific conflicts are in the case of taxonomies/CPTs registered using reserved terms, and either (a) fix them, or (b) only block registration in the case of the specific conflict (ie, rewrite base). Option (a) essentially means that there will no longer be reserved terms. Eg, we could refuse to register a taxonomy with conflicting query vars, or force a non-conflicting slug, or something like that. But there are likely other potential points of conflict. Some of these conflicts won't arise in core. It's likely we'll never be able to make them work 100%. So while this strategy is nice because it accommodates the greatest breadth of use, it's likely to be the buggiest and hardest to figure out.
3. Allow registration of taxonomies/CPTs with reserved terms, but throw a _doing_it_wrong() notice warning that it's probably not going to work. Big downside here is that plugins that are using reserved terms without any problems now see an annoying notice that they can't easily get rid of. And building a tool in a plugin to migrate to a new CPT/tax name is non-trivial and subject to lots of breakage. Maybe we could add a filter that allows the suppression of this specific notice - if ( ! apply_filters( 'i_know_what_im_doing', false ) ) { _doing_it_wrong( ... ); }
4. Do nothing in core, but improve the documentation. The inline docs currently say nothing about reserved terms, and the codex page is not easily found ​https://codex.wordpress.org/Reserved_Terms. This is the easiest and poses the fewest compatibility concerns.

[danielbachhuber]

@boonebgorges Do you think this is still worth pursuing or can it be punted? I'm not sure it's worth solving.

[boonebgorges]

Fine with me to close. If someone steps up to do the heavy lifting, we can reopen.