[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


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


Nate DeSimone
 

-----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