per SA-CORE-2009-007

The forum module does not check the incoming tid:

It appears that the forum module allows malicious HTML. Example:
http://drupal.org/?q=forum/pipes%22%3E%3Ch1%3Emalicious%20html%3Ctable

D6 patch was:

Index: modules/forum/forum.pages.inc
===================================================================
RCS file: /cvs/drupal/drupal/modules/forum/forum.pages.inc,v
retrieving revision 1.2
diff -u -p -r1.2 forum.pages.inc
--- modules/forum/forum.pages.inc 26 Jul 2007 06:48:03 -0000 1.2
+++ modules/forum/forum.pages.inc 16 May 2009 16:53:33 -0000
@@ -10,6 +10,11 @@
  * Menu callback; prints a forum listing.
  */
function forum_page($tid = 0) {
+  if (!is_numeric($tid)) {
+    return MENU_NOT_FOUND;
+  }
+  $tid = (int)$tid;
+
   $topics = '';
   $forum_per_page = variable_get('forum_per_page', 25);
   $sortby = variable_get('forum_order', 1);
Files: 
CommentFileSizeAuthor
#43 520736.patch6.85 KBgrendzy
PASSED: [[SimpleTest]]: [MySQL] 19,586 pass(es).
[ View ]
#41 520736.patch6.85 KBgrendzy
PASSED: [[SimpleTest]]: [MySQL] 19,589 pass(es).
[ View ]
#39 520736.patch6.88 KBgrendzy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 520736_3.patch.
[ View ]
#35 520736.patch6.76 KBgrendzy
PASSED: [[SimpleTest]]: [MySQL] 19,208 pass(es).
[ View ]
#33 520736.patch6.72 KBgrendzy
PASSED: [[SimpleTest]]: [MySQL] 19,200 pass(es).
[ View ]
#31 520736.patch6.75 KBgrendzy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 520736_0.patch.
[ View ]
#25 520736-25-forum-menu-load-crazyness.patch5.73 KBgreggles
PASSED: [[SimpleTest]]: [MySQL] 18,702 pass(es).
[ View ]
#19 520736-forum-menu-load-crazyness.patch5.22 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 17,635 pass(es).
[ View ]
#17 520736-validate-tid-forum-pages-inc-rev5.patch755 bytesbrianV
Passed on all environments.
[ View ]
#15 520736-validate-tid-forum-pages-inc-rev4.patch993 bytesbrianV
Failed on MySQL 5.0 ISAM, with: 15,570 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#13 520736-validate-tid-forum-pages-inc-rev3.patch963 bytesbrianV
Failed on MySQL 5.0 ISAM, with: 15,568 pass(es), 3 fail(s), and 0 exception(es).
[ View ]
#11 520736-validate-tid-forum-pages-inc-rev2.patch963 bytesbrianV
Failed on MySQL 5.0 ISAM, with: 15,513 pass(es), 39 fail(s), and 14 exception(es).
[ View ]
#10 520736-validate-tid-forum-pages-inc.patch967 bytesbrianV
Failed on MySQL 5.0 ISAM, with: 15,523 pass(es), 39 fail(s), and 14 exception(es).
[ View ]
#4 520736.patch793 byteskjy07
Failed on MySQL 5.0 ISAM, with: 15,580 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#1 520736-1-SA-CORE-2009-007.patch700 bytesJoshuaRogers
Passed: 12115 passes, 0 fails, 0 exceptions
[ View ]

Comments

StatusFileSize
new700 bytes
Passed: 12115 passes, 0 fails, 0 exceptions
[ View ]

Status:Patch (to be ported)» Needs review

I always forget to change the status.

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

StatusFileSize
new793 bytes
Failed on MySQL 5.0 ISAM, with: 15,580 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch failed testing.

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.

Status:Needs work» Needs review
Issue tags:-Security Advisory follow-up

brianV requested that failed test be re-tested.

Status:Needs review» Needs work
Issue tags:+Security Advisory follow-up

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new967 bytes
Failed on MySQL 5.0 ISAM, with: 15,523 pass(es), 39 fail(s), and 14 exception(es).
[ View ]

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.

StatusFileSize
new963 bytes
Failed on MySQL 5.0 ISAM, with: 15,513 pass(es), 39 fail(s), and 14 exception(es).
[ View ]

Err, now with whitespace fixes.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new963 bytes
Failed on MySQL 5.0 ISAM, with: 15,568 pass(es), 3 fail(s), and 0 exception(es).
[ View ]

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new993 bytes
Failed on MySQL 5.0 ISAM, with: 15,570 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new755 bytes
Passed on all environments.
[ View ]

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? :)

StatusFileSize
new5.22 KB
PASSED: [[SimpleTest]]: [MySQL] 17,635 pass(es).
[ View ]

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?

Status:Needs review» Needs work

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new5.73 KB
PASSED: [[SimpleTest]]: [MySQL] 18,702 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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?

Status:Reviewed & tested by the community» Needs work

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

Priority:Critical» Normal
Issue tags:-Security Advisory follow-up, -webchick's D7 alpha hit list

Downgrading to normal and removing tags.

Priority:Normal» Critical
Issue tags:+Security Advisory follow-up, +webchick's D7 alpha hit list

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

Status:Needs work» Needs review
StatusFileSize
new6.75 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 520736_0.patch.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 520736.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.72 KB
PASSED: [[SimpleTest]]: [MySQL] 19,200 pass(es).
[ View ]

whoops, forgot to remove the git prefix.

code style problem:

+  return $cache[$tid] = $forum_term;;

remove extra ; and usually we don't have assignment and return on the same line.

StatusFileSize
new6.76 KB
PASSED: [[SimpleTest]]: [MySQL] 19,208 pass(es).
[ View ]

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.

Status:Needs review» Needs work

this is a bit inconsistent:

-function forum_get_forums($tid = 0) {
+function forum_forum_load($tid) {
   $cache = &drupal_static(__FUNCTION__, array());
+  // Return a cached forum tree if available.
   if (isset($cache[$tid])) {
     return $cache[$tid];
   }
-  $forums = array();
   $vid = variable_get('forum_nav_vocabulary', 0);
+
+  // Load and validate the parent term.
+  if ($tid) {
+    $forum_term = taxonomy_term_load($tid);
+    if (!$forum_term || ($forum_term->vid != $vid)) {
+      return $cache[$tid] = FALSE;
+    }
+  }
+  // If $tid is 0, create an empty object to hold the child terms.
+  else if ($tid === 0) {
+    $forum_term = (object) array(
+      'tid' => 0,
+    );
+  }

versus:

+function forum_page($forum_term = NULL) {
+  if (!isset($forum_term)) {
+    // On the main page, display all the top-level forums.
+    $forum_term = forum_forum_load(0);
+  }

Status:Needs work» Needs review
StatusFileSize
new6.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 520736_3.patch.
[ View ]

incorporating feedback from dmitrig01 and pwolanin.

Status:Needs review» Needs work

The last submitted patch, 520736.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.85 KB
PASSED: [[SimpleTest]]: [MySQL] 19,589 pass(es).
[ View ]

+++ 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!

StatusFileSize
new6.85 KB
PASSED: [[SimpleTest]]: [MySQL] 19,586 pass(es).
[ View ]

changed to !isset().

Status:Needs review» Reviewed & tested by the community

Looks great.

Status:Reviewed & tested by the community» Fixed

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.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

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