Here's how I think the bug is demonstrated:
1) I set the user agent in your browser to something like "simpletest1"
2) visit install.php of a D7 site
3) take control of the new installation
This worked for me today using Safari on my laptop. Seems to me like major security hole. I did bring up this concern at one point when the tests were getting added to core, but apparently whatever fix was in place got obliterated.
Karoly responded by e-mail that indeed there was a check for this at one point, but it's gone now. He suggests putting code in install.php to check for this situation, since the tests use install.inc not install.php.
Prior to that response I was brainstorming about possible ways to set a token and a unique key. On possibility would be to make sure the number has at least X digits (e.g. simpletest9999999999_) and then create an empty table with a name like simpletest9999999999_simpletest_token. The code in database.inc could check whether this table/token exists for the incoming user agent in order to validate that it's an expected/permitted simpletest?
An alternative (though I think this would be less secure) would be to write the allowed user agents (or a salted/hashed version to compare against?) to a file.
Comment | File | Size | Author |
---|---|---|---|
#12 | simpletest-user-258200-11.patch | 865 bytes | pwolanin |
#9 | simpletest-user-258200-9.patch | 845 bytes | dropcube |
#7 | simpletest-user-258200-7.patch | 972 bytes | pwolanin |
#6 | simpletest-user-258200-6.patch | 981 bytes | pwolanin |
#3 | simpletest-user-258200-3.patch | 1.04 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedHere's a simple fix in install.php. Are there any other vectors?
Comment #2
pwolanin CreditAttribution: pwolanin commentednow with 200% fewer typos in the code comments...
Comment #3
pwolanin CreditAttribution: pwolanin commentedand... with some markup borrowed from the Apache 403 output.
Comment #4
chx CreditAttribution: chx commentedNow, now. There is no way someone without sinister intentions get to this message so why bother with a nice message or a message at all? Just die.
Comment #5
pwolanin CreditAttribution: pwolanin commentedchx - well, either way is ok with me. We could certainly delete the message and leave just the title and h1.
Comment #6
pwolanin CreditAttribution: pwolanin commentedless of a message, fixed one more typo.
Comment #7
pwolanin CreditAttribution: pwolanin commentedlowercase the tags, per current convention.
Comment #8
pwolanin CreditAttribution: pwolanin commentedany feedback? this is a pretty trivial patch, and closes a critical sec hole.
Comment #9
dropcube CreditAttribution: dropcube commentedI confirm the bug, using Firefox with the User Agent Switcher add-on you can easily change the user agent, run install.php and take control of new installations.
Won't be better to move this check to
install_main
and just send a 403 header and exit, without message ? Here is the patch.Comment #10
pwolanin CreditAttribution: pwolanin commented@dropcube - I think chx is in favor of no message - I don't really care except that it's easier to debug with the message there.
I just figured we should exit as early as possible in the script if we encounter this.
Comment #11
dropcube CreditAttribution: dropcube commented@pwolanin:
install_main
function is the executable entry point of install.php. About the message, I don't care, is just to have the code as clean as possible.Comment #12
pwolanin CreditAttribution: pwolanin commentedOk, sure, maybe easier to follow what's going on this way. I re-rolled the patch just to remove the non-Unix line endings.
Tested it with Safari, and it works as expected
Comment #13
pwolanin CreditAttribution: pwolanin commentedpatch still applies cleanly
Comment #14
pwolanin CreditAttribution: pwolanin commentedpatch still applies cleanly.
Tests still run fine. Since install.php is never used or tested during test runs, this patch cannot cause existing tests to fail.
Comment #15
Dries CreditAttribution: Dries commentedLooked at it again, tested it, and decided to commit it. Thanks pwolanin.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.