This was originally submitted as a security issue, but i have assessed that there is no threat. reposting in the regular issue queue so that this can be handled properly.

Looks like my personal web site has been targeted by some hacker. For the past mount or so I have notice some PHP errors in my log.
First this is my configuration.
"Status report"
Drupal 5.7
Configuration file Protected
Cron maintenance tasks Last run 3 days 4 hours ago
You can run cron manually.
Database schema Up to date
Drupal core update status Up to date
File system Writable (public download method)
Module update status Up to date
MySQL database 4.1.20
PHP 4.3.11
PHP register globals Disabled
Unicode library PHP Mbstring Extension
Web server Apache/2.0.52 (CentOS)

"Available updates"
Last checked: 19 min 13 sec ago (Check manually)
Up to date
Drupal 5.7
Includes: System, Taxonomy, Book, Help, Block, Profile, Path, Menu, Filter, Comment, User, Node, Watchdog
Up to date
Gallery 5.x-2.0
Includes: Gallery Profile, Gallery
Up to date
OpenID 5.x-1.1
Includes: OpenID
Up to date
OpenID URL 5.x-1.2
Includes: OpenID URL
Up to date
Project 5.x-1.1
Includes: Project
Up to date
TinyMCE WYSIWYG Editor 5.x-1.9
Includes: TinyMCE
Up to date
Update status 5.x-2.2
Includes: Update status
Up to date
userlink 5.x-1.2
Includes: Userlink
Up to date
Views 5.x-1.6
Includes: Views, Views UI

I have traced the problem to Project Issue Tracking module. The hacker looks like is trying CSRF or XSS attack. I'm not a security expert so I can't identify exact vector of attack.
Every time the the hacker hits my site I get following log entries:
php 2008-02-04 07:17 You have an error in your SQL syntax; check the ... Anonymous
php 2008-02-04 07:17 implode(): Bad arguments. in ... Anonymous
The full text of messages are:
Type php
Date Monday, February 4, 2008 - 07:17
User Anonymous
Location http://sal-e.net/project/issues?projects=http%3A%2F%2Fwww.asigurareamea....
Referrer
Message implode(): Bad arguments. in /public/web/drupal/modules/node/node.module on line 525.
Severity error
Hostname 217.113.62.8

Type php
Date Monday, February 4, 2008 - 07:17
User Anonymous
Location http://sal-e.net/project/issues?projects=http%3A%2F%2Fwww.asigurareamea....
Referrer
Message You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1 query: SELECT n.nid, n.vid, n.type, n.status, n.created, n.changed, n.comment, n.promote, n.sticky, r.timestamp AS revision_timestamp, r.title, r.span, r.teaser, r.log, r.format, u.uid, u.name, u.picture, u.data FROM node n INNER JOIN users u ON u.uid = n.uid INNER JOIN node_revisions r ON r.vid = n.vid WHERE in /public/web/drupal/includes/database.mysql.inc on line 172.
Severity error
Hostname 217.113.62.8

I have disable the Project Issue Tracking module and the PHP errors stopped. I have examen my system and it does not appears to be compromised, but I not sure and I think that there is problem with the module. The URL input should not result in PHP error or invalid SQL query. So I need help to assess the problem.

CommentFileSizeAuthor
#4 bad_project_get_fix.patch617 byteshunmonk
#2 bad_project_get_fix.patch657 byteshunmonk

Comments

hunmonk’s picture

hunmonk commented

the SQL error generate above is coming from project issue, but not from the issue query. this section of code is causing the problem:

// Load active project
if (count($query->projects) == 1) {
  $project = node_load($query->projects[0]);
}

there is no policing of the value of $query->projects[0], so if a string is passed, then the node_load() throws the error listed above.

all database value are properly escaped, so there's no security issue here.

hunmonk’s picture

hunmonk commented
Status: Active » Needs review
StatusFileSize
new bad_project_get_fix.patch657 bytes

attached patch has been tested as working on 5.x-1.x and 5.x-2.x -- just simply using the same check as node_load() does on $params, so that we never even try to load a bad string.

this still leaves the string in the issue query itself, but it causes no harm or error, as it's properly escaped, and simply returns no rows to the table. given that this code is destined for the trashbin anyways, i think this patch does what we need for now.

aclight’s picture

aclight commented
Status: Needs review » Needs work

I tested the patch and it works as promised. One thing I noticed is that if you supply a project number that is not a valid project on the site but which is a valid node on the site, you'll get a page with "Issues for ", where is the title of the non-project node.

I don't believe this is a new bug, but I think it's easy to fix this here also. Just make the patch look like so:

   // Load active project
-  if (count($query->projects) == 1) {
+ if (count($query->projects) == 1 && is_numeric($query->projects[0]) && array_key_exists($query->projects[0], $projects)) {
     $project = node_load($query->projects[0]);
   }

I tested such a change and it worked for me. I don't have a good clean copy to roll a patch from right now, but I'll reroll this when I get the chance if hunmonk doesn't beat me to it.

hunmonk’s picture

hunmonk commented
Status: Needs work » Needs review
StatusFileSize
new bad_project_get_fix.patch617 bytes

attached solves all problems mentioned in a much cleaner way -- with a more restrictive node_load().

aclight’s picture

aclight commented
Status: Needs review » Reviewed & tested by the community

Tested and patch works as advertised. Code looks good as well.

dww’s picture

dww
we/he/they
commented

Yup, looks good to me, too Thanks, folks. Ship it! ;)

hunmonk’s picture

hunmonk commented
Status: Reviewed & tested by the community » Fixed

committed to 5.x-2.x, 5.x-1.x, 4.7.x-2.x, 4.7.x-1.x, deployed on d.o, sec.d.o

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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