[Official BrainCMS] 1.4.0

Status
Not open for further replies.

Sledmore

Chaturbate Livestreamer
Staff member
FindRetros Moderator
Jul 24, 2010
5,194
3,901
Nice share, and thanks for contributing to the community.

From seeing a lot of users using this I decided to dive in and take a look at it, I'm not hating or anything - just if I were to use a CMS for a hotel I'll look for certain aspects, which is what I looked for in this.

--

Haven't had all the time to go through it, but wanted to debug it and take a dive in. I like the way you import some of the missing tables if they don't exist, but don't you think you should suggest to the user that they can remove those exists queries afterwards? Because of this there are 8 queries additional per page (nothing to shout about, just if they're not needed, they can go IMO).

H89Eaau.png


Logging in to the CMS, the me page seems a little bit mental, 34 queries (not counting room count & user count as I had to disable the ajax for that).

MSU9izp.png

So in theory, 36 queries.

Don't get me wrong, it runs fast enough but I don't see anything in this CMS that wouldn't make me just slap PDO on Rev and continue using that.

As I said above, I haven't gone all the way through but there is a lot to improve IMO. Also just to be cautious - make sure PHPmailer is up to date (exploit was recently found afaik).
 

BIOS

ಠ‿ಠ
Apr 25, 2012
906
247
Nice share, and thanks for contributing to the community.

From seeing a lot of users using this I decided to dive in and take a look at it, I'm not hating or anything - just if I were to use a CMS for a hotel I'll look for certain aspects, which is what I looked for in this.

--

Haven't had all the time to go through it, but wanted to debug it and take a dive in. I like the way you import some of the missing tables if they don't exist, but don't you think you should suggest to the user that they can remove those exists queries afterwards? Because of this there are 8 queries additional per page (nothing to shout about, just if they're not needed, they can go IMO).

H89Eaau.png


Logging in to the CMS, the me page seems a little bit mental, 34 queries (not counting room count & user count as I had to disable the ajax for that).

MSU9izp.png

So in theory, 36 queries.

Don't get me wrong, it runs fast enough but I don't see anything in this CMS that wouldn't make me just slap PDO on Rev and continue using that.

As I said above, I haven't gone all the way through but there is a lot to improve IMO. Also just to be cautious - make sure PHPmailer is up to date (exploit was recently found afaik).
@Sledmore What tool is that you're using for profiling? :p

36 queries is bad, 36 queries with 33 of those reoccurring on a single page is dreadful and won't last long if the hotel grows. All of that could be done in like a couple queries at the most, don't understand why it's selecting * either as that's also just a waste of resources.

Will probably have a look through this myself, although from what I've seen looks extremely messy and inefficient.
 

MayoMayn

BestDev
Oct 18, 2016
1,423
683
@Sledmore What tool is that you're using for profiling? [emoji14]

36 queries is bad, 36 queries with 33 of those reoccurring on a single page is dreadful and won't last long if the hotel grows. All of that could be done in like a couple queries at the most, don't understand why it's selecting * either as that's also just a waste of resources.

Will probably have a look through this myself, although from what I've seen looks extremely messy and inefficient.
Never understood neither why so many people select all instead of the columns they need, even the MySQL manual forbid it. Waste of resources too.

Sent from my SM-G928F using Tapatalk
 

BIOS

ಠ‿ಠ
Apr 25, 2012
906
247
So, had a quick look through a couple classes; one thing that popped out to me more than anything else.

system\app\classes\functions.php
PHP:
    function checkCloudflare()
    {
        if (isset($_SERVER["HTTP_CF_CONNECTING_IP"]))
        {
            $_SERVER['REMOTE_ADDR'] = $_SERVER["HTTP_CF_CONNECTING_IP"];
            return $_SERVER['REMOTE_ADDR'];
        }
        else
        {
            return $_SERVER['REMOTE_ADDR'];
        }
    }
The checkCloudflare() function checks if HTTP_CF_CONNECTING_IP exists, if it does then REMOTE_ADDR is updated with it's value. No IP validation is in place at all here, it gets worse.

system\app\classes\class.user.php::register()

I'd copy the entire function but it's so messy you probably wouldn't be able to pinpoint the issue, so i'll post key snippets.

line 197:
PHP:
$stmt = $dbh->prepare("SELECT ip_reg FROM users WHERE ip_reg = '".checkCloudflare()."'");
$stmt->execute();


line 212:
PHP:
$addNewUser = $dbh->prepare("
INSERT INTO
users
(username, password, rank, motto, account_created, mail, look, ip_last, ip_reg, credits, activity_points, vip_points)
VALUES
(
:username,
:password,
'1',
:motto,
'".strtotime("now")."',
:email,
:avatar,
'".checkCloudflare()."',
'".checkCloudflare()."',
:credits,
:duckets,
:diamonds
)");
$addNewUser->bindParam(':username', $_POST['username']);
$addNewUser->bindParam(':password', $password);
$addNewUser->bindParam(':motto', $motto);
$addNewUser->bindParam(':email', $_POST['email']);
$addNewUser->bindParam(':avatar', $avatar);
$addNewUser->bindParam(':credits', $config['credits']);
$addNewUser->bindParam(':duckets', $config['duckets']);
$addNewUser->bindParam(':diamonds', $config['diamonds']);
$addNewUser->execute();
Alright so it's using PDO, in most cases it's preparing the data before performing the actual query but it just so happens that data which can be spoofed (The IP) isn't prepared nor validated, bingo! SQL Injection.

This is a severe issue caused due to bad practice, this could've easily been avoided if the developer had utilized prepared statements throughout and validated input; no matter what input it is, it should be validated before being processed.

That'd be it for me, I don't know about anyone else thinking about using this CMS; keep in mind I only looked through a couple classes so there's potentially more lurking around. If you really do want to use this CMS, I'd recommend being cautious and definitely looking through it yourself first.
 

MayoMayn

BestDev
Oct 18, 2016
1,423
683
So, had a quick look through a couple classes; one thing that popped out to me more than anything else.

system\app\classes\functions.php
PHP:
    function checkCloudflare()
    {
        if (isset($_SERVER["HTTP_CF_CONNECTING_IP"]))
        {
            $_SERVER['REMOTE_ADDR'] = $_SERVER["HTTP_CF_CONNECTING_IP"];
            return $_SERVER['REMOTE_ADDR'];
        }
        else
        {
            return $_SERVER['REMOTE_ADDR'];
        }
    }
The checkCloudflare() function checks if HTTP_CF_CONNECTING_IP exists, if it does then REMOTE_ADDR is updated with it's value. No IP validation is in place at all here, it gets worse.

system\app\classes\class.user.php::register()

I'd copy the entire function but it's so messy you probably wouldn't be able to pinpoint the issue, so i'll post key snippets.

line 197:
PHP:
$stmt = $dbh->prepare("SELECT ip_reg FROM users WHERE ip_reg = '".checkCloudflare()."'");
$stmt->execute();


line 212:
PHP:
$addNewUser = $dbh->prepare("
INSERT INTO
users
(username, password, rank, motto, account_created, mail, look, ip_last, ip_reg, credits, activity_points, vip_points)
VALUES
(
:username,
:password,
'1',
:motto,
'".strtotime("now")."',
:email,
:avatar,
'".checkCloudflare()."',
'".checkCloudflare()."',
:credits,
:duckets,
:diamonds
)");
$addNewUser->bindParam(':username', $_POST['username']);
$addNewUser->bindParam(':password', $password);
$addNewUser->bindParam(':motto', $motto);
$addNewUser->bindParam(':email', $_POST['email']);
$addNewUser->bindParam(':avatar', $avatar);
$addNewUser->bindParam(':credits', $config['credits']);
$addNewUser->bindParam(':duckets', $config['duckets']);
$addNewUser->bindParam(':diamonds', $config['diamonds']);
$addNewUser->execute();
Alright so it's using PDO, in most cases it's preparing the data before performing the actual query but it just so happens that data which can be spoofed (The IP) isn't prepared nor validated, bingo! SQL Injection.

This is a severe issue caused due to bad practice, this could've easily been avoided if the developer had utilized prepared statements throughout and validated input; no matter what input it is, it should be validated before being processed.

That'd be it for me, I don't know about anyone else thinking about using this CMS; keep in mind I only looked through a couple classes so there's potentially more lurking around. If you really do want to use this CMS, I'd recommend being cautious and definitely looking through it yourself first.
Everybody is making the same mistake of using HTTP_CF_CONNECTING_IP.
If people just bothered to research, they would know its the wrong way, they should instead check whether or not HTTP_(X_)FORWARDED is set like so:
PHP:
public function getIP() {
        $ip = $_SERVER['REMOTE_ADDR'];
        if(isset($_SERVER['HTTP_FORWARDED'])) {
            $ip = $_SERVER['HTTP_FORWARDED'];
        } elseif(isset($_SERVER['HTTP_X_FORWARDED'])) {
            $ip = $_SERVER['HTTP_X_FORWARDED'];
        }
        return $ip;
    }
 

BIOS

ಠ‿ಠ
Apr 25, 2012
906
247
Everybody is making the same mistake of using HTTP_CF_CONNECTING_IP.
If people just bothered too look, they should use a script that checks whether or not HTTP_(X_)FORWARDED is set like so:
PHP:
public function getIP() {
        $ip = $_SERVER['REMOTE_ADDR'];
        if(isset($_SERVER['HTTP_FORWARDED'])) {
            $ip = $_SERVER['HTTP_FORWARDED'];
        } elseif(isset($_SERVER['HTTP_X_FORWARDED'])) {
            $ip = $_SERVER['HTTP_X_FORWARDED'];
        }
        return $ip;
    }
I'd recommend validating that it's actually an IP also if you want to do it that way, like so:
PHP:
public function getIP(){

    if(isset($_SERVER['HTTP_FORWARDED'])){

        if(filter_var($_SERVER['HTTP_FORWARDED'], FILTER_VALIDATE_IP)){
            // any other validation checks
            return $_SERVER['HTTP_FORWARDED'];
        }

    }
    // handle standard REMOTE_ADDR as the ip instead, etc.
}
 

MayoMayn

BestDev
Oct 18, 2016
1,423
683
I'd recommend validating that it's actually an IP also if you want to do it that way, like so:
PHP:
public function getIP(){

    if(isset($_SERVER['HTTP_FORWARDED'])){

        if(filter_var($_SERVER['HTTP_FORWARDED'], FILTER_VALIDATE_IP)){
            // any other validation checks
            return $_SERVER['HTTP_FORWARDED'];
        }

    }
    // handle standard REMOTE_ADDR as the ip instead, etc.
}
That is plain dumb. The IP will only get returned IF HTTP_FORWARDED is set. Do you you even know what it does?

Nevermind, didnt bother to read your quoted comment since I just woke up lol

Sent from my SM-G928F using Tapatalk
 
Last edited:

BrainCMS

Brain is live, Live is Brain
Feb 17, 2014
86
32
Update 1.7.0
Code:
Update the plugins:

    Userlike.php // You can now enable or disable it
    simpleFriendsHome.php //language changed
    simpleFriends.php // Add images
    newscomment.php // You can now enable or disable it
    habbooftheweek.php //online status and badge

Add new plugin:

    wordfilter.php // Filter plugin use te tabels wordfilter and wordfilter_characters

Update Classes:

    NON

Update \templates\brain

    news.php // Edit php to enable or disable the news commands
    roomcount.php // If there are no users in a room, You get to see that
    me.php // Edit the group display, If there are no groups You get to see that
    home.php // Edit language

Add new config options:
 
    $config['userLikeEnable'] = true;
    $config['newsCommandEnable'] = true;
    $config['newsCommandFilter'] = true;


Adminpan full EN!


New table in the database

    wordfilter_characters
Download link:
 

Ian2456

Member
Feb 19, 2013
275
7
Press F12 to open up the Console Log when on the client page.
Says Failed {link}/swfs/gordon/{production version}/Habbo.swf to load resource: the server respond with a status of 404 (Not found)

Everything is correct.

Sent from my SM-G900V using Tapatalk
 

MayoMayn

BestDev
Oct 18, 2016
1,423
683
Says Failed {link}/swfs/gordon/{production version}/Habbo.swf to load resource: the server respond with a status of 404 (Not found)

Everything is correct.

Sent from my SM-G900V using Tapatalk
Obviously its not correct. There's no variable called link or product version.
Switch link out with url and product version out with the swf version inside your gordon folder.
 
Status
Not open for further replies.

Users who are viewing this thread

Top