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.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 2932196-sec-fix-crash-4.patch | 529 bytes | kscheirer |
Comments
Comment #2
rjlang commentedComment #3
rjlang commentedComment #4
kscheirerAttached 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.
Comment #5
nohup commentedPlease 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.
Comment #6
kscheirerThe 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.
Comment #7
rjlang commentedWhat 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).
Comment #8
nohup commentedI don't have anything in the logs.
Please provide your drupal version and the modules you have enabled.
Comment #9
kscheirerOk, 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: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?
Comment #10
rjlang commentedOK, 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.
Comment #11
kscheirerThat's great news! @nohup, do you think it's worth adding a cache clear or menu_rebuild in a hook update?
Comment #13
nohup commentedChanged committed. Releasing 7.x-1.4
Comment #14
rjlang commentedSo, 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.