Make WordPress Core

Opened 4 years ago

Closed 7 months ago

#11531 closed defect (bug) (duplicate)

Some taxonomy names should be disallowed

Reported by: Denis-de-Bernardy Owned by: ryan
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: Cache API Keywords: has-patch needs-unit-tests
Focuses: Cc:


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 19 months ago.
21760.diff (10.8 KB) - added by wonderboymusic 15 months ago.

Download all attachments as: .zip

Change History (19)

comment:1 dd324 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.

comment:2 follow-ups: hakre4 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.

comment:3 hakre4 years ago

  • Keywords reporter-feedback added

comment:4 in reply to: ↑ 2 rooodini4 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);

comment:5 ryan4 years ago

  • Milestone changed from 3.0 to 3.1

comment:6 shidouhikari4 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.

comment:7 nacin3 years ago

  • Milestone changed from Awaiting Triage to Future Release

comment:8 in reply to: ↑ 2 ocean903 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).

comment:9 nacin20 months 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-'.

comment:10 wonderboymusic20 months ago

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

Last edited 20 months ago by wonderboymusic (previous) (diff)

comment:11 wonderboymusic19 months 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.

comment:12 bpetty18 months 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.

wonderboymusic15 months ago

comment:13 wonderboymusic15 months 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.

comment:14 ocean9011 months ago

  • Milestone changed from 3.6 to Future Release

comment:15 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

For inwestigation, #21760 is probably the focus

comment:16 wonderboymusic8 months ago

#12929 was marked as a duplicate.

comment:17 nacin7 months 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.

Note: See TracTickets for help on using tickets.