The security fix in version 1.3 of Me Aliases crashes our website. Maybe others.

1.3 replaces this code in me_handler() (starting at line 168)

  $args = func_get_args();

  $parts = array_pop($args);
  $callback = array_pop($args);

with this:

  $args = func_get_args();

  $callback = array_shift($args);
  $parts = array_shift($args);

The problem is that when calling, e.g., /user/me, $args has this form:

array(
  <a copy of the $user object>,
  'user_view_page',
  array('user', '%me')),
)

The second element is destined for $callback; the last is destined for $parts.

Normally, after popping off $parts and $callback, the remainder of $args (which would contain just the $user object) is used in the last line of the function,

  return call_user_func_array($callback, $args);

The version 1.3 code completely scrambles this. It sets $callback to the $user object, sets $parts to the string 'user_view_page', and passes the array to call_user_func_array. Badness ensues.

The following seems to work, though.

(1) Replace that bit of code with this:

 $args = func_get_args();

  $front_arg = array_shift($args);
  $callback = array_shift($args);
  $parts = array_shift($args);

(2) And then change the last line of the function to

  return call_user_func_array($callback, array($front_arg));

This is working for us. I don't know how it would fare under other use cases of Me Aliases, but I'll put it out there for anyone else running into the same problem.

P.S. Filing this as a 7.1.x-dev version bug because there doesn't seem to be a 1.3 version tag yet.

CommentFileSizeAuthor
#4 2932196-sec-fix-crash-4.patch529 byteskscheirer

Comments

rjlang created an issue. See original summary.

rjlang’s picture

Version: 7.x-1.x-dev » 7.x-1.3
rjlang’s picture

Issue summary: View changes
kscheirer’s picture

Priority: Major » Critical
Status: Active » Needs review
Issue tags: +Needs security review
StatusFileSize
new529 bytes

Attached patch implements the code changes above. Bumping to critical since this seems to be affecting a fair number of sites. Added security review tag to get some more eyes on it and make sure we don't break the original 1.3 fix.

nohup’s picture

Please post the error log or steps to reproduce the error. I have used a fresh installation of drupal 7.56 and have not seen any crashes.

I am happy to apply this patch and make a new release but I want to reproduce the error before I do that.

kscheirer’s picture

The watchdog error logged was:
Warning: call_user_func_array() expects parameter 1 to be a valid callback, no array or string given in me_handler() (line 215 of (docroot)/modules/contrib/me/me.module).
This was happening when viewing user profile page.

rjlang’s picture

What kscheirer #6 reported is also what I saw in the watchdog log. Also:

Warning: Variable passed to each() is not an array or object in me_handler() (line 184 of (docroot)/sites/all/modules/me/me.module).

nohup’s picture

I don't have anything in the logs.

Please provide your drupal version and the modules you have enabled.

kscheirer’s picture

Priority: Critical » Normal
Status: Needs review » Needs work
Issue tags: -Needs security review

Ok, I think this is a false alarm. After a complete (or menu) cache clear, 7.x-1.3 is working as expected.

The issue is in me_menu_alter(), around line 320:

  $data['page arguments'] = array_merge(array($data['page callback'], $parts), $data['page arguments']);

The page arguments have been reversed from the order in 7.x-1.2, which is correct. But until you clear cache, this code is not executed, and the order remains the same as it was in 7.x-1.2, which leads to the errors we reported.

So it depends on how the update was performed. Possibly we could use a hook_update_N() to force a cache clear or menu_rebuild() like in me_update_7001().

@rjlang can you confirm a menu or full cache clear resolves this issue?

rjlang’s picture

OK, yes. If I replace 1.2 with 1.3, then clear the menu cache before trying to reload a me-containing URL, version 1.3 works properly. And if I replace 1.2 with 1.3 without rebuilding the menu cache, I get the WSOD.

kscheirer’s picture

Title: Security fix v1.3 causes crash » 7.x-1.3 update requires cache clear
Category: Bug report » Task
Status: Needs work » Active

That's great news! @nohup, do you think it's worth adding a cache clear or menu_rebuild in a hook update?

  • nohup committed 07008d0 on 7.x-1.x
    Issue #2932196 by rjlang, kscheirer: Added hook_update_N to rebuild...
nohup’s picture

Status: Active » Fixed

Changed committed. Releasing 7.x-1.4

rjlang’s picture

So, I tried out the 1.4 version. After installing 1.4 over a preexisting 1.2 installation and then immediately running update.php, I did get two warnings on the update.php page and corresponding warnings in the dblog:

Warning: Variable passed to each() is not an array or object in me_handler() (line 184 of (docroot)/sites/all/modules/me/me.module).
Warning: call_user_func_array() expects parameter 1 to be a valid callback, no array or string given in me_handler() (line 214 of (docroot)/sites/all/modules/me/me.module).

However, I was still able to run update.php, and after the update, things worked fine with no more warnings.

Status: Fixed » Closed (fixed)

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