[security:low] removal of registrable 2F does not test the current authn level

Environment

LemonLDAP::NG version: 2.0.9

Summary

Given the following conditions

• A registrable (UBK,U2F,TOTP) 2F is used
• Self-registration is enabled on first login (sfRequired)
• Authentication levels are used to filter access to certain applications depending on the SF in use

Then an attacker can:

• Login with a "low-security" method, (for example: using stolen credentials)
• Remove the existing second factor (in portal 2FA menu)
• Login again, and be prompted to register a new device they control
• Access high-security applications

This problem is especially noticeable when using the newly introduced sfOnlyUpgrade feature is used, because the 2F will not be asked if the user directly logs into the portal. But it can also happen when using a choice menu, or 2F conditions in general, or when multiple 2F are offered with different authnLevels.

Possible fixes

Before allowing a 2F to be removed, we must check that the user asking for removal has an authnLevel equals or greater than the authnLevel granted by the removed 2F.

The current state of the code does not make this very easy. All 2F modules handle removal themselves entirely. Perhaps we should wrap a test around it.

We should also maybe refactor registrable 2F plugins to have a better API, right now they look like this

sub run {
 if ($action eq "add") {
 }
 elsif ($action eq "remove") {
 }
 etc.
}

they should probably look like this instead:

sub add {
}

sub remove {
}

Security impact

Probably quite low, it takes a very complicated setup to be affected before 2.0.9, and sfOnlyUpgrade is marked as beta in the doc