Vocabulary help needs filter_xss

here's the doc - though this code needs some cleanup too.

"though this code needs some cleanup too"

Right, as I was working on these patches, I discovered a few lame things about all this code:

A) The 2nd arg to _taxonomy_term_select(), $name, is completely ignored. ;) The only reason it's passed into taxonomy_form() itself is because it's "needed" for _taxonomy_term_select(), so it could be killed from taxonomy_form(), too.

B) There's sort of a tangled web of who is responsible for producing and sanitizing the various pieces of data that could be malicious here. taxonomy_form() itself in taxonomy.module always sanitizes the title (which always comes from the vocabulary itself) as it's invoking _taxonomy_term_select(), but the help text is either trusted from the caller, or sanitized when we look it up. Basically, this whole API is weird. Some things you can override, other things are hard-coded given the data saved with the vocabulary. We end up sanitizing things at a few different call sites. It's not completely self-evident at each call site why things need to be sanitized, and I don't get the sense we're consistent about this -- I suspect other places of core put the sanitation closer to where we actually generate something that's going to be output. I think this is very fragile, and there's potential for introducing new XSS as this code is moved around if we don't have a clear "who's responsible for what" policy in here that can be easily understood and verified. Even though in some cases the help text is coming from a t()'ed string literal, part of me would really like to put all the sanitation in one place, deep inside _taxonomy_term_select(), so none of the call sites have to (sometimes) sanitize on their own.

(A) should clearly be fixed. We could leave (B) in D7 the same as D6, since technically it's all correct and secure, but I think it's a bit of a DX mess and we should consider if/how to clear this up.

Also, shouldn't security ports be considered "critical"?

Cheers,
-Derek

Well, look at that. ;) (A) was already solved in HEAD via #431130: Unused arguments in internal taxonomy functions... Cool.

This patch merges my PHPDoc from #1 into HEAD (removing the unused $name argument), and applies the XSS fixes. I didn't yet touch (B), since that needs discussion before spending time on it. But, IMHO, this patch should be RTBC to at least close the vulnerability in HEAD itself.

we should either deal with the clean-up issue to some degree, or also add doxygen stating that $help passed in to taxonomy_form() needs to be sanitized.

Clean up is in progress at #295395: DX: convert Taxonomy selectors to FAPI widgets (which ironically fixes the XSS issue with a check_plain($vocabulary->help) too).

Should it be check_plain()?

I see no reason to prevent markup in vocab help text. I think filter_xss_admin() makes the most sense.

So, what's going on with these issues? Are you saying this is duplicate with #295395? I don't understand why we can't at least plug the hole now, and worry about refactoring + cleanup seperately.

I meant since there's an existing issue for cleanup, let's just plug the hole here and sort the rest of the mess out later. Which means #4 ought to be RTBC I think, but I'll leave it to pwolanin in case we still want to add those extra docs.

D7 patch with phpdoc for taxonomy_form() explaining that $help needs to be sanitized if defined. Also, backport patches of this phpdoc for D6 and D5.

I think the new doxygen needs a little more work - I don't think we usually use "defined", core uses "specified" or "omitted"

@param $sid
  (optional) The SID of the item to wipe. If specified, $type must be passed too.

 @param $field_type
  (optional) A field type name. If ommitted, all field types will be returned.

So something more like:

+ * @param $help
+ *   (optional) help text to use for the form element. This must already be
+ *   sanitized (e.g. with filter_xss_admin() or check_plain()).  If omitted,
+ *   the help text stored with the vocaulary (if any) will be used.

@Heine - I don't understand the downside. It must be sanitized somewhere upstream.

All of this code will potentially change before D7 ships via #295395: DX: convert Taxonomy selectors to FAPI widgets. I wish we weren't wasting time splitting hairs over this PHPdoc. This issue was meant to be a quick-fix to get HEAD up to par with the D6 security release, nothing more.

That said, to appease both of you, s/defined/(specified|omitted)/ as requested. I also added an "if it is user-supplied" qualifier on the part about needing check_plain() or filter_xss_admin(). This comment does not seem like an appropriate place to teach developers about the difference between those two, and it already points to the filter_xss_admin() docs.

Can we please stop bikesheding these comments and plug this hole? ;)

Try that again, testing bot.

looks fine.

Committed to HEAD. Moving down to 6.x for those docs changes.

- The $vid PHPdoc on taxonomy_form() does not have a dot at the end.
- On the security discussions, it was requested that _taxonomy_term_select() gets more detailed documentation on what "sanitized" means. This was better documented on taxonomy_form().

Let's get these fixed on the 7.x patch and then backport.

@Gabor - I don't recall such a discussion. Given that _taxonomy_term_select() is not an API function, (it's nominally private) I think what's in the patch is really sufficient.

Heine objected against unclear description of what should a parameter actually contain. Regardless of internal or external use, this is important for security, since we just made a mistake here, which we fixed via the SA, so let's make the documentation clear.

Knock yourselves out.

just phpdoc now.

Patch from #14 still valid for D6

D7 have neither _taxonomy_term_select() no taxonomy_form() anymore

Ok, looks like the latest patch does have some examples of sanitization and the dot I could add by hand-editing. Committed, thanks.

D5 should be fixed too! patch from #14

Marking as won't fix as d5 is end of life.