Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

insane token params #14918

Closed
thelounge-zz opened this issue Jan 30, 2019 · 8 comments
Closed

insane token params #14918

thelounge-zz opened this issue Jan 30, 2019 · 8 comments
Assignees
Labels
enhancement A feature request for improving phpMyAdmin has-pr An issue that has a pull request pending that may fix this issue. The pull request may be incomplete undecided
Projects
Milestone

Comments

@thelounge-zz
Copy link

may you guys consider instead crap like "&token=hzRo%3F6v~*95v'_7]" where intrusion detection systems react create tokens like every normal person on this plant with hash functions?

@thelounge-zz
Copy link
Author

and errors like "PHP Warning: Unknown: Input variables exceeded 1000. To increase the limit change max_input_vars in php.ini. in Unknown on line 0" triggered by various exports sucks too - triggered by a user

may you consider roll back to the version before ajax and forget any "development" which happened from then?

@thelounge-zz
Copy link
Author

BTW: it's the f**ing ~ - who the hell is using that in a GET request on servers where you don't serve userhomes and backup files to the world?

@thelounge-zz
Copy link
Author

guess what - gefore phpMyAdmin 4.8.x no problem

18.05.2013

SecRule REQUEST_URI "~" "id:'121',phase:1,status:404,nolog,t:none,t:htmlEntityDecode,t:urlDecodeUni,t:removeWhitespace,t:replaceNulls,block"

@williamdes
Copy link
Member

@ibennetch @MauricioFauth

@mynetx
Copy link
Contributor

mynetx commented Mar 9, 2019

From Util.php, lines 4761+:

    public static function generateRandom($length)
    {
        $result = '';
        if (class_exists('phpseclib\\Crypt\\Random')) {
            $random_func = [
                'phpseclib\\Crypt\\Random',
                'string',
            ];
        } else {
            $random_func = 'openssl_random_pseudo_bytes';
        }
        while (strlen($result) < $length) {
            // Get random byte and strip highest bit
            // to get ASCII only range
            $byte = ord($random_func(1)) & 0x7f;
            // We want only ASCII chars
            if ($byte > 32) {
                $result .= chr($byte);
            }
        }
        return $result;
    }

The problem here is that the returned random string can also contain URL-problematic characters, like ~ and #.

After taking a look at the ASCII table, I suggest to remove the interval from U+0021 (33) to U+002B (43) and thus exclude these characters from the allowed set:

Unicode codepoint Decimal character
U+0021 33 !
U+0022 34 "
U+0023 35 #
U+0024 36 $
U+0025 37 %
U+0026 38 &
U+0027 39 '
U+0028 40 (
U+0029 41 )
U+002A 42 *
U+002B 43 +

In addition, I’d add a blacklist for these characters:

Unicode codepoint Decimal character
U+003C 60 <
U+003E 62 >
U+003F 63 ?
U+0060 96 `
U+007E 126 ~

The most effective approach may be either:

  • To use a hash library (suggesting sha256 – need to check phpMyAdmin’s dependencies on the php-hash extension though), applied on the raw string generated in generateRandom(), or
  • To provide a whitelist of allowed characters vs. forbidden ones.

Performance considerations are not that important in my opinion, since the token is not regenerated on every user action, but instead stored in the session.

Open to input from the others here.

@williamdes williamdes added the enhancement A feature request for improving phpMyAdmin label Mar 9, 2019
@thelounge-zz
Copy link
Author

thelounge-zz commented Mar 9, 2019

seriously why do people these days need for every trivial piece of code complex solutions, libraries and frameworks?

php > echo bin2hex(openssl_random_pseudo_bytes(30));
09f233736262e11d4a1e24be2833c539b48d6804b50cc565ebd04fa9132b

php > echo bin2hex(random_bytes(30));
0448309d9f75c816242a3ed1f196e6ccb761bf9e44951012106b5a430dbe

bin2hex(generateRandom()) and you are done for the sake of god if ypu can#t live without using at least one framework

@williamdes williamdes added this to Needs triage in issues via automation May 5, 2019
@williamdes williamdes moved this from Needs triage to Enhancements in issues May 5, 2019
williamdes added a commit to williamdes/phpmyadmintest that referenced this issue Jun 12, 2019
Signed-off-by: William Desportes <williamdes@wdes.fr>
williamdes added a commit to williamdes/phpmyadmintest that referenced this issue Jun 24, 2019
Signed-off-by: William Desportes <williamdes@wdes.fr>
@williamdes williamdes added the has-pr An issue that has a pull request pending that may fix this issue. The pull request may be incomplete label Jun 24, 2019
williamdes added a commit that referenced this issue Jun 27, 2019
Fixes: #14918
Pull-request: #15356

Signed-off-by: William Desportes <williamdes@wdes.fr>
williamdes added a commit that referenced this issue Jun 27, 2019
Signed-off-by: William Desportes <williamdes@wdes.fr>
@williamdes
Copy link
Member

@thelounge-zz We will now use hex for the session token 🎉 !
Will be part of next version

@williamdes williamdes added this to the 4.9.1 milestone Jun 27, 2019
@williamdes williamdes self-assigned this Jun 27, 2019
issues automation moved this from Enhancements to Closed Jun 27, 2019
@thelounge-zz
Copy link
Author

thanks

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement A feature request for improving phpMyAdmin has-pr An issue that has a pull request pending that may fix this issue. The pull request may be incomplete undecided
Projects
issues
  
Closed
Development

No branches or pull requests

3 participants