Cleaning up pull request 68 - acli/ATutor GitHub Wiki
Since pull request 68 was still not integrated into trunk, I tried to do additional “tests” to see if my patch would work correctly under different values of magic_quotes_gpc.
After an hour, I could not notice anything difference between magic_quotes_gpc = on and magic_quotes_gpc = off. I never had to deal with magic_quotes_gpc and I had real trouble figuring out how to write a set of test cases that will replicate runtime behaviour, so I decided to take a different approach: Just look at the code and see if I could make any inferences out of it.
So I took a second look at the code in step2.php. The input field for “Database Password” was
<input type="text" name="db_password" id="pass" value="<?php echo stripslashes(htmlspecialchars($_POST['db_password'])); ?>" class="formfield" />
and my patched version was
addslashes(urldecode($db_pwd))
addslashes() is the reverse of stripslashes() and urldecode() is the reverse of htmlspecialchars(), but then I noticed that the nesting looked wrong: The reverse of stripslashes(htmlspecialchars()) is urldecode(addslashes($db_pwd)) and not addslashes(urldecode($db_pwd)).
Since it was addslashes() that actually fixed the bug, this suggests that addslashes(urldecode($db_pwd)) is correct, which in turn suggests that the original stripslashes(htmlspecialchars($_POST['db_password'])) was not. In particular, it looks logical to conclude that this stripslashes() call must be a no-op.
Adding print statements to write_config_file() shows that $db_pwd (which is just $_POST['step2']['db_password']) contains %-escaped apostrophes and backslashes. Which means that stripslashes() never had anything to strip out.
Actually this still looks wrong, because htmlspecialchars() does not produce %-escaped strings: That’s urlencode()’s job. I still haven’t found the real sequence of events that got us the strange $db_pwd value that write_config_file() is getting.
It is, however, probably a good idea to make sure that the real “special” HTML characters, in particular < and > are in fact properly escaped.
Testing it with yet another MySQL account showed that < and > are indeed correctly escaped.
A grep seems to suggest that urlencode() was called in ustep2.php, but ustep2 is not actually used. Digging deeper, it turns out that urlencode() is actually called in store_steps(), defined in install/include/common.inc.php which is included by the top-leve install/install.php.
We can now reasonably sure to say how $db_pwd got its strange value and why it is urldecode() and why it is just addslashes():
- User enters password in step2
- step2.php receives the password as a normal string
- step2 calls store_steps() which
- calls $stripslashes to counteract magic_quotes_gpc, and
- calls urlencode() to turn the original normal string into a %-escaped string
- This %-escaped string is what write_config_file() receives as $db_pwd
- To reverse the encode, we simply apply urldecode() to arrive at the original normal string
- Since store_steps() already applied $stripslashes, it is the real password that the user entered
- Since we are going to create something that PHP can interpret as a string literal, we call addslashes() (not $addslashes)