passport-multiauth icon indicating copy to clipboard operation
passport-multiauth copied to clipboard

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

Open kudlohlavec opened this issue 6 years ago • 20 comments

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.

kudlohlavec avatar Nov 28 '18 17:11 kudlohlavec

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

michaelnguyen2021 avatar Dec 10 '18 22:12 michaelnguyen2021

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

sfelix-martins avatar Dec 24 '18 14:12 sfelix-martins

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()]);
}

askme-gpt avatar Jan 03 '19 06:01 askme-gpt

@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 avatar Jan 03 '19 09:01 kudlohlavec

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...

kudlohlavec avatar Jan 03 '19 12:01 kudlohlavec

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

sfelix-martins avatar Jan 03 '19 12:01 sfelix-martins

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.

kudlohlavec avatar Jan 03 '19 12:01 kudlohlavec

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?

bienhoang avatar Jan 24 '19 10:01 bienhoang

@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 avatar Jan 29 '19 11:01 sfelix-martins

@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.

kudlohlavec avatar Jan 30 '19 08:01 kudlohlavec

@pakcybershagufta did you tested the instructions from https://github.com/sfelix-martins/passport-multiauth/issues/63#issuecomment-458506088 ?

sfelix-martins avatar Mar 04 '19 11:03 sfelix-martins

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

sfelix-martins avatar Mar 04 '19 11:03 sfelix-martins

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?

pakcybershagufta avatar Mar 04 '19 11:03 pakcybershagufta

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

sfelix-martins avatar Mar 04 '19 11:03 sfelix-martins

Yes exactly..

pakcybershagufta avatar Mar 04 '19 12:03 pakcybershagufta

@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.

pakcybershagufta avatar Mar 04 '19 12:03 pakcybershagufta

@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.

cch504 avatar Mar 05 '19 14:03 cch504

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',
        ],
    ],```

emircansancar avatar May 10 '19 13:05 emircansancar

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

netbuilding avatar Oct 21 '19 08:10 netbuilding

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);
    }

kluivert-queiroz avatar Aug 20 '20 15:08 kluivert-queiroz