Security and administrative username

Posted: Wed Jan 02, 2008 9:40 am

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.

Posted: Wed Jan 02, 2008 11:07 am

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

Posted: Wed Jan 02, 2008 12:01 pm

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.

Posted: Wed Jan 02, 2008 4:07 pm

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

Posted: Wed Jan 02, 2008 6:24 pm

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

Posted: Thu Jan 03, 2008 7:03 am

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.

Posted: Thu Jan 03, 2008 7:56 am

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.

Posted: Thu Jan 03, 2008 8:57 am

fkelly wrote:

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

I didn't want to say it!

ROTFL

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?

Posted: Thu Jan 03, 2008 11:08 am

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

Posted: Thu Jan 03, 2008 12:01 pm

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.

Posted: Sun Jan 13, 2008 12:42 pm

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/>";

Posted: Mon Jan 14, 2008 12:26 pm

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.

Posted: Mon Jan 14, 2008 1:47 pm

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

Posted: Mon Jan 14, 2008 2:24 pm

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

)

Thanks!