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

authenticate() can leak sensetive user data via token service #162

Closed
farwayer opened this Issue Apr 16, 2016 · 7 comments

Comments

Projects
None yet
4 participants
@farwayer
Copy link

farwayer commented Apr 16, 2016

Token service returns all user data except password field. It ignores all after hooks from user service so it can leak some sensetive data. It should be documented at least.

@gurisko

This comment has been minimized.

Copy link

gurisko commented Apr 16, 2016

You can always change the data returned by the service with an after hook. However I also find it a bit obscure that it ignores user service after hooks. So +1

@farwayer

This comment has been minimized.

Copy link
Author

farwayer commented Apr 16, 2016

Adding after hook to token service is not problem. But first of all you must know about such behaviour. And second I prefere to secure the concrete data in one place not around all the project.
Now I see hooks is bad place for secure logic by design. All other services are avoiding it.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Apr 16, 2016

If the hook on the service is a remove hook it won't run if accessed internally. The remove hook only removes data for external calls through REST and Socket.io. For example, you want to remove the password field when accessed externally but you still need it to compare the passwords in your app internally.

The token service should remove the user password field before returning already though so there might be a bug there.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Apr 17, 2016

@farwayer can you please provide example code where you are seeing this behaviour? Then we can determine if there is a security issue or not. Thanks 😄.

@farwayer

This comment has been minimized.

Copy link
Author

farwayer commented Apr 17, 2016

Use case: I have veryImportantData field in user service. I prefer client application will never has access to it (at best other services also). So I add remove('veryImportantData') after hook to user service and I hope server will never send this data to user. But token service meanly ignores my efforts and sends all user data except password.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Apr 17, 2016

I think the right fix would be to retrieve the user object twice, once without params (to compare the password) and once with parameters (so that all hooks run and you send it back with the token).

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Apr 18, 2016

@farwayer Just add an after hook to the auth/token service as well. You can register as many of your own hooks on the auth services as you would like. They will be applied after those default ones. Your veryImportantData is custom to your app so that's something you need to lock down explicitly.

I'm going to close this as that should do the trick. If not we can revisit. This may actually just be a documentation thing and something we can create an issue for in the feathers-docs repo.

@ekryski ekryski closed this Apr 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.