SA-CORE-2009-007 forum module XSS

There's another issue out there somewhere for making this not only check that it's numeric but also check that the number is actually a forum term so you don't get wierd results by passing TIDs from non forum terms. I don't know if that can be added here or has to be seperate from this security patch but thought I'd mention it as long as you're patching that bit. :)

Michelle

Attached patch includes check to verify the provided term is valid.

I think we should just commit the patch in #1, and put the other part in a follow-up, as in the current state, it severely changes behaviour of that function (in case of a 0).

The conditional statement in #4 won't allow zero values for $tid to pass. It will display the Forum overview or no Forums defined message (whichever is appropriate), rather than returning MENU_NOT_FOUND.

Attached patch uses filter_var() to validate that $tid is an integer greater than or equal to 0. Furthermore, it checks to make sure it's a valid taxonomy term in the forum vocabulary.

Err, now with whitespace fixes.

Final pass.

I removed the checks ensuring that the taxonomy term is valid and is a forum term as I decided to rethink where they should be implemented. I will be opening a separate issue for that.

This final patch accomplishes only what is needed to fix this issue - that is, the security issue.

Could someone please take a look at this so we can ship alpha 1 with no known security holes? :)

I suggest we fix that properly.

The real issue is that the forum module is using menu loaders improperly. Here is a minimal fix.

By the way, properly using menu loaders also fixes #221860: 404 pages and containers.

Looks good to me, but I will wait for someone else to verify the fix as well since it's a security issue.

Are going to want to re-backport this fix to D6 after?

It seems there is an incomplete docblock in the comments for forum_forum_load? It looks like you were going to define the keys in the array and then forgot.

I tried to confirm that this patch fixes the original security bug, but I can no longer directly reproduce that bug.

Steps to repeat:
1. Install core
2. Create a container
3. Create a forum inside that container
4. Post a node into that forum from step 3
5. Visit http://7d.local/forum/pipes%22%3E%3Ch1%3Emalicious%20html%3Ctable

Expected results:
The <1>malicious html<table would be injected into the page as html.

Actual results:
They aren't included as HTML, though I do get a "No forums defined" message which DamZ is right makes less sense than a 404.

After applying the patch and rebuilding the cache I get a 404 which feels more appropriate and is a performance improvement (since requests for invalid pages should do as little work as possible and 404 pages are relatively lightweight).

In summary: I think the security issue has been fixed already elsewhere, but the current patch in #19 is an improvement to core.

Needs work for the documentation.

#19: 520736-forum-menu-load-crazyness.patch queued for re-testing.

+++ modules/forum/forum.module
@@ -715,17 +722,40 @@ function forum_url_outbound_alter(&$path, &$options, $original_path) {
  * @return
- *   Array of object containing the forum information.
+ *   Array of object containing the forum information, with the following keys:
+ *    - 
  */

patch looks good, but I agree the docblock is incomplete. The return value now looks something like
tid (String)
vid (String)
name (String)
description (String)
format (String)
weight (String)
vocabulary_machine_name (String)
rdf_mapping (Array)
parents (Array)
forums (Array)

Powered by Dreditor.

Ok, here is a re-roll with that included in the docblock and to fix some offsets.

Applies cleanly. Confirmed this is the same fix as #19 plus the corrected docblock.

This looks good, although the code in forum_forum_load() could use some additional code comments. Care to try and add some?

Putting to needs work, please add some comments so we can clean up the issue queue.

Downgrading to normal and removing tags.

Whoops, sorry, thought this was commited and set to needs work for comments only...

Revised patch with code comments, and correct docblock. Functionally equivalent to #19.

whoops, forgot to remove the git prefix.

good catch.

+++ modules/forum/forum.module
@@ -719,27 +726,51 @@ function forum_form($node, $form_state) {
+ *    -num_topics Number of topics in the forum
+ *    -num_posts Total number of posts in all topics
+ *    -last_post Most recent post for the forum
+ *    -forums An array of child forums

Space between - and the key please. And add single quotes.

+++ modules/forum/forum.module
@@ -91,6 +91,14 @@ function forum_menu() {
+    'page arguments' => array(),

Unneeded

+++ modules/forum/forum.module
@@ -719,27 +726,51 @@ function forum_form($node, $form_state) {
+ *    -num_topics Number of topics in the forum
+ *    -num_posts Total number of posts in all topics
+ *    -last_post Most recent post for the forum
+ *    -forums An array of child forums

Space between '-' and the key please. And add single quotes.

Other than that, looks good.

incorporating feedback from dmitrig01 and pwolanin.

+++ modules/forum/forum.module
@@ -719,27 +725,54 @@ function forum_form($node, $form_state) {
+  if (is_null($tid)) {
+    $tid = 0;
+  }

This should be !isset().

Otherwise looks good.

104 critical left. Go review some!

changed to !isset().

Looks great.

The amount of code being changed here makes me go "EEEEE!" and think we probably need tests for this. But I talked it over with catch. It would be odd to test just one menu router path's security and not all of them, and it would also be odd to test that a function which only accepts an int as valid input only accepts an int as valid input... and we're not likely to break this again.

So therefore, committed to HEAD. Thanks a lot for your work on this folks! Great to see the number of known security holes coming down.

This fix leads to unavailability to fix #148145: "Forums" title is not localized
Because user can only rename /forum menu item and not /forum/%