[Security Bugs] CSRF + XSS in Change User Profile

[trichimtrich]

In Change User Profile function, there is no Old password to confirm change user password, and also no CSRF Token to protect CSRF malicious request. Reference Owasp.
So when the admin user access to malicious web, it will trigger to automatically change admin password to attacker's password.
Example request:

• http://localhost/slims8/admin/modules/system/app_user.php?changecurrent=true&action=detail&ajaxload=1

userName=admin
&realName=Admin
&userType=1
&eMail=
&social[fb]=
&social[tw]=
&social[li]=
&social[rd]=
&social[pn]=
&social[gp]=
&social[yt]=
&social[bl]=
&social[ym]=
&image=
&base64picstring=
&passwd1=trichimtrich
&passwd2=trichimtrich
&saveData=Update
&updateRecordID=1

This will change admin password to trichimtrich
And also, there is a stored XSS in here too. All the field realName, eMail, social[xx] have the same problem.
Sample request:
&realName=Admin" autofocus onfocus="alert(1)

PoC

Attacker can trigger admin to execute abitrary javascript to do anything.

[matlam]

Is anybody working on this?
As far as I know there is no protection against CSRF anywhere in slims. It would be good to add it to a central place(maybe in simbio) and use the CSRF-protection on all the forms

[dicarve]

Hi, We are planning to implement token for each form in Simbio for protection against CSRF, thank you for your suggestion Regards,

…

On Mon, Jun 5, 2017 at 3:05 AM, matlam ***@***.***> wrote: Is anybody working on this? As far as I know there is no protection against CSRF anywhere in slims. It would be good to add it to a central place(maybe in simbio) and use the CSRF-protection on all the forms — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#49 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHI6wBIymta71C8WpxCZccFxF0_2MRNks5sAw3vgaJpZM4NnilP> .

-- Open Source is about respecting other people creation and sharing knowledge to others. Sebaik baiknya manusia adalah yang bermanfaat bagi orang lain. http://dicarve.blogspot.com

[matlam]

Is there any progress on this or the other security bugs? Do you need any help?

[slims]

@trichimtrich and @matlam i have added CSRF token in Simbio Form Maker class in commit 2bc5e5e23926d855d997df2d4f40f3217f3a6a8c could you please help me check for it? thank you

[matlam]

@trichimtrich and @matlam i have added CSRF token in Simbio Form Maker class in commit 2bc5e5e could you please help me check for it? thank you

I'm not sure if I understand you correctly. Are you asking us to check if your implementation of a CSRF token is correct? Or do you want help in looking for places where a CSRF-Token is needed?

I had a quick look at the implementation and noticed two things:

1. it fails silently when neither random_bytes nor mcrypt_create_iv nor openssl_random_pseudo_bytes are available. That is probably unlikely, but throwing an exception or aborting the script with an error message is better IMHO.
2. there is only one token valid at the same time in the same session. This fails if the user opens another form in a new window or a page which has multiple forms(e.g. the member_card_generator which has the "Add To Print Queue"-form, the "Member card print settings"-form and the buttons "Clear Print Queue" and "Print Member Cards" which should ideally have CSRF-Protection as well).

[dicarve]

@matlam Yes i need help in testing the token implementation as i just only implement it on Bibliography form. Currently there is only one valid token in session, i will implement token form name to make it sure it will only validate the related form. Maybe i should try another hash mechanisme such as hash or sha512 function to generate random string

[matlam]

i will implement token form name to make it sure it will only validate the related form

that is better, but it still has a problem if a user opens the same form in two windows. I don't know what the best solution is. Maybe something like this:

    public static function generateCSRFToken($formName) {
        $token = self::genRandomToken();
        if (!isset($_SESSION['csrf_token'])) {
            $_SESSION['csrf_token'] = array();
        }
        if (!isset($_SESSION['csrf_token'][$formName])) {
            $_SESSION['csrf_token'][$formName] = array();
        }
        $_SESSION['csrf_token'][$formName][$token] = $token;
        return $token;
    }

    public static function isTokenValid($formName) {
        if (isset($_POST['csrf_token'])) {
            $token = $_POST['csrf_token'];
        } elseif (isset($_GET['csrf_token'])) {
            $token = $_GET['csrf_token'];
        } else {
            return false;
        }
        if (isset($_SESSION['csrf_token'][$formName][$token])) {
            unset($_SESSION['csrf_token'][$formName][$token]);
            return true;
        } else {
            return false;
        }
    }

But this has the disadvantage that sessions grow over time.

Maybe i should try another hash mechanisme such as hash or sha512 function to generate random string

For what purpose? A hash function in itself doesn't create a random string. You could generate one random value per session and use a cryptographic hash function to combine it with the form name. This is slightly less secure, but avoids the growing sessions. And it would allow the resending of the same form, but I'm not sure if that's a good or bad thing.

[nalamapu]

How it will be helpful if add a field of current password? If current password doesn't match, password will not be changed.

[matlam]

Is there any progress on this?