UserSpice 4.2 Bugs and security

[SavageStyle]

Here is few bugs I have found so far, may be I will remember some more that I patched for myself, so here is what i've found:

found bugs:

1) http://localhost/usercontrol/users/admin_user.php?id=2
not using global settings for name length

Code:

line 57, 58

should be changed to
<pre>

Code:

'min' => $settings->min_un,
'max' => $settings->max_un

</pre>



2)

Code:

function display_errors($errors = array())

- helpers.php - remove

Code:

echo "<br>"

- that br resulting content to slip below on every page that function called

3) http://localhost/usercontrol/users/admin...ssions.php

Code:

line 97,98

- dublicate of
<pre>

Code:

$errors = [];
$successes = [];

</pre>


that decleared above - that 2 lines should be removed, preventing any message to appear

4) http://localhost/usercontrol/users/admin...ssions.php

Code:

line 65

- echoing error to nowere, appearing above in black ugly block

Code:

echo "Permission Updated";

should be

Code:

$successes[] = 'TEXT';

5) http://localhost/usercontrol/users/profile.php?id=0
giving wrong id, resulting in banch of errors

Security issues:
1) http://localhost/usercontrol/users/admin.php
2 forms have no csrf protection - adding session token will solve that (i am using both session and per-request tokens)

2) http://localhost/usercontrol/users/admin...n.php?id=1

Code:

line 28

, value of

Code:

$_GET

is not sanitized

3) user name validation - user can create crazy names like <script>lala - should not allow that

That may be not bugs but logic flaws:
1) http://localhost/usercontrol/users/joinThankYou.php - should redirect if logged in?
2) http://localhost/usercontrol/users/join.php - should redirect if logged in?
3) http://localhost/usercontrol/users/login.php - should redirect if logged in?
4) http://localhost/usercontrol/users/maintenance.php - should redirect if no maintenance?

[Brandin]

Hello!

Thanks so much for these! We appreciate it a lot! I throw in some patches where I quickly could!

B.

Bugs

1) Patched in 43 alpha
2) Patched in 43 alpha
3) Patched previously in 43 alpha
4) Patched previously in 43 alpha
5) Patched previously in 43 alpha

Security
1) Documented
2) Documented
3) In 43 alpha we deployed and auto-assign username feature that would resolve this should the US admin require restrictions

Logic Flaws:
1) Patched in 43 alpha
2) Patched in 43 alpha
3) Patched previously in 43 alpha
4) Patched previously in 43 alpha

[mudmin]

Awesome. Thanks so much! I had to make an unexpected trip up to Fairbanks, so I'm out of my routines. We're so close to beta on 4.3. I think we might have a few more forms that are missing token checks.

And @SavageStyle ...thanks so much for giving back to the project.

[SavageStyle]

Found another major issue:
http://localhost/usercontrol/users/user_settings.php
This check is not valid:
<pre>

Code:

<?php if (($settings->change_un == 0) || (($settings->change_un == 2) && ($user->data()->un_changed == 1)) ) {
    echo "<input  class='form-control' type='text' name='username' value='$displayname' readonly/>";
}else{
    echo "<input  class='form-control' type='text' name='username' value='$displayname'>";
}?>

</pre>


User can open developer console (for in chrome ctrl+shift+i) and remove "readonly" - that's it, now user can change his name even if he is not allowed to.

That what I've added for myself (two additional checks):
<pre>

Code:

$validation->check($_POST,array(
    'username' => array(
        'display' => 'Псевдоним',
        'required' => true,
        'unique_update' => 'users,'.$userId,
        'min' => (int)$settings->min_un,
        'max' => (int)$settings->max_un,
        'valid_username' => true,
        'can_change_name' => true
    )
));

</pre>


In validate.php I added that code:
<pre>

Code:

case 'valid_username':
    if (!preg_match('/[^A-Za-z0-9]/', $value)) // using ! is important: It will save you from scanning entire user input .
    {
      // string contains only english letters & digits
    }else{
        $this->addError(["{$display} using wron format", $item]);
    }
    break;
case 'can_change_name':
    global $settings;
    global $user;
    if(($settings->change_un == 0) || (($settings->change_un == 2) && ($user->data()->un_changed == 1))){
        $this->addError(["You are not allowed to change your name.", $item]);
    }
    break;

</pre>

[Brandin]

I patched this in 4.3 alpha by adding validation to the PHP.