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

FR: Allow for better return values of two factor module #26593

Closed
cornelinux opened this Issue Nov 10, 2016 · 11 comments

Comments

Projects
None yet
5 participants
@cornelinux
Contributor

cornelinux commented Nov 10, 2016

I am working with ownCloud 9.1 Two Factor authentication framework.

The method verifyChallenge

public function verifyChallenge(IUser $user, $challenge);

is supposed to return true or false.
In most cases this is great, since we do not want the attacker to give any additional information, why an authentication failed. But it also increases the hurdle for normal users and the help desk. Returning additinoal information could help the support to track down issues.

Please tell me, what you think. I would be happy to provide any input or a pull request.

Steps to reproduce

  1. Enable privacyIDEA owncloud App
  2. enter wrong OTP value -> standard error message
  3. shut down privacyIDEA server and enter correct OTP value -> standard error message as before

Expected behaviour

The method should return additional information to be displayed, in case of failed authentication.

Actual behaviour

The method only returns true or false. The login screen displays a standard information, that an "error occurred during token verification".

Server configuration

Operating system:
Ubuntu 16.04

Web server:
Apache

Database:
MySQL

ownCloud version: (see ownCloud admin page)
9.1.1 RC3

List of activated apps: privacyIDEA ownCloud App

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 10, 2016

Member

Would it make sense to store the detailed error in the log instead of exposing it to the user ?
If a user reports having trouble logging in, then an admin or support would need to find the matching log entries. This, assuming that the error was caused by an env issue or a bug.

Member

PVince81 commented Nov 10, 2016

Would it make sense to store the detailed error in the log instead of exposing it to the user ?
If a user reports having trouble logging in, then an admin or support would need to find the matching log entries. This, assuming that the error was caused by an env issue or a bug.

@cornelinux

This comment has been minimized.

Show comment
Hide comment
@cornelinux

cornelinux Nov 10, 2016

Contributor

There might be some errors which are so obvious, that it is nice to not need to look into the log.
Like:

  • the authentication server is down.
  • the authentication server is not reachable.

OK, you should have monitoring systems to do this. should! ;-)
But it is great, if such an issue is directly known.

Also and most important: ownCloud is doing an authentication in two steps.

  1. step: Username and password against owncloud. If you fail to authenticate here, you are already asked, if you want to set your password.
  2. The second step asks for the otp. Everyone who reaches the second step (also the attacker) already knows that he has a valid username and a valid password.
    So it has no security impact showing additional information to the user like:
  • wrong OTP value
  • previous OTP value used again
  • you have no token assigned
  • please return your token and get a new one at your IT department

The idea of displaying this information to the user is, to avoid having him call the helpdesk. "Oh. I entered a previous OTP? So I have to push the button again" - instead of calling UHD.

Honestly there is also another scenario I am thinking of.
A scenario like:

  • Your subscription has expired!

Displaying this message to the user or even to the boss! has a much larger effect on getting a new subscription than hiding this message in the log file.

Contributor

cornelinux commented Nov 10, 2016

There might be some errors which are so obvious, that it is nice to not need to look into the log.
Like:

  • the authentication server is down.
  • the authentication server is not reachable.

OK, you should have monitoring systems to do this. should! ;-)
But it is great, if such an issue is directly known.

Also and most important: ownCloud is doing an authentication in two steps.

  1. step: Username and password against owncloud. If you fail to authenticate here, you are already asked, if you want to set your password.
  2. The second step asks for the otp. Everyone who reaches the second step (also the attacker) already knows that he has a valid username and a valid password.
    So it has no security impact showing additional information to the user like:
  • wrong OTP value
  • previous OTP value used again
  • you have no token assigned
  • please return your token and get a new one at your IT department

The idea of displaying this information to the user is, to avoid having him call the helpdesk. "Oh. I entered a previous OTP? So I have to push the button again" - instead of calling UHD.

Honestly there is also another scenario I am thinking of.
A scenario like:

  • Your subscription has expired!

Displaying this message to the user or even to the boss! has a much larger effect on getting a new subscription than hiding this message in the log file.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 10, 2016

Member

@Peter-Prochaska what do you think about such detailed messages in the UI ?

Member

PVince81 commented Nov 10, 2016

@Peter-Prochaska what do you think about such detailed messages in the UI ?

@peterprochaska

This comment has been minimized.

Show comment
Hide comment
@peterprochaska

peterprochaska Nov 10, 2016

Contributor

@PVince81 as @cornelinux said, if the user sees the detailed error messages, there was a username/password authentication before. Therefore, I see no problem with the detailed messages in the UI.

Contributor

peterprochaska commented Nov 10, 2016

@PVince81 as @cornelinux said, if the user sees the detailed error messages, there was a username/password authentication before. Therefore, I see no problem with the detailed messages in the UI.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 10, 2016

Member

Okay cool.

From what I see the PHPDoc of the IProvider::verifyChallenge method is already missing a @return statement. To prevent changing the expectations regarding implementation I suggest to continue allowing returning a boolean with a generic exception message.

In addition to that, let verifyChallenge throw exceptions containing more detailed exception messages. These exceptions (maybe a new base class exception?) must be caught in the calling code to then display the message. I found calling code here: https://github.com/owncloud/core/blob/v9.1.2/core/Controller/TwoFactorChallengeController.php#L153.

Member

PVince81 commented Nov 10, 2016

Okay cool.

From what I see the PHPDoc of the IProvider::verifyChallenge method is already missing a @return statement. To prevent changing the expectations regarding implementation I suggest to continue allowing returning a boolean with a generic exception message.

In addition to that, let verifyChallenge throw exceptions containing more detailed exception messages. These exceptions (maybe a new base class exception?) must be caught in the calling code to then display the message. I found calling code here: https://github.com/owncloud/core/blob/v9.1.2/core/Controller/TwoFactorChallengeController.php#L153.

@cornelinux

This comment has been minimized.

Show comment
Hide comment
@cornelinux

cornelinux Nov 11, 2016

Contributor

This sounds cool to me, if the 2FAProvider can raise an Exception and the upper layer would catch this and display the text as error message to the user.
By raising the new Exception class, we could also determine, if the message is displayed or not. like:

  1. if the exception is a 2FAChallengeException, then display the text
  2. if the exception is of any other type, only display "Some internal error occured" (like before)
Contributor

cornelinux commented Nov 11, 2016

This sounds cool to me, if the 2FAProvider can raise an Exception and the upper layer would catch this and display the text as error message to the user.
By raising the new Exception class, we could also determine, if the message is displayed or not. like:

  1. if the exception is a 2FAChallengeException, then display the text
  2. if the exception is of any other type, only display "Some internal error occured" (like before)
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 11, 2016

Member

@cornelinux sounds good. Do you have time to make a PR to core to adjust it ?

Member

PVince81 commented Nov 11, 2016

@cornelinux sounds good. Do you have time to make a PR to core to adjust it ?

@cornelinux

This comment has been minimized.

Show comment
Hide comment
@cornelinux

cornelinux Nov 11, 2016

Contributor

I try to take a look at it next week.

Contributor

cornelinux commented Nov 11, 2016

I try to take a look at it next week.

@DeepDiver1975 DeepDiver1975 added this to the 9.2 milestone Nov 11, 2016

@cornelinux

This comment has been minimized.

Show comment
Hide comment
@cornelinux

cornelinux Nov 14, 2016

Contributor

@PVince81 The 2FAChallengeException should be publically available in namespave OCP.
As also the apps will use it.
Is there already a central location where you define such Exceptions/Errors?


I guess each component under lib/public/ defines its own Exceptions. I will put it to lib/public/Authentication/TwoFactorAuth

Contributor

cornelinux commented Nov 14, 2016

@PVince81 The 2FAChallengeException should be publically available in namespave OCP.
As also the apps will use it.
Is there already a central location where you define such Exceptions/Errors?


I guess each component under lib/public/ defines its own Exceptions. I will put it to lib/public/Authentication/TwoFactorAuth

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 14, 2016

Member

lib/public/Authentication/TwoFactorAuth/TwoFactorChalengeException sounds good

Member

PVince81 commented Nov 14, 2016

lib/public/Authentication/TwoFactorAuth/TwoFactorChalengeException sounds good

@cornelinux cornelinux referenced this issue Nov 14, 2016

Merged

Add a TwoFactorException #26628

3 of 9 tasks complete

PVince81 added a commit that referenced this issue Nov 18, 2016

Add a TwoFactorException (#26628)
* Add a TwoFactorException

A Two Factor third party App may throw a TwoFactorException()
with a more detailed error message in case the authentication fails.
The 2FA Controller will then display the message of this Exception
to the user.

Working on #26593

* Update TwoFactorException.php

* Copyright

* Fix tests
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 6, 2016

Member

Closing as fixed in #26628

Member

PVince81 commented Dec 6, 2016

Closing as fixed in #26628

@PVince81 PVince81 closed this Dec 6, 2016

@PVince81 PVince81 referenced this issue Apr 18, 2017

Closed

10.0 testing #423

68 of 82 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment