Security: Added configurable (constant) authentication-time

[thibsy]

Hi all, this PR is based on #4460

As discussed with and commented by @klees I've come up with an implementation to stretch the execution-time of certain methods or functions to a specific duration. The reason behind this is as mentioned in the previous PR that an attacker could gather information about the existence of user-profiles by inspecting the response-time. To solve this issue I already applied this mechanism in the ilStartUpGUI. Because the execution time is different on each server I added a configuration to enable this feature and define the duration in milliseconds (ms). The configuration is located in Users and Roles > Authentication and Registration and is linked in Users and Roles > Privacy and Security.

I didn't apply the mechanism in ilAccountRegistrationGUI (yet), because this implementation leaks information about the existence of profiles anyways and the mechanism wouldn't change that.

The implementation of this mechanism is located in the HTTP service because I'd like to introduce the usage of increments (IncrementStrategy) that are bound to a users IP-address and will be applied on failed attempts in the future.

There's a register_shutdown_function in my code that might seem weird but serves a crucial purpose: Ensure that given callbacks DO NOT contain exit-statements (as used in redirects e.g.). The implementation will throw an according exception if the ILIAS installation has dev-mode enabled (as all developers should have). This should warn developers that their code might not behave as expected and that they should look into their callback.

Kind regards!

[thibsy]

As mentioned by @klees I removed the debug conditions from the thrown exceptions, since there's a serious problem if one of them is reached.

Though when doing this I noticed the following scenario: Imagine some administrator configures an amount of 2 seconds for the constant authentication time. The system ILIAS is running on is rather slow, so the authentication will exceed the 2 seconds 10 out of 10 times. To test or figure this out, the administrator logs out, but now every time he tries to authenticate a RuntimeException will be thrown, because the server is too slow.

How would we prevent this? My first thought was to do some sort of dry-run on the authentication when configuring the authentication-time, so the administrator can't end up in said infinity loop. This problem also suggests that we shouldn't even store values that exceed the normal authentication-time.

Another thought has been to not throw a RuntimeException if this happens, but then nobody would ever know that his configured amount cannot be applied and the authentication still isn't "secure".

Kind regards.

[klees]

Hi @thibsy!

You could move said configuration to the setup. Please also look into the according guideline, maybe the config should be in the setup anyway.

Best regards!

[thibsy]

Hi @klees

Thx for the hint, according to the guideline which states:

If a configuration contains decisions or fields that can only be known reliably by the server administrator, such as IPs, paths to files on the server or passwords for services on the hosting environment, the configuration MUST be written in the setup only.

I think the configuration should be implemented in the setup only, because a "common" ILIAS administrator probably doesn't know how to figure out the average authentication time.

Could you point me into the right direction on how/where to add this configuration?

Kind regards!

[klees]

Hi @thibsy,

of course =)

Try:

• DevConf35 • Richard Klees: Setup Machinery 10 on Youtube
• README of Setup

Please document config here.

Hope this helps so far, feel free to contact me again.

Best regards!

[thibsy]

Hi all

I've finally moved the configuration of the authentication-time to the ILIAS setup as pointed out by @klees in a previous comment. I also added some minimal test-cases that should cover the mechanism of "stretching" the execution time of a given callback. Unfortunately, we cannot test the shutdown-function with PHPUnit, unless some of you guys would know a way?

Otherwise I see this PR as ready and would like to hear about your remarks in a code-review or comment, especially from you guys @mjansenDatabay and @smeyer-ilias =).

Thanks and best regards!

[thibsy]

Hi @mjansenDatabay and thanks for your approval =). Let me elaborate on the ilAccountRegistrationGUI a bit:

For obvious reasons it shouldn't be possible to create two accounts with the same username, unless some combination of username + ID is used like e.g. on Discord (though in this case the email should be used for logins anyways). I would also assume the same for email addresses, but that is currently possible. Is that intentional?

Now it isn't quite possible to "validate" an account registration without telling the user (or in this case the attacker) the username he just submitted already exists. So it doesn't really matter if the request is stretched or not, since the response will inform the user if it exists anyways. The only safe way would be to disallow self-registration completely.

I see however, that there is potential for brute-force to figure out existing profiles faster, which could be addressed by a delay. But I think that is more fitting for the follow-up PR, since this one covers another issue.

What definitely benefits from this mechanism as well would be the ilPasswordAssistanceGUI, I should probably apply the new mechanism there too.

[mjansenDatabay]

Hi @thibsy ,

First of all thx for your elaboration, now I understood your thoughts :-).

I would also assume the same for email addresses, but that is currently possible. Is that intentional?

I really don't know, maybe the former User component maintainer can provide any feedback here.

So it doesn't really matter if the request is stretched or not, since the response will inform the user if it exists anyways.

Yes, you are right. With this PR, this is only done if all other input is valid: https://github.com/ILIAS-eLearning/ILIAS/pull/4123

Best regards, @mjansenDatabay

[thibsy]

Hi all

I've applied the mechanism for the ilPasswordAssistanceGUI now as well. I thought @mjansenDatabay was responsible for this class, thats why I re-requested his review. You can ignore that Michael, since I just now realized that @pascalseeland is the maintainer :).

As documented in the setup/README.md I also added a separate duration-time (in ms) for these kind of operations, called account_assistance_duration. Do you prefer to split up this config into e.g. password_ and username_assistance_duration @pascalseeland?

Kind regards!

[Amstutz]

We discussed this in the TB Meeting today: The TB does not feel responsible for this. However, Fabian or Michael might take further action here. If not, we have to wait until Per Pascal returns.

However, note, that we are not happy with the current situation concern to long abscence of a maintainer without standin.

[mjansenDatabay]

As discussed in our @ILIAS-eLearning/technical-board meeting ten days ago we would like to merge this PR now. Almost all changed code belongs to the responsibility of @chfsx , the changes in the other components are minimally invasive and trivial.

The PasswordAssistance and Startup code changes only result in existing processes being now wrapped in a callback and (depending on global settings) executed by the HTTP service or immediatly by invoking the callback.

The PrivacySecurity changes only affect a global setting being managed by the setup agent.

[thibsy]

Thx a lot @mjansenDatabay!