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

severe security issue - authenticated user is passing all types of guards #63

Open
kudlohlavec opened this issue Nov 28, 2018 · 20 comments

Comments

@kudlohlavec
Copy link

kudlohlavec commented Nov 28, 2018

Hi,

I am experiencing severe security issue with version 4.0 . It doesnt appear in version 3.0

Problem is that after retrieval of access token I am able to pass through all of the defined auth guards. I got 2 types of models, however type 1 can go through auth guard 1 and 2 as well and vice versa for model of type 2.

As I have already mentioned, I am not experiencing this bug on version 3.0.

Problematic version combination is:

Laravel - 5.7.15
Passport - 7.0.3
Multiauth - 4.0

EDIT:
I am returning user instance by Auth::user() as response and when I am accessing guard 1 with token from model 2, I am retrieving user from model 1 as response with id that is same as for model 2.

However when I am accessing guard 2 with model 2 I am getting right credentials for that models user. And when I am accessing guard 2 with model 1 I am getting credentials from user of model 1.

@michaelnguyen2021
Copy link
Contributor

can you post your code? I don't have this issue

@sfelix-martins
Copy link
Owner

@kudlohlavec Can you post your code? Or link to a similar project simulating the error?

@askme-gpt
Copy link

It still has this problem ,my code :

Route::group(['prefix' => 'data', 'middleware' => 'multiauth:api'], function ($route) {
    $route->get('readData', 'ChannelFormDataController@readData');
});

image
and when I cahnge the middleware to a not exist provider , It still works ,and this is the problem .

Route::group(['prefix' => 'data', 'middleware' => 'multiauth:somethingxxx'], function ($route) {
    $route->get('readData', 'ChannelFormDataController@readData');
});

the result is the same !

the action method as follow :

    public function readData(Request $request)
    {
        return response()->json([auth()->user()]);
}

@kudlohlavec
Copy link
Author

kudlohlavec commented Jan 3, 2019

@michaelnguyen547 , @sfelix-martins I'm posting related code:

This is login method for model 1 (user):

public function apiLogin(UserLoginRequest $request){
        
        $http = new Client();

        $validated = $request->validated();

        $response = $http->post(env('REVERSE_PROXY') . '/oauth/token', [
            'form_params' => [
                'grant_type' => 'password',
                'client_id' => env('PASSPORT_CLIENT_ID'),
                'client_secret' => env('PASSPORT_KEY'),
                'username' => $validated['email'],
                'password' => $validated['password'],
                'scope' => '',
                'provider' => 'users'
            ],
        ]);

        return json_decode((string) $response->getBody(), true);
    }

This is login method for model 2 (gateway):

public function apiLogin(GatewayLoginRequest $request){
        $http = new Client();

        $validated = $request->validated();

        $response = $http->post(env('REVERSE_PROXY') . '/oauth/token', [
            'form_params' => [
                'grant_type' => 'password',
                'client_id' => env('PASSPORT_CLIENT_ID'),
                'client_secret' => env('PASSPORT_KEY'),
                'username' => $validated['serial_code'],
                'password' => $validated['password'],
                'scope' => '',
                'provider' => 'gateways'
            ],
        ]);

        return json_decode((string) $response->getBody(), true);
    }

config/auth.php:

return [
'defaults' => [
        'guard' => 'api',
        'passwords' => 'users',
    ],

'guards' => [
        'api' => [
            'driver' => 'passport',
            'provider' => 'users',
        ],

        'gateway-api' => [
            'driver' => 'passport',
            'provider' => 'gateways',
        ],
    ],

'providers' => [
        'users' => [
            'driver' => 'eloquent',
            'model' => App\User::class,
        ],

        'gateways' => [
            'driver' => 'eloquent',
            'model' => App\Gateway::class,
        ],
    ],

'passwords' => [
        'users' => [
            'provider' => 'users',
            'table' => 'password_resets',
            'expire' => 60,
        ],
    ],

];

app/Http/Kernel.php:

protected $routeMiddleware = [
        // 'auth' => \App\Http\Middleware\Authenticate::class,
        'auth' => \SMartins\PassportMultiauth\Http\Middleware\MultiAuthenticate::class,
        'auth.basic' => \Illuminate\Auth\Middleware\AuthenticateWithBasicAuth::class,
        'bindings' => \Illuminate\Routing\Middleware\SubstituteBindings::class,
        'cache.headers' => \Illuminate\Http\Middleware\SetCacheHeaders::class,
        'can' => \Illuminate\Auth\Middleware\Authorize::class,
        'guest' => \App\Http\Middleware\RedirectIfAuthenticated::class,
        'signed' => \Illuminate\Routing\Middleware\ValidateSignature::class,
        'throttle' => \Illuminate\Routing\Middleware\ThrottleRequests::class,
        'verified' => \Illuminate\Auth\Middleware\EnsureEmailIsVerified::class,
        'oauth.providers' => \SMartins\PassportMultiauth\Http\Middleware\AddCustomProvider::class,
    ];

app/Providers/AuthServiceProvider.php:

public function boot()
    {
        $this->registerPolicies();

        Passport::routes();

        // Middleware `oauth.providers` middleware defined on $routeMiddleware above
        Route::group(['middleware' => 'oauth.providers'], function () {
            Passport::routes(function ($router) {
                return $router->forAccessTokens();
            });
        });
    }

I am protecting routes by auth middleware like this:

User routes

Route::group([
    'prefix' => 'app-api/v1',
    'namespace' => 'Api\V1\Application',
    'middleware' => 'auth:api'
    ],
function()
{
//API endpoints for users
}

Gateway routes

Route::group([
    'prefix' => 'gateway-api/v1',
    'namespace' => 'Api\V1\Gateway',
    'middleware' => 'auth:gateway-api'
    ],
function()
{
//API endpoints for gateways
}

Hope that it will help, however i consider it to be very basic setup...

@kudlohlavec
Copy link
Author

I got little bit more informations regarding this issue.

I tried clean laravel installation where I only added passport and multiauth extension and everything is working as it should be.

However I dont understand what can be the problem, for both projects, the broken one and the new one that is working properly, I used same setup steps for implementation of multiauth functionality. The only difference is that broken project runs in docker and the correct one runs directly on localhost...

@sfelix-martins
Copy link
Owner

@kudlohlavec @ShuiPingYang Thanks for you contribution. I will see it ASAP.

@kudlohlavec
Copy link
Author

Thanks @sfelix-martins . I was also able to reproduce @ShuiPingYang problem with unexisting provider in my docker version. In my correct localhost version, unexisting provider throws error. So as @ShuiPingYang proposed it looks to be the same problem for me and him.

@bienhoang
Copy link

bienhoang commented Jan 24, 2019

Anyone has a solution? I got the same problem
But I install this package for Lumen, It works but got our problem here.
Anyone try to specify scope for each guard?

@sfelix-martins
Copy link
Owner

@bienhoang @kudlohlavec @ShuiPingYang
I think that updates on branch fix-same-id-model-one-token-4.0 can solve the problem! Or fix-same-id-model-one-token if you are using version 3.0 of the package.

Can you test it, for the good of open source 😄 ?

Change your composer.json file:

"minimum-stability": "dev",
"prefer-stable": true,
"require": {
    "smartins/passport-multiauth": "dev-fix-same-id-model-one-token-4.0",
}

And run composer update smartins/passport-multiauth

Give me a feedback, please.

@kudlohlavec
Copy link
Author

kudlohlavec commented Jan 30, 2019

@sfelix-martins Sorry to announce, that it didnt work for me... Still same issue. But it would be for the best if others can confirm it too.

@sfelix-martins
Copy link
Owner

@pakcybershagufta did you tested the instructions from #63 (comment) ?

@sfelix-martins
Copy link
Owner

Please, delete your composer.lock and remove the vendor folder and try again

@pakcybershagufta
Copy link

Can ii ask one more thing any facility availbe in this package i want secure my categories /countries api which does not require user login?

@sfelix-martins
Copy link
Owner

sfelix-martins commented Mar 4, 2019

Do you want that just your client apps use your /countries route, but this route don’t have authentication?

@pakcybershagufta
Copy link

Yes exactly..

@pakcybershagufta
Copy link

@bienhoang @kudlohlavec @ShuiPingYang
I think that updates on branch fix-same-id-model-one-token-4.0 can solve the problem! Or fix-same-id-model-one-token if you are using version 3.0 of the package.

Can you test it, for the good of open source smile ?

Change your composer.json file:

"minimum-stability": "dev",
"prefer-stable": true,
"require": {
    "smartins/passport-multiauth": "dev-fix-same-id-model-one-token-4.0",
}

And run composer update smartins/passport-multiauth

Give me a feedback, please.

@sfelix-martins Sorry to announce, that it didnt work for me... Still same issue. But it would be for the best if others can confirm it too.

imp point is missing when ever you wnat use multauth you should deifne route something like

Route::middleware('multiauth:admin,user')->get('user', function (Request $request) {
return $request->user(); // Returns an instance of designer or preloved user
});
middleware 'multiauth:admin,user' instead of auth..Very important point.

@cch504
Copy link

cch504 commented Mar 5, 2019

@sfelix-martins @kudlohlavec
I can also verify this issue and I found a workaround (I think). In my case I'm using 3 models:

'guards' => [
        'web' => [
            'driver' => 'session',
            'provider' => 'users',
        ],
        'api' => [
            'driver' => 'passport',
            'provider' => 'users',
        ],
        'admin' => [
            'driver' => 'passport',
            'provider' => 'admins',
        ],
        'doctor' => [
            'driver' => 'passport',
            'provider' => 'doctors',
        ],
    ],`

When a user (from the User model) gets authenticated it will be able to pass the doctor and admin guard. The same applies to all models. The only difference was that with a user requested token trying to get the user from a doctor guard I would get the user model, and the same happened when requesting the admin data from passing an admin guard. This made me think that maybe my default guard had something to do with this issue since I changed my default to api (user model). I changed my default guard back to web and everything started working as it should.

@emircansancar
Copy link

emircansancar commented May 10, 2019

I installed the dev-fix-same-id-model-one-token-4.0 package but I still have the same problem. two types of users access each other's routes. I get the problem when I change the default guard.

Is there a way to do this without the default I want to make a endpoint consisting of only api I do not want to use the default?

//Work
    'defaults' => [
        'guard' => 'web',
    ],

    'guards' => [
        'web' => [
            'driver' => 'session',
            'provider' => 'admins',
        ],
        'admin' => [
            'driver' => 'passport',
            'provider' => 'admins',
        ],
        'member' => [
            'driver' => 'passport',
            'provider' => 'members',
        ],
    ],

    //Bug
    'defaults' => [
        'guard' => 'admin',
    ],

    'guards' => [
        'admin' => [
            'driver' => 'passport',
            'provider' => 'admins',
        ],
        'member' => [
            'driver' => 'passport',
            'provider' => 'members',
        ],
    ],```

@netbuilding
Copy link

same problem here, the bug exists when I changed the default guard. It's okay when the default change back to 'web'.

@kluivert-queiroz
Copy link

I had the same problem with Lumen. Could solve this by automatically providing the "provider" param, whenever token is provided I get the token's provider and pass it to Passport. Don't know if this can cause some trouble in any way. For my ongoing project may works fine.

public function handle(Request $request, Closure $next)
    {
        $this->defaultApiProvider = config('auth.guards.api.provider');

        $provider = $request->get('provider', 'users');

        if ($this->invalidProvider($provider)) {
            throw OAuthServerException::invalidRequest('provider');
        }
        $token = $request->bearerToken();
        if($token) {
            $parsedToken = (new Parser)->parse($token)->getClaim('jti');
            $provider = Provider::find($parsedToken)->provider;
        }
        config(['auth.guards.api.provider' => $provider]);
        
        return $next($request);
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants