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

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

Pushed: https://github.com/tianocore/edk2-platforms/commit/15d8aa1

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Nate
> DeSimone
> Sent: Wednesday, December 1, 2021 2:52 PM
> 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-devel] [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
> 
> 
> 
> 
>