WordPress.org

Make WordPress Core

Opened 9 years ago

Last modified 2 years ago

#11531 reopened defect (bug)

Some taxonomy names should be disallowed

Reported by: Denis-de-Bernardy Owned by: ryan
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 2.9
Component: Taxonomy Keywords: dev-feedback
Focuses: Cc:

Description

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);

Attachments (2)

term-cache.patch (13.0 KB) - added by wonderboymusic 6 years ago.
21760.diff (10.8 KB) - added by wonderboymusic 5 years ago.

Download all attachments as: .zip

Change History (20)

#1 @dd32
9 years ago

  • Keywords needs-patch added
  • Milestone changed from 2.9.1 to 3.0

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.

#2 follow-ups: @hakre
8 years ago

  • Priority changed from normal to high
  • Severity changed from normal to major

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.

#3 @hakre
8 years ago

  • Keywords reporter-feedback added

#4 in reply to: ↑ 2 @rooodini
8 years ago

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);

#5 @ryan
8 years ago

  • Milestone changed from 3.0 to 3.1

#6 @shidouhikari
8 years ago

  • Cc shidouhikari added
  • Priority changed from high to normal
  • Severity changed from major to normal

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.

#7 @nacin
8 years ago

  • Milestone changed from Awaiting Triage to Future Release

#8 in reply to: ↑ 2 @ocean90
7 years ago

  • Keywords reporter-feedback removed

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).

#9 @nacin
6 years ago

  • Milestone changed from Future Release to 3.5

This is silly.

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

#10 @wonderboymusic
6 years ago

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

Last edited 6 years ago by wonderboymusic (previous) (diff)

#11 @wonderboymusic
6 years ago

  • Keywords has-patch added; needs-patch removed

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.

#12 @bpetty
6 years ago

  • Keywords needs-refresh needs-unit-tests added
  • Milestone changed from 3.5 to Future Release

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
5 years ago

#13 @wonderboymusic
5 years ago

  • Keywords needs-refresh removed
  • Milestone changed from Future Release to 3.6

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

#14 @ocean90
5 years ago

  • Milestone changed from 3.6 to Future Release

#15 @wonderboymusic
5 years ago

  • Milestone changed from Future Release to 3.7

For inwestigation, #21760 is probably the focus

#16 @wonderboymusic
5 years ago

#12929 was marked as a duplicate.

#17 @nacin
5 years ago

  • Milestone 3.7 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

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

#18 @boonebgorges
2 years ago

  • Component changed from Cache API to Taxonomy
  • Keywords dev-feedback added; has-patch needs-unit-tests removed
  • Milestone set to Awaiting Review
  • Resolution duplicate deleted
  • Status changed from closed to reopened

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.
Note: See TracTickets for help on using tickets.