[edk2-platforms] [PATCH v1] UserAuthFeaturePkg: VerifyPassword() allows one extra password attempt

[edk2-platforms] [PATCH v1] UserAuthFeaturePkg: VerifyPassword() allows one extra password attempt

[Nate DeSimone]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3756

If the password provided by the user is incorrect, then the VerifyPassword()
function is supposed to return EFI_SECURITY_VIOLATION if the user has not
exceeded the maximum number of password guesses (currently set to 3). If the
number of password guesses has been exceeded, then VerifyPassword() shall return
EFI_ACCESS_DENIED. UserAuthenticationDxe uses EFI_ACCESS_DENIED as the signal
that the number of guesses has been exceeded for the purposes of triggering a
forced reboot.

VerifyPassword() checks if the number of password guess attempts has exceeded
the maximum allowed before checking if the current password guess is correct. If
it has, then VerifyPassword() immediately returns EFI_ACCESS_DENIED. This
behavior is correct since it is possible for VerifyPassword() to be called again
after the maximum number of attempts has been exceeded. However, if the user
guesses incorrectly, then VerifyPassword() will always return
EFI_SECURITY_VIOLATION. This is where the bug is. It is possible that after the
current attempt, the maximum allowed number of attempts is exceeded. Therefore,
VerifyPassword() should check the number of attempts again, after checking if
the password is correct.

Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jadhav Manoj D <manoj.d.jadhav@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
---
 .../UserAuthenticationSmm.c                        | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthenticationSmm.c b/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthenticationSmm.c
index c24c3c47a5..16e3405a82 100644
--- a/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthenticationSmm.c
+++ b/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDxeSmm/UserAuthenticationSmm.c
@@ -504,7 +504,12 @@ SmmPasswordHandler (
 
     if (!IsPasswordVerified (UserGuid, SmmCommunicateSetPassword.OldPassword, PasswordLen + 1)) {
       DEBUG ((DEBUG_ERROR, "SmmPasswordHandler: PasswordVerify - FAIL\n"));
-      Status = EFI_SECURITY_VIOLATION;
+      if (*PasswordTryCount >= PASSWORD_MAX_TRY_COUNT) {
+        DEBUG ((DEBUG_ERROR, "SmmPasswordHandler: SET_PASSWORD try count reach!\n"));
+        Status = EFI_ACCESS_DENIED;
+      } else {
+        Status = EFI_SECURITY_VIOLATION;
+      }
       goto EXIT;
     }
 
@@ -554,7 +559,12 @@ SmmPasswordHandler (
     }
     if (!IsPasswordVerified (UserGuid, SmmCommunicateVerifyPassword.Password, PasswordLen + 1)) {
       DEBUG ((DEBUG_ERROR, "SmmPasswordHandler: PasswordVerify - FAIL\n"));
-      Status = EFI_SECURITY_VIOLATION;
+      if (*PasswordTryCount >= PASSWORD_MAX_TRY_COUNT) {
+        DEBUG ((DEBUG_ERROR, "SmmPasswordHandler: VERIFY_PASSWORD try count reach!\n"));
+        Status = EFI_ACCESS_DENIED;
+      } else {
+        Status = EFI_SECURITY_VIOLATION;
+      }
       goto EXIT;
     }
     mPasswordVerified = TRUE;
-- 
2.27.0.windows.1

Re: [edk2-platforms] [PATCH v1] UserAuthFeaturePkg: VerifyPassword() allows one extra password attempt

[Dandan Bi]

Reviewed-by: Dandan Bi <dandan.bi@intel.com>



Thanks,
Dandan
> -----Original Message-----
> From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
> Sent: Thursday, December 2, 2021 6:52 AM
> To: devel@edk2.groups.io
> Cc: Bi, Dandan <dandan.bi@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Jadhav, Manoj D
> <manoj.d.jadhav@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>
> Subject: [edk2-platforms] [PATCH v1] UserAuthFeaturePkg: VerifyPassword()
> allows one extra password attempt
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3756
> 
> If the password provided by the user is incorrect, then the VerifyPassword()
> function is supposed to return EFI_SECURITY_VIOLATION if the user has not
> exceeded the maximum number of password guesses (currently set to 3). If
> the number of password guesses has been exceeded, then VerifyPassword()
> shall return EFI_ACCESS_DENIED. UserAuthenticationDxe uses
> EFI_ACCESS_DENIED as the signal that the number of guesses has been
> exceeded for the purposes of triggering a forced reboot.
> 
> VerifyPassword() checks if the number of password guess attempts has
> exceeded the maximum allowed before checking if the current password
> guess is correct. If it has, then VerifyPassword() immediately returns
> EFI_ACCESS_DENIED. This behavior is correct since it is possible for
> VerifyPassword() to be called again after the maximum number of attempts
> has been exceeded. However, if the user guesses incorrectly, then
> VerifyPassword() will always return EFI_SECURITY_VIOLATION. This is where
> the bug is. It is possible that after the current attempt, the maximum allowed
> number of attempts is exceeded. Therefore,
> VerifyPassword() should check the number of attempts again, after checking
> if the password is correct.
> 
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Jadhav Manoj D <manoj.d.jadhav@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
> ---
>  .../UserAuthenticationSmm.c                        | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git
> a/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDx
> eSmm/UserAuthenticationSmm.c
> b/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDx
> eSmm/UserAuthenticationSmm.c
> index c24c3c47a5..16e3405a82 100644
> ---
> a/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthenticationDx
> eSmm/UserAuthenticationSmm.c
> +++
> b/Features/Intel/UserInterface/UserAuthFeaturePkg/UserAuthentication
> +++ DxeSmm/UserAuthenticationSmm.c
> @@ -504,7 +504,12 @@ SmmPasswordHandler (
> 
>      if (!IsPasswordVerified (UserGuid,
> SmmCommunicateSetPassword.OldPassword, PasswordLen + 1)) {
>        DEBUG ((DEBUG_ERROR, "SmmPasswordHandler: PasswordVerify -
> FAIL\n"));
> -      Status = EFI_SECURITY_VIOLATION;
> +      if (*PasswordTryCount >= PASSWORD_MAX_TRY_COUNT) {
> +        DEBUG ((DEBUG_ERROR, "SmmPasswordHandler: SET_PASSWORD try
> count reach!\n"));
> +        Status = EFI_ACCESS_DENIED;
> +      } else {
> +        Status = EFI_SECURITY_VIOLATION;
> +      }
>        goto EXIT;
>      }
> 
> @@ -554,7 +559,12 @@ SmmPasswordHandler (
>      }
>      if (!IsPasswordVerified (UserGuid,
> SmmCommunicateVerifyPassword.Password, PasswordLen + 1)) {
>        DEBUG ((DEBUG_ERROR, "SmmPasswordHandler: PasswordVerify -
> FAIL\n"));
> -      Status = EFI_SECURITY_VIOLATION;
> +      if (*PasswordTryCount >= PASSWORD_MAX_TRY_COUNT) {
> +        DEBUG ((DEBUG_ERROR, "SmmPasswordHandler: VERIFY_PASSWORD
> try count reach!\n"));
> +        Status = EFI_ACCESS_DENIED;
> +      } else {
> +        Status = EFI_SECURITY_VIOLATION;
> +      }
>        goto EXIT;
>      }
>      mPasswordVerified = TRUE;
> --
> 2.27.0.windows.1