Skip to content

Commit

Permalink
prevent insecure characters in locale
Browse files Browse the repository at this point in the history
  • Loading branch information
@taylorotwell
taylorotwell committed Apr 8, 2020
1 parent 5b7f541 commit c248521
Showing 1 changed file with 5 additions and 0 deletions.
5 changes: 5 additions & 0 deletions src/Illuminate/Translation/Translator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use Illuminate\Support\NamespacedItemResolver;
use Illuminate\Support\Str;
use Illuminate\Support\Traits\Macroable;
use InvalidArgumentException;

class Translator extends NamespacedItemResolver implements TranslatorContract
{
Expand Down Expand Up @@ -406,6 +407,10 @@ public function getLocale()
*/
public function setLocale($locale)
{
if (Str::contains($locale, ['.', '/', '\\'])) {

This comment has been minimized.

Sign in to view

Copy link
@mnesgutski

mnesgutski Apr 15, 2020

Hi Taylor,
I'm currently using Laravel for one of our projects and am wondering why you're checking for characters like '.', '/', '' when the default Accept-Language-Header from Mozilla and Chrome looks like this:
de,en-US;q=0.7,en;q=0.3
After an update my Project threw an InvalidArgumentException. I had to revert to an older version.
How are we expected to work with the exception? Is there a place where we have to catch it if it occurs?

Best regards

This comment has been minimized.

Sign in to view

Copy link
@mpyw

mpyw Apr 15, 2020 •
edited
Loading

Contributor

@mnesgutski Basically, HTTP input is not secure. You must not trust them. You can send any
malicious HTTP request to the server.

And you should not call setLocale() directly passing Accept-Language header.

This comment has been minimized.

Sign in to view

Copy link
@mpyw

mpyw Apr 15, 2020 •
edited
Loading

Contributor

My middleware implementation:

<?php

namespace App\Http\Middleware;

use Closure;
use Illuminate\Contracts\Translation\Translator;
use Illuminate\Http\Request;

class DetectLanguage
{
    /**
     * @var \Illuminate\Contracts\Translation\Translator|\Illuminate\Translation\Translator
     */
    protected $translator;

    /**
     * DetectLanguage constructor.
     *
     * @param \Illuminate\Contracts\Translation\Translator $translator
     */
    public function __construct(Translator $translator)
    {
        $this->translator = $translator;
    }

    /**
     * Handle an incoming request.
     *
     * @param  \Illuminate\Http\Request $request
     * @param  \Closure                 $next
     * @param  string[]                 $availableLocales
     * @return mixed
     */
    public function handle(Request $request, Closure $next, string ...$availableLocales)
    {
        if ($locale = $this->getPreferredLocale($request, $availableLocales)) {
            $this->translator->setLocale($locale);
        }

        return $next($request);
    }

    /**
     * @param  \Illuminate\Http\Request $request
     * @param  string[]                 $availableLocales
     * @return null|string
     */
    protected function getPreferredLocale(Request $request, array $availableLocales): ?string
    {
        if (!$locale = explode('_', (string)$request->getPreferredLanguage($availableLocales), 2)[0]) {
            return null;
        }

        if (preg_match('~[/\\\\.]~', $locale)) {
            return null;
        }

        return $locale;
    }

Usage:

DetectLanguage::class . ':de,en'

This comment has been minimized.

Sign in to view

Copy link
@mnesgutski

mnesgutski Apr 15, 2020

That makes sense. Thanks for the answer, I'll take inspiration from your implementation.

This comment has been minimized.

Sign in to view

Copy link
@RoodFruit

RoodFruit Apr 15, 2020 •
edited
Loading

@taylorotwell @mpyw @mnesgutski
Are you aware that locales on Dutch servers usually use "nl_NL.UTF-8"?

Since this update, locale is broken on every website...

I set the locale in config/app.php only, but still no luck

This comment has been minimized.

Sign in to view

Copy link
@mpyw

mpyw Apr 15, 2020 •
edited
Loading

Contributor

@RoodFruit @taylorotwell

In fact,

Str::contains($locale, ['.', '/', '\\'])

is a bad solution for this. The better checking is:

Str::contains($locale, ['/', '\\']) || Str::is($locale, ['.', '..'])

Feel free to submit the PR for this!

This comment has been minimized.

Sign in to view

Copy link
@RoodFruit

RoodFruit Apr 15, 2020 •
edited
Loading

@mpyw @taylorotwell
There we go.. that should do it!
Can you submit the PR please? Currently behind a phone ^^;

This comment has been minimized.

Sign in to view

Copy link
@RoodFruit

RoodFruit Apr 15, 2020 •
edited
Loading

@mpyw @taylorotwell
FYI, it should be:

Str::contains($locale, ['/', '\\']) || Str::is($locale, '.') || Str::is($locale, '..')

Str::is() cannot handle arrays. It will probably resolve in a preg_match() error that expects a string and gets an array.

This comment has been minimized.

Sign in to view

Copy link
@mpyw

mpyw Apr 15, 2020 •
edited
Loading

Contributor

Sorry, choose one of the following:

Str::contains($locale, ['/', '\\']) || Str::is($locale, '.') || Str::is($locale, '..')
Str::contains($locale, ['/', '\\']) || Str::is(['.', '..'], $locale)

The function signature is

bool is(string|array $pattern, string $value)

So that the $locale should be placed after the pattern.

This comment has been minimized.

Sign in to view

Copy link
@RoodFruit

RoodFruit Apr 15, 2020

@mpyw That sounds about right.. didn't check it myself, only know by head the second argument is a string.
I see @taylorotwell already made a change.

This comment has been minimized.

Sign in to view

Copy link
@mpyw

mpyw Apr 15, 2020

Contributor

a0094a5
throw new InvalidArgumentException('Invalid characters present in locale.');
}

$this->locale = $locale;
}

Expand Down

2 comments on commit c248521

@mpyw
Copy link
Contributor

@mpyw mpyw commented on c248521 Apr 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mpyw
Copy link
Contributor

@mpyw mpyw commented on c248521 Apr 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original vulnerability report to @taylorotwell:

Overview

Lang::setLocale() unexpectedly accepts Directory Traversal Attack.
Files with *.php or *.json extension are includable.

Reproduction

$request = Request::create('/', 'GET', [], [], [], [
    'HTTP_ACCEPT_LANGUAGE' => '../../composer',
]);

Lang::setLocale($request->getPreferredLanguage());

dump(__('name'));  // "laravel/laravel"
dump(__('description'));  // "The Laravel Framework."

Proposal

Validate all path strings found in Illuminate\Translation\FileLoader methods.

loadNamespaceOverrides()

$file = "{$this->path}/vendor/{$namespace}/{$locale}/{$group}.php";
  • $namespace should not contain .. . segments.
  • $locale should not contain .. . / segments.
  • $group should not contain .. . segments.

loadPath()

$full = "{$path}/{$locale}/{$group}.php"
  • $locale should not contain .. . / segments.
  • $group should not contain .. . segments.

loadJsonPaths()

$full = "{$path}/{$locale}.json"
  • $locale should not contain .. . / segments.

Target Branches

  • 5.5
  • 6.x
  • 7.x

Please sign in to comment.