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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Active » Needs review
FileSize
857 bytes

Here's a simple fix in install.php. Are there any other vectors?

pwolanin’s picture

FileSize
861 bytes

now with 200% fewer typos in the code comments...

pwolanin’s picture

and... with some markup borrowed from the Apache 403 output.

chx’s picture

Now, 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.

pwolanin’s picture

chx - well, either way is ok with me. We could certainly delete the message and leave just the title and h1.

pwolanin’s picture

FileSize
981 bytes

less of a message, fixed one more typo.

pwolanin’s picture

FileSize
972 bytes

lowercase the tags, per current convention.

pwolanin’s picture

any feedback? this is a pretty trivial patch, and closes a critical sec hole.

dropcube’s picture

FileSize
845 bytes

I 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.

pwolanin’s picture

@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.

dropcube’s picture

Component: base system » install system

@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.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
865 bytes

Ok, 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

pwolanin’s picture

patch still applies cleanly

pwolanin’s picture

patch 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.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Looked at it again, tested it, and decided to commit it. Thanks pwolanin.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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