Skip to main content ↑ ↓
index.php oddities Started by Nao · · Read 6219 times 0 Members and 1 Guest are viewing this topic. previous topic - next topic

index.php oddities

Some of these things date back to early SMF days, and may not be 'useful' any more. I'm in the process of rewriting some stuff in Wedge's index.php file, and compared it with ElkArte's, and noticed some things that just seem odd to me. (Not all of these are in Wedge at this point, and it still seems to work fine... :P)

Code: [Select]
if (function_exists('set_magic_quotes_runtime'))
	@set_magic_quotes_runtime(0);

IIRC, some PHP might give an error message, even with the @ set. My code for this adds a version_compare.

Code: [Select]
// We don't need no globals.
foreach (array('db_character_set', 'cachedir') as $variable)
	if (isset($GLOBALS[$variable]))
		unset($GLOBALS[$variable], $GLOBALS[$variable]);

I remember seeing a discussion long ago that tried to explain why a variable needed to be unset twice (!), but that still seems completely crazy to me...
Also, the whole point of this..? Unsetting $GLOBALS['db_character_set'] (and the other), to me, is exactly the same as unsetting $db_character_set at this point. Not only that, but because Settings.php overrides any existing variables, I really don't see a need to unset them first...
And finally: why these variables, and not other globals..? You might as well go through $GLOBALS, and unset anything that doesn't start with an underscore, or that isn't "GLOBALS".

Code: [Select]
if (!file_exists($boarddir) && file_exists(dirname(__FILE__) . '/agreement.txt'))
	$boarddir = dirname(__FILE__);

Why agreement.txt..? To me, it seems likelier for an admin to remove that file, than for instance, SSI.php... (Which is why I changed that.)

Code: [Select]
// Pre-dispatch
elk_main();

Well, it's no longer a 'pre'-dispatch, since the function is called from within that... In fact, it looks like you guys simply forgot to remove that function. Just move its contents in place of the call, and you can throw away the 4 globals as a bonus.

Just my 2 cents! ;)

Re: index.php oddities

Reply #1 –

Also... This is a bit of a shot in the dark, but considering most developers here used to be SMF devs... Does anyone have a local copy of the SMF repo before it was transferred to GitHub..?
My problem is that I don't, and the last copy of the SMF codebase I have is from Wedge's first revision (equivalent to SMF's revision 10068, August 2010), after which we quickly diverged from the SMF codebase, and taking different routes doesn't mean Wedge's way of doing was the optimal one, and I'd like to be able to check what changes were made to the SMF codebase between August 2010 and revision 10610 in January 2012, according to the GitHub repo. That's over 500 commits I've never had the opportunity to explore.

To be specific, the latest thing that prompted me to miss these commits is index.php (otherwise I wouldn't be posting here). There's a block of code that was in 10068 and still is in Wedge, but that isn't in 10610. That block takes care of redirecting spiders to a PHPSESSID-free URL if possibly_robot is found, to help prevent Google from indexing URLs with a PHPSESSID in the URL. The fact is that this problem mostly went away with the advent of the rel=canonical feature, but there are still many pages in the SMF codebase that don't provide a canonical URL, as far as I know. Or maybe that was changed...
So, in case no one has a copy of the repo around (for which I'd be very grateful), I'd be thrilled to learn if anyone knows if the block was removed specifically because of rel=canonical, and if yes, if it helped fight PHPSESSID problems entirely.

Thanks :)

Re: index.php oddities

Reply #2 –

Another one...

SMF 2.1 and ElkArte still render an empty GIF image (encoded in index.php) when requesting a keepalive.
Because the keepalive image is generated via a "new Image()" call, and never actually added to the DOM, there are no visual issues (e.g. IE's infamous "X" box) related to receiving an incorrect image. Because the goal is only to keep the session active (and quite honestly, I don't know if there's a point to that anymore, even the keepalive JS code mentions it's a Firefox bug but we're talking about years ago...), there's no reason to do anything once the session has been loaded and it's been determined that the action is a keepalive. Thus, Wedge just exits the script, and I think you guys should do the same, because it saves both the server and the user some useless bandwidth use. ;) Don't know what you think about that.

(Basically, I'm doing a tabula-rasa review of both Elk's and Wedge's index files, and determine what's a hack for some old outdated era, what's a fix that makes no sense in addition to other existing fixes, etc.)

Re: index.php oddities

Reply #3 –

Quote from: Nao – Some of these things date back to early SMF days, and may not be 'useful' any more. I'm in the process of rewriting some stuff in Wedge's index.php file, and compared it with ElkArte's, and noticed some things that just seem odd to me. (Not all of these are in Wedge at this point, and it still seems to work fine... :P)

Code: [Select]
if (function_exists('set_magic_quotes_runtime'))
 @set_magic_quotes_runtime(0);

IIRC, some PHP might give an error message, even with the @ set. My code for this adds a version_compare.
1.1 will be php 5.3+ "strict", so it will be removed, for the moment we are still kind of covering 5.2 as well, so better keep it unless it ends up badly broken. I think.

Quote from: Nao –
Code: [Select]
// We don't need no globals.
foreach (array('db_character_set', 'cachedir') as $variable)
 if (isset($GLOBALS[$variable]))
 unset($GLOBALS[$variable], $GLOBALS[$variable]);

I remember seeing a discussion long ago that tried to explain why a variable needed to be unset twice (!), but that still seems completely crazy to me...
Also, the whole point of this..? Unsetting $GLOBALS['db_character_set'] (and the other), to me, is exactly the same as unsetting $db_character_set at this point. Not only that, but because Settings.php overrides any existing variables, I really don't see a need to unset them first...
And finally: why these variables, and not other globals..? You might as well go through $GLOBALS, and unset anything that doesn't start with an underscore, or that isn't "GLOBALS".
This was the exploit:
http://www.securityfocus.com/bid/51001/exploit
In fact it was a pretty nasty bug because $db_character_set was used directly into a query (SET NAMES).
For Elk is less of an issue because $db_character_set is basically unused, so it could be dropped.

I assume cache_dir was added because:
  • it may or may not be set,
  • if it is not set, using the vulnerability it may be possible to se any kind of path as cache and that could open the forum to other vulnerabilities.

That said, the vulnerability seems to affect some old php versions (< 5.1.4), so it should be safe to drop it.
But then again, index.php will undergo some huge changes in 1.1 so I'd better wait until then to get rid of that one.

Quote from: Nao –
Code: [Select]
if (!file_exists($boarddir) && file_exists(dirname(__FILE__) . '/agreement.txt'))
 $boarddir = dirname(__FILE__);

Why agreement.txt..? To me, it seems likelier for an admin to remove that file, than for instance, SSI.php... (Which is why I changed that.)
Well, first because SSI may or may not be there.
Of course agreement.txt may as well be there or not, but is the only one that if required shall be there, all the others may be somewhere else even if used.

TBH, I would get rid of all these checks altogether.
I usually prefer to have my settings right and not rely on guesses. Dunno if now is the moment to do that...

Quote from: Nao –
Code: [Select]
// Pre-dispatch
elk_main();

Well, it's no longer a 'pre'-dispatch, since the function is called from within that... In fact, it looks like you guys simply forgot to remove that function. Just move its contents in place of the call, and you can throw away the 4 globals as a bonus.
That will likely change quite a bit in the next version, but indeed the comment is a bit misleading... lol

On the gif... no idea at the moment.
Bugs creator.
Features destroyer.
Template killer.

Re: index.php oddities

Reply #4 –

This is called from index.php: loadBadBehavior
Code: [Select]
	elseif (in_array($override, array('SAMEORIGIN', 'DENY', 'SAMEORIGIN')))
It's testing for SAMEORIGIN twice... Bug ;)
Might as well just do == tests, for two tests that's much faster than in_array!

Another bug, unrelated to index.php: "Drag and Drop files here" goes out of bounds on a mobile device. Just shrink down your window, you'll see what I mean!

Quote from: emanuele – 1.1 will be php 5.3+ "strict", so it will be removed, for the moment we are still kind of covering 5.2 as well, so better keep it unless it ends up badly broken. I think.
Wedge is PHP 5.3+, but I'm still leaving this in, because magic quotes are only 'deprecated' in 5.3, not removed. It's only in 5.4 that it can cause problems, which is why I added a version compare.
Hmm, okay, I just checked (remembered my current local server is 5.5.6), and it's working fine without the version check, so I'll just leave my check out now.

Quote from: emanuele – This was the exploit:
http://www.securityfocus.com/bid/51001/exploit
Oh.. Good, thanks!
http://www.hardened-php.net/hphp/zend_hash_del_key_or_index_vulnerability.html
I'm finally getting the reason why SMF used to do unsets twice in a few places. I actually removed the HTTP_POST_VARS duplicate back in January 2011, with an ironic comment about SMF... Arantor was the security guy at Wedge and his departure left me with the task of ensuring it was still good, but from what I can see, his work wasn't that good either since this happened under his watch... :P
Anyway, I won't put the blame on anyone, because Wedge is protected against this now that it's PHP 5.3+ only. (And even before, when it was PHP 5.2+ only. But back in January 2011, I think it was PHP 5.1+ only, so some setups might have reproduced the problem, I suppose.)
For the record, the bug was fixed in PHP 5.1.4. Oh, I see you mentioned it below. But it's good to mention it as much as possible. :P
Anyway, that means you can safely remove duplicate unsets from ElkArte, as well as the db_character_set unset, because of it being UTF8-only.

See? I'm doing my share of the work! :P

Quote from: emanuele – TBH, I would get rid of all these checks altogether.
It's tempting, yes.
It's easier for me because from now on, Wedge uses hard-coded paths, because it just makes life gently caress easier for forum transitions (changing URLs without having to update 17 admin settings or whatever.)

Quote from: emanuele – That will likely change quite a bit in the next version, but indeed the comment is a bit misleading... lol

On the gif... no idea at the moment.
Feel free to comment further later! But I can swear to you, it won't disrupt the feature if you remove it. ;)

So, I take you don't have a copy of the SMF SVN repo with you? :P Maybe Spuds, then..?

Re: index.php oddities

Reply #5 –

Actually I have both the SVN and the git version of the SMF repo, I just use them when I have to find the reason of something odd in the code. O:-) (And usually it doesn't give much help... LOL)
Bugs creator.
Features destroyer.
Template killer.

Re: index.php oddities

Reply #6 –

Wouldn't it be a much smaller target if you only looked for things that weren't odd? :D
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: index.php oddities

Reply #7 –

P p p please share!!!
I only have the svn copy of SMF, I.e. without the repo files allowing for local browsing of old commits.

I can't believe I'm actually gonna say this, but... svn sucks!! I used to hate using svn blame because it did a server request and it was so slow. Having a local repo is so much better for blaming!

Re: index.php oddities

Reply #8 –

Most of the unsets have to do with memory usage, not security.

Re: index.php oddities

Reply #9 –

Quote from: Nao – P p p please share!!!
I only have the svn copy of SMF, I.e. without the repo files allowing for local browsing of old commits.
If you have the SVN one you can convert it! ;D

Quote from: Nao – I can't believe I'm actually gonna say this, but... svn sucks!! I used to hate using svn blame because it did a server request and it was so slow. Having a local repo is so much better for blaming!
blame is one of the functions I don't use that much, mainly because looking in SMF history you always hit a wall (between RC4 and RC5) of the last dos2unix conversion (and I think there are a couple), so it's mostly useless... :-\
Bugs creator.
Features destroyer.
Template killer.

Re: index.php oddities

Reply #10 –

- I'm interested in rev 10000-10610, ie the ones after I got kicked out of the SMF team. It's the missing link: I never got to see them... so pretty please ;)

- I had a similar problem with the Wedge codebase. What I did was simply to filter-branch and squash commits that were fixing crlf problems. Wedge's svn had a dozen like these. It took me a couple of days to remove them from git.

Anyway with tortoisegit and problematic revisions, you'll simply hit a wall, and then click Blame again until you find the proper commit. I don't know if other tools are as proficient as handling blame. I know I'm a tortoisegit diehard addict. That, and BeyondCompare for file comparisons. Much better than default tools... really. Helps with productivity.

Hope it was helpful. I'm a bit drunk right now.

Re: index.php oddities

Reply #11 –

<EDIT> Woops, just noticed your PM, so I'm editing my request out. Thanks :)

Also, to stay on topic: today I made various tests between PHP 5.3 & 5.4, with and without the function_exists, the version_compare and @ on the magic quotes code.

- Doing 1000 iterations or more, I get very different results from each combination.
- Doing only one iteration, with an efficient microsecond timer, I can see that whatever the combination is, it's always, err... Super fast. And varies by a factor of 2 across page reloads, just like the combinations give performance varying by a factor of 2, so basically, they're even.
- The fastest way is to simply do the version_compare first. If it's 5.4, the function is deprecated AND magic quotes are always off, so it's safe to just skip the rest, including the function_exists.
- The second fastest ways is to remove the '@', which actually slows down the instruction by a factor of 3, even if it doesn't generate a warning. Yup... But, since it's outside of my control, I'm just keeping the original code, and simply adding the version_compare at the very beginning. Simpler.
Thus, I'd recommend you do the same, because PHP 5.4+ is here to stay, innit..?