Ravens PHP Scripts: Forums
 

 

View next topic
View previous topic
Post new topic   Reply to topic    Ravens PHP Scripts And Web Hosting Forum Index -> Security - PHP Nuke
Author Message
fkelly
Former Moderator in Good Standing


Joined: Aug 30, 2005
Posts: 3312
Location: near Albany NY

PostPosted: Wed Jan 02, 2008 9:40 am Reply with quote

In admin.php there is this code:

Code:
        if (isset($aid) && (ereg('[^a-zA-Z0-9_-]',trim($aid)))) {

           die('Begone');
        }


This is at line 82 of the RN 2.10.01 release. On my web site I originally had assigned usernames using email addresses (a mistake retrospectively) and thus when I made a few people admins I used their email address. Of course when they hit this section of code they get "begoned" because the eregi checks for both the . and @ characters.

For 2.20 I've been working on a replacement ereg that may look something like this:

Code:
      if (isset($aid) && (ereg('[^a-zA-Z0-9@_.]',trim($aid)))) {

         die('Begone');
      }


Subject to further testing. But in the back of my mind I keep having the question: why the heck are we doing this? First of all an administrative ID has to be created by the God admin. Second of all, if we are going to protect what ID goes in, that should be done in authors.php and not "retroactively" in admin.php. (OF course FB did this kind of thing all the time, doing intval on integer fields coming from the database for instance).

But most of all help me understand how a security risk can be injected into an admins name. We aren't going to execute the name at any point as a SQL statement are we? And while I'm asking, what the heck is with:

Code:
      if (isset($aid)) { $aid = substr($aid, 0,25);}

      if (isset($pwd)) { $pwd = substr($pwd, 0,40);}


This kind of substr happens all over Nuke, like in Your Account. Why do we need to substr it? And in admin.php of course this is about the third test of whether $aid is set but that's only nanoseconds I suppose.

I'm fairly new to all this and I'm really just trying to understand what the nature of the threats is and whether all this code is there for any good purpose.
 
View user's profile Send private message Visit poster's website
evaders99
Former Moderator in Good Standing


Joined: Apr 30, 2004
Posts: 3221

PostPosted: Wed Jan 02, 2008 11:07 am Reply with quote

Retroactive code patching without one specific purpose. I believe substr were added to enforce the character limit that either applied through the admin creation or the database anyway.

I agree - it's a mess. Many areas have variables that are filtered in different ways, its hard to keep track what's been done before. I would see about trying to fix that, but my guess is that it would be easier to overhaul the entire system and start from scratch Wink

_________________
- Only registered users can see links on this board! Get registered or login! -

Need help? Only registered users can see links on this board! Get registered or login! 
View user's profile Send private message Visit poster's website
Gremmie
Former Moderator in Good Standing


Joined: Apr 06, 2006
Posts: 2415
Location: Iowa, USA

PostPosted: Wed Jan 02, 2008 12:01 pm Reply with quote

About substr, I certainly don't know, other than in general it is a good idea to limit input from the user, as it can make it harder for them to inject SQL if they have less room to do it.

A lot of stuff in core nuke just doesn't make sense, and the reasons, if any, are now lost because of lack of comments or documentation.

Let me just comment on your "doing intval on integer fields coming from the database" comment. Yes, even though they are integer fields in the database, it is my understanding they are strings when you get them out through the PHP mysql library. So converting them to int will actually save you time if you plan on doing lots of math or comparisons with them later.

The mysqli library, available in PHP5, will do the conversions for you because you can tell it how to convert the types when you do sets and gets from the database.

_________________
Only registered users can see links on this board! Get registered or login! - An Event Calendar for PHP-Nuke
Only registered users can see links on this board! Get registered or login! - A Google Maps Nuke Module 
View user's profile Send private message
fkelly
PostPosted: Wed Jan 02, 2008 4:07 pm Reply with quote

I wouldn't propose eliminating the substr without a lot of testing first -- I guess it's a good line of defense though I think they'd probably get stopped by XSS checks before they were able to forge a form successfully. Still "thinking" and knowing are two different things.

It's more long term but I was just looking thru the logic (term applied loosely) of admin.php. Why in the name of ... we have that create_first function in there and all the checks for it is beyond me. Just create the GOD administrator as part of installation and be done with it. If God gets deleted the system shouldn't run anyway. I'd think it would be more secure to make God administration be dependent on (1) an installer type program that gets removed when God maintenance is over or (2) phpmyadmin where you'd need a separate ID and password to get at the table. Then treat that record within Nuke administration as basically read only.

And, yes, if you are doing a bunch of arithmetic perhaps it's faster to cast the variable as integer first. But most of the time in Nuke that's not what's done. In fact there are many many instances where a variable assignment isn't even needed. Nuke does something like:

[code]$var = intval($row['fieldx']);
echo $var;
[code]

instead of just echoing $row['fieldx'];

an unnecessary function AND an unnecessary variable assignment. Think of the nanoseconds and bytes of memory wasted. Smile
 
Gremmie
PostPosted: Wed Jan 02, 2008 6:24 pm Reply with quote

True. But if you used $var more than once it might be faster than doing a subscript lookup. But who knows without profiling. Speed obviously wasn't a concern with the original code. LOL. I think the original authors just did a "monkey see, monkey do" and always did database processing like that. Or maybe they just didn't like all the extra typing involved with the $row['fieldx'] form. Smile
 
evaders99
PostPosted: Thu Jan 03, 2008 7:03 am Reply with quote

Quote:
(1) an installer type program that gets removed when God maintenance is over or (2) phpmyadmin where you'd need a separate ID and password to get at the table


1) Wasn't available until 8.0 (even though there were other scripts to install the SQL data)
2) Relies on the ability of users to use phpMyAdmin, which they may have never used before. While it should be a staple for the average PHP coder, few users starting with phpNuke are those.

Not saying its a bad idea or anything. Just pointing out that is phpNuke is tied down to its history.
 
fkelly
PostPosted: Thu Jan 03, 2008 7:56 am Reply with quote

Both points true Evaders. I'm really talking about in the Ravennuke line of Nuke because I am not the slightest concerned with PHPnuke personally. I don't even look at it anymore. It would be pretty simple to write a script to do what the create first function does. Put it in your Nuke root, run it, create your God admin and get the script off your server. Thinking about it over night, it's just ridiculous that every time admin.php is run in the history of a Nuke system (and that's a lot because every administrative function goes thru it every time) there is a check to see if there are any records in the authors table and if not create_first would be run. I don't know why such things bug me so much, must be compulsive.
 
montego
Site Admin


Joined: Aug 29, 2004
Posts: 9449
Location: Arizona

PostPosted: Thu Jan 03, 2008 8:57 am Reply with quote

OffTopic

fkelly wrote:
I don't know why such things bug me so much, must be compulsive.


I didn't want to say it!

ROTFL Smack killing me

I think we would be better served to focus on tuning code and SQL for the functions that are more heavily used, such as content that is crawled and content that is accessed a lot. Why focus so hard on the admin functions? It will just drive you insane. Laughing

However, back to the original thread, we are questioning why to have the regex checks on what characters may be used in setting up a new admin. While I agree that the God admin is the one who must do this, I still think some minimal "cleansing" should be considered, just enough to ensure that the admin can log in and does not get "bitten" by any other code that is out there.

What I am concerned about are things like what we see with download/link titles with parenthesis in them (I know we got rid of this issue in RN, but you get the point) and NukeSentinel!

Unfortunately, I don't know what trouble might be lurking under the covers, so it is difficult for me to suggest what we should allow / not-allow. If I had to throw out a suggestion, it would be to possibly check for a "black list" rather than try and "white list" this thing. I would possibly stay away from allowing these characters:

& < > ' " %

And, maybe from a NukeSentinel standpoint, do we also black list:

( )

Thoughts?

_________________
Only registered users can see links on this board! Get registered or login!
Only registered users can see links on this board! Get registered or login! 
View user's profile Send private message Visit poster's website
evaders99
PostPosted: Thu Jan 03, 2008 11:08 am Reply with quote

What if you inherit a site and don't get that installer script?

I know its kinda a trivial issue, but for what's its worth, I can see why having just the admin page create a new admin could be beneficial. It's easy to use. Could hackers exploit it, sure. Do they? I see more attempts to create an admin using the regular script and a false admin cookie. Should we disable admin creation completely and move it to an installer script? Smile
 
fkelly
PostPosted: Thu Jan 03, 2008 12:01 pm Reply with quote

The installer script could be available on a downloads page. But really the bit about create_first is just a rumination and maybe something to consider for some future release as part of streamlining administrative functions. I was just looking at admin.php yesterday and thinking "what a mess" -- just following the logic is difficult and I was thinking "why do we even need that in there?" but like I say it's more a rumination than an immediate suggestion.

As to the eregi, right now I'm planning to allow @ . and _ (the @sign, a period and a underline) in addition to the characters a to z (case insensitive) and numbers 1 thru 9. Unless it problems show up in the testing or otherwise, that will go into Ravennuke 2.20. At some point in the future we could go to a black list rather than a white list, I agree that might make sense, but I am trying to minimize the changes in view of how imminent the release is.
 
Gremmie
PostPosted: Sun Jan 13, 2008 12:42 pm Reply with quote

fkelly wrote:
In fact there are many many instances where a variable assignment isn't even needed. Nuke does something like:

Code:
$var = intval($row['fieldx']);

echo $var;


instead of just echoing $row['fieldx'];

an unnecessary function AND an unnecessary variable assignment. Think of the nanoseconds and bytes of memory wasted. Smile


LOL, I just thought of why FB probably did it this way. As you guys probably know, this doesn't work in PHP:

Code:


echo "And the title is $row['title']<br/>";


FB didn't know about the curly brace idiom, or maybe it wasn't in PHP when he started. This would have worked:

Code:


echo "And the title is {$row['title']}<br/>";


Of course using single quotes and breaking up the string is the other option, but he didn't seem to do that too often.

So I think he arrived on this, and just did it all the time:

Code:


$title = $row['title'];
echo "And the title is $title<br/>";
 
montego
PostPosted: Mon Jan 14, 2008 12:26 pm Reply with quote

Gremmie, I believe your left-brace isn't in the right position. I think it needs to be this:


echo "And the title is ${row['title']}<br/>";

at least that is how I recall using it in the HTML Newsletter.
 
Gremmie
PostPosted: Mon Jan 14, 2008 1:47 pm Reply with quote

Montego, I'm not sure but I think either way will work, at least for the case of scalar variables.
Only registered users can see links on this board! Get registered or login!

Scroll down to the "Complex (curly) syntax".
 
montego
PostPosted: Mon Jan 14, 2008 2:24 pm Reply with quote

Ah, interesting, your format is actually more broadly applied, which is a good thing (the exceptions I have to remember the better Wink )

Thanks!
 
Display posts from previous:       
Post new topic   Reply to topic    Ravens PHP Scripts And Web Hosting Forum Index -> Security - PHP Nuke

View next topic
View previous topic
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
You can attach files in this forum
You can download files in this forum


Powered by phpBB © 2001-2007 phpBB Group
All times are GMT - 6 Hours
 
Forums ©