CVE-2021-45890: Improper Authentication (CWE-287)
value: basic/BasicAuthProvider.java in AuthGuard before 0.9.0 allows authentication via an inactive identifier.
CWE description: When an actor claims to have a given identity, the product does not prove or insufficiently proves that the claim is correct.
CVSS v3 base score: 9.8
Published: 2021-12-27T20:15Z
Repository: https://github.com/AuthGuard/AuthGuard
Language: Java
Files changed: basic-auth/src/main/java/com/nexblocks/authguard/basic/BasicAuthProvider.java, basic-auth/src/test/java/com/nexblocks/authguard/basic/BasicAuthProviderTest.java, service-api/src/main/java/com/nexblocks/authguard/service/exceptions/codes/ErrorCode.java, service-api/src/main/java/com/nexblocks/authguard/service/model/UserIdentifier.java
Commit date: 2021-10-06 18:48:22 +0100
Commit message: Prevent authentication with inactive identifiers

Diff: diff --git a/basic-auth/src/main/java/com/nexblocks/authguard/basic/BasicAuthProvider.java b/basic-auth/src/main/java/com/nexblocks/authguard/basic/BasicAuthProvider.java
index d27c325..fc4908c 100644
--- a/basic-auth/src/main/java/com/nexblocks/authguard/basic/BasicAuthProvider.java
+++ b/basic-auth/src/main/java/com/nexblocks/authguard/basic/BasicAuthProvider.java
@@ -5,14 +5,11 @@ import com.nexblocks.authguard.basic.passwords.SecurePasswordProvider;
 import com.nexblocks.authguard.service.AccountsService;
 import com.nexblocks.authguard.service.CredentialsService;
 import com.nexblocks.authguard.service.exceptions.ServiceAuthorizationException;
 import com.nexblocks.authguard.service.exceptions.ServiceException;
 import com.nexblocks.authguard.service.exceptions.codes.ErrorCode;
-import com.nexblocks.authguard.service.model.AccountBO;
-import com.nexblocks.authguard.service.model.AuthRequestBO;
-import com.nexblocks.authguard.service.model.CredentialsBO;
-import com.nexblocks.authguard.service.model.EntityType;
+import com.nexblocks.authguard.service.model.*;
 import com.google.inject.Inject;
 import io.vavr.control.Either;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -84,11 +81,18 @@ public class BasicAuthProvider {
     }
 
     private Either<Exception, AccountBO> verifyCredentialsAndGetAccount(final String username, final String password) {
         final Optional<CredentialsBO> credentials = credentialsService.getByUsernameUnsafe(username);
 
+        // TODO replace this with Either mapping
         if (credentials.isPresent()) {
+            final Optional<Exception> validationError = checkIdentifier(credentials.get(), username);
+
+            if (validationError.isPresent()) {
+                return Either.left(validationError.get());
+            }
+
             if (securePassword.verify(password, credentials.get().getHashedPassword())) {
                 return getAccountById(credentials.get().getAccountId());
             } else {
                 return Either.left(new ServiceAuthorizationException(ErrorCode.PASSWORDS_DO_NOT_MATCH,
                         "Passwords do not match", EntityType.ACCOUNT, credentials.get().getAccountId()));
@@ -101,17 +105,42 @@ public class BasicAuthProvider {
 
     private Either<Exception, AccountBO> verifyCredentialsAndGetAccount(final String username) {
         final Optional<CredentialsBO> credentials = credentialsService.getByUsernameUnsafe(username);
 
         if (credentials.isPresent()) {
+            final Optional<Exception> validationError = checkIdentifier(credentials.get(), username);
+
+            if (validationError.isPresent()) {
+                return Either.left(validationError.get());
+            }
+
             return getAccountById(credentials.get().getAccountId());
         } else {
             return Either.left(new ServiceAuthorizationException(ErrorCode.CREDENTIALS_DOES_NOT_EXIST,
                     "Identifier " + username + " does not exist"));
         }
     }
 
+    private Optional<Exception> checkIdentifier(final CredentialsBO credentials,
+                                                final String identifier) {
+        final Optional<UserIdentifierBO> matchedIdentifier = credentials.getIdentifiers()
+                .stream()
+                .filter(existing -> identifier.equals(existing.getIdentifier()))
+                .findFirst();
+
+        if (matchedIdentifier.isEmpty()) {
+            return Optional.of(new IllegalStateException("No identifier matched but credentials were returned"));
+        }
+
+        if (!matchedIdentifier.get().isActive()) {
+            return Optional.of(new ServiceAuthorizationException(ErrorCode.INACTIVE_IDENTIFIER,
+                    "Identifier is not active", EntityType.ACCOUNT, credentials.getAccountId()));
+        }
+
+        return Optional.empty();
+    }
+
     private Either<Exception, AccountBO> getAccountById(final String accountId) {
         final Optional<AccountBO> account = accountsService.getById(accountId);
 
         return account
                 .<Either<Exception, AccountBO>>map(Either::right)
diff --git a/basic-auth/src/test/java/com/nexblocks/authguard/basic/BasicAuthProviderTest.java b/basic-auth/src/test/java/com/nexblocks/authguard/basic/BasicAuthProviderTest.java
index 34d1df7..f21e574 100644
--- a/basic-auth/src/test/java/com/nexblocks/authguard/basic/BasicAuthProviderTest.java
+++ b/basic-auth/src/test/java/com/nexblocks/authguard/basic/BasicAuthProviderTest.java
@@ -62,10 +62,11 @@ class BasicAuthProviderTest {
         final AccountBO account = RANDOM.nextObject(AccountBO.class);
         final CredentialsBO credentials = RANDOM.nextObject(CredentialsBO.class)
                 .withIdentifiers(UserIdentifierBO.builder()
                         .identifier(username)
                         .type(UserIdentifier.Type.USERNAME)
+                        .active(true)
                         .build());
         final HashedPasswordBO hashedPasswordBO = HashedPasswordBO.builder()
                 .password(credentials.getHashedPassword().getPassword())
                 .salt(credentials.getHashedPassword().getSalt())
                 .build();
@@ -77,10 +78,31 @@ class BasicAuthProviderTest {
         final Either<Exception, AccountBO> result = basicAuth.authenticateAndGetAccount(authorization);
 
         assertThat(result.get()).isEqualTo(account);
     }
 
+    @Test
+    void authenticateInactiveIdentifier() {
+        final String username = "username";
+        final String password = "password";
+        final String authorization = Base64.getEncoder().encodeToString((username + ":" + password).getBytes());
+
+        final CredentialsBO credentials = RANDOM.nextObject(CredentialsBO.class)
+                .withIdentifiers(UserIdentifierBO.builder()
+                        .identifier(username)
+                        .type(UserIdentifier.Type.USERNAME)
+                        .active(false)
+                        .build());
+
+        Mockito.when(credentialsService.getByUsernameUnsafe(username)).thenReturn(Optional.of(credentials));
+
+        final Either<Exception, AccountBO> result = basicAuth.authenticateAndGetAccount(authorization);
+
+        assertThat(result.isLeft()).isTrue();
+        assertThat(result.getLeft()).isInstanceOf(ServiceAuthorizationException.class);
+    }
+
     @Test
     void authenticateNotFound() {
         final String username = "username";
         final String password = "password";
         final String authorization = Base64.getEncoder().encodeToString((username + ":" + password).getBytes());
diff --git a/service-api/src/main/java/com/nexblocks/authguard/service/exceptions/codes/ErrorCode.java b/service-api/src/main/java/com/nexblocks/authguard/service/exceptions/codes/ErrorCode.java
index 30bf4c0..262ddd5 100644
--- a/service-api/src/main/java/com/nexblocks/authguard/service/exceptions/codes/ErrorCode.java
+++ b/service-api/src/main/java/com/nexblocks/authguard/service/exceptions/codes/ErrorCode.java
@@ -31,10 +31,11 @@ public enum ErrorCode {
     INVALID_AUTHORIZATION_FORMAT("AT.023"),
     INVALID_ADDITIONAL_INFORMATION_TYPE("AT.024"),
     TOKEN_EXPIRED_OR_DOES_NOT_EXIST("AT.025"),
     TOKEN_GENERATION_FAILED("AT.031"),
     ACCOUNT_IS_LOCKED("AT.032"),
+    INACTIVE_IDENTIFIER("AT.033"),
     GENERIC_AUTH_FAILURE("AT.039"),
 
     UNSUPPORTED_JWT_ALGORITHM("JT.021"),
     ENCRYPTION_NOT_SUPPORTED("JT.022"),
 
diff --git a/service-api/src/main/java/com/nexblocks/authguard/service/model/UserIdentifier.java b/service-api/src/main/java/com/nexblocks/authguard/service/model/UserIdentifier.java
index 605a481..6a5d76a 100644
--- a/service-api/src/main/java/com/nexblocks/authguard/service/model/UserIdentifier.java
+++ b/service-api/src/main/java/com/nexblocks/authguard/service/model/UserIdentifier.java
@@ -6,11 +6,15 @@ import org.immutables.value.Value;
 @BOStyle
 public interface UserIdentifier {
     Long getId(); // only useful for relational DBs
     Type getType();
     String getIdentifier();
-    boolean isActive();
+
+    @Value.Default
+    default boolean isActive() {
+        return true;
+    }
 
     enum Type {
         USERNAME,
         EMAIL,
         PHONE_NUMBER

Vulnerable code: import com.nexblocks.authguard.service.model.AccountBO;
import com.nexblocks.authguard.service.model.AuthRequestBO;
import com.nexblocks.authguard.service.model.CredentialsBO;
import com.nexblocks.authguard.service.model.EntityType;
    boolean isActive();
Fixed code: import com.nexblocks.authguard.service.model.*;
        // TODO replace this with Either mapping
            final Optional<Exception> validationError = checkIdentifier(credentials.get(), username);

            if (validationError.isPresent()) {
                return Either.left(validationError.get());
            }

            final Optional<Exception> validationError = checkIdentifier(credentials.get(), username);

            if (validationError.isPresent()) {
                return Either.left(validationError.get());
            }

    private Optional<Exception> checkIdentifier(final CredentialsBO credentials,
                                                final String identifier) {
        final Optional<UserIdentifierBO> matchedIdentifier = credentials.getIdentifiers()
                .stream()
                .filter(existing -> identifier.equals(existing.getIdentifier()))
                .findFirst();

        if (matchedIdentifier.isEmpty()) {
            return Optional.of(new IllegalStateException("No identifier matched but credentials were returned"));
        }

        if (!matchedIdentifier.get().isActive()) {
            return Optional.of(new ServiceAuthorizationException(ErrorCode.INACTIVE_IDENTIFIER,
                    "Identifier is not active", EntityType.ACCOUNT, credentials.getAccountId()));
        }

        return Optional.empty();
    }

                        .active(true)
    @Test
    void authenticateInactiveIdentifier() {
        final String username = "username";
        final String password = "password";
        final String authorization = Base64.getEncoder().encodeToString((username + ":" + password).getBytes());

        final CredentialsBO credentials = RANDOM.nextObject(CredentialsBO.class)
                .withIdentifiers(UserIdentifierBO.builder()
                        .identifier(username)
                        .type(UserIdentifier.Type.USERNAME)
                        .active(false)
                        .build());

        Mockito.when(credentialsService.getByUsernameUnsafe(username)).thenReturn(Optional.of(credentials));

        final Either<Exception, AccountBO> result = basicAuth.authenticateAndGetAccount(authorization);

        assertThat(result.isLeft()).isTrue();
        assertThat(result.getLeft()).isInstanceOf(ServiceAuthorizationException.class);
    }

    INACTIVE_IDENTIFIER("AT.033"),

    @Value.Default
    default boolean isActive() {
        return true;
    }