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

unneeded check for < #12710

Closed
emanuelb opened this issue Nov 16, 2016 · 3 comments
Closed

unneeded check for < #12710

emanuelb opened this issue Nov 16, 2016 · 3 comments
Assignees
Milestone

Comments

@emanuelb
Copy link

  1. in:
    https://github.com/phpmyadmin/phpmyadmin/blob/master/libraries/LanguageManager.php#L846
        if ($accepted_languages && false === mb_strpos($accepted_languages, '<')) {

The matchesAcceptLanguage function use the value of $accepted_languages in preg_match call that will determinate if $language will be returned (it match the $accepted_languages)
Thus, the below:

 && false === mb_strpos($accepted_languages, '<')

need to be removed.

  1. in:
    && false === strpos(PMA_getenv('HTTP_AUTHORIZATION'), '<')
                && false === strpos(PMA_getenv('HTTP_AUTHORIZATION'), '<')

also look useless (exists only in parsing of one of AUTHORIZATION headers, username can contain <)

@nijel nijel self-assigned this Nov 18, 2016
@nijel
Copy link
Contributor

nijel commented Nov 18, 2016

It was added as XSS prevention (see 63508c4 and 2d6e0f0). However it indeed seems a bit weird. Though this was in time where everything was put into globals.

@nijel nijel added this to the 4.6.5 milestone Nov 18, 2016
nijel added a commit that referenced this issue Nov 18, 2016
It is no longer needed since we do match language against existing ones.

Issue #12710

Signed-off-by: Michal Čihař <michal@cihar.com>
@nijel nijel closed this as completed in 53f07e7 Nov 18, 2016
@emanuelb
Copy link
Author

emanuelb commented Nov 18, 2016

The point in 2 (and this bug report) is:

  1. it's better to use htmlspecialchars/urlencode (correct output escaping) in relevant places that output content to prevent XSS instead of disallowing < in input.
  2. Where is in the code the value that come from AUTHORIZATION header / $accepted_languages variable are printed? (the sink) as I didn't found any such insecure flow, it's ok to remove this check.
  3. the above commit point to file that doesn't exists anymore in master (grab_globals.lib.php)

@nijel
Copy link
Contributor

nijel commented Nov 18, 2016

I've removed these as indeed the check is not needed anymore. It does come from ages where all environment were in globals and to avoid problems with XSS, anything containing < was simply rejected. The code was simply copied over and over without much thinking what is it's purpose.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants