[drupal-devel] [bug] 4.6.2 node.module incorrectly assumes 404 when access denied

[killes]

Issue status update for 
http://drupal.org/node/27864
Post a follow up: 
http://drupal.org/project/comments/add/27864

 Project:      Drupal
 Version:      4.6.2
 Component:    node.module
 Category:     bug reports
 Priority:     normal
 Assigned to:  willmoy
 Reported by:  willmoy
 Updated by:   killes at www.drop.org
 Status:       patch (code needs review)

I think the problem is this:
WHERE nid=%s', arg(1)


It should be


WHERE nid = '%s'", arg(1)


if arg(1) were indeed a string, but since it isn't it should be


WHERE nid = %d', arg(1)




killes at www.drop.org



Previous comments:
------------------------------------------------------------------------

Sat, 30 Jul 2005 19:58:15 +0000 : willmoy

To reproduce:
- Take a page which is denied to anonymous users by node_privacy_byrole
- Go to it as an anonymous user
- Receive 404 error


Note: this bug did not exist in 4.5.x




------------------------------------------------------------------------

Sat, 30 Jul 2005 20:23:02 +0000 : willmoy

Attachment: http://drupal.org/files/issues/27864-user.module-4.6.2.patch (686 bytes)

Tested patch against 4.6.2 branch attached.




------------------------------------------------------------------------

Sun, 31 Jul 2005 01:01:08 +0000 : willmoy

Attachment: http://drupal.org/files/issues/27864-node.module-4.6.2.patch (682 bytes)

New patch. Correctly handles both 403s and 404s. Adds an extra query to
verify which is happening.




------------------------------------------------------------------------

Sun, 31 Jul 2005 10:18:30 +0000 : Dries

That code is insecure and may lead to SQL injection attacks.




------------------------------------------------------------------------

Sun, 31 Jul 2005 19:09:15 +0000 : willmoy

The patch doesn't include enough context to show that the query is
subject to a check of is_numeric(arg(1)). I don't think this is
vulnerable to SQL injection, but I'd be grateful if someone else would
check and I apologise if I'm missing something.


The patched code in context:
case 'view':
      if (is_numeric(arg(1))) {
        $node = node_load(array('nid' => arg(1)),
$_GET['revision']);
        if ($node->nid) {
          drupal_set_title(check_plain($node->title));
          print theme('page', node_show($node, arg(2)));
        }
        else if (db_fetch_object(db_query('SELECT nid FROM
{node} WHERE nid=%s', arg(1)))) {
          drupal_access_denied();
        }
        else {
          drupal_not_found();
        }
      }
      break;


The cvs version of this patch http://drupal.org/node/27873 has the same
context.