* [RFC PATCH 1/3] newrole: constant time password comparison
@ 2024-04-08 15:30 Christian Göttsche
2024-04-08 15:30 ` [RFC PATCH 2/3] newrole: cleanse shadow data hold by libc Christian Göttsche
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Christian Göttsche @ 2024-04-08 15:30 UTC (permalink / raw
To: selinux; +Cc: Christian Göttsche
From: Christian Göttsche <cgzones@googlemail.com>
Perform the password hash comparison in a time constant way to avoid
leaking the length of the identical prefix via a side-channel.
Since only hashes are compared leaking the total length is non critical.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
policycoreutils/newrole/newrole.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/policycoreutils/newrole/newrole.c b/policycoreutils/newrole/newrole.c
index 5a1a1129..1e01d2ef 100644
--- a/policycoreutils/newrole/newrole.c
+++ b/policycoreutils/newrole/newrole.c
@@ -338,6 +338,24 @@ static void memzero(void *ptr, size_t size)
}
}
+static int streq_constant(const char *userinput, const char *secret)
+{
+ const volatile char *x = userinput, *y = secret;
+ size_t i, u_len, s_len;
+ int ret = 0;
+
+ u_len = strlen(userinput);
+ s_len = strlen(secret);
+
+ if (u_len != s_len)
+ return 0;
+
+ for (i = 0; i < u_len; i++)
+ ret |= x[i] ^ y[i];
+
+ return ret == 0;
+}
+
/* authenticate_via_shadow_passwd()
*
* in: uname - the calling user's user name
@@ -383,7 +401,7 @@ static int authenticate_via_shadow_passwd(const char *uname)
return 0;
}
- ret = !strcmp(encrypted_password_s, p_shadow_line->sp_pwdp);
+ ret = streq_constant(encrypted_password_s, p_shadow_line->sp_pwdp);
memzero(encrypted_password_s, strlen(encrypted_password_s));
return ret;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [RFC PATCH 2/3] newrole: cleanse shadow data hold by libc
2024-04-08 15:30 [RFC PATCH 1/3] newrole: constant time password comparison Christian Göttsche
@ 2024-04-08 15:30 ` Christian Göttsche
2024-04-08 15:30 ` [RFC PATCH 3/3] newrole: use ROWHAMMER resistant values Christian Göttsche
2024-04-09 17:56 ` [RFC PATCH 1/3] newrole: constant time password comparison Stephen Smalley
2 siblings, 0 replies; 4+ messages in thread
From: Christian Göttsche @ 2024-04-08 15:30 UTC (permalink / raw
To: selinux; +Cc: Christian Göttsche
From: Christian Göttsche <cgzones@googlemail.com>
Override the memory holding the retrieved password after usage to avoid
potential leaks.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
policycoreutils/newrole/newrole.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/policycoreutils/newrole/newrole.c b/policycoreutils/newrole/newrole.c
index 1e01d2ef..59a5caa3 100644
--- a/policycoreutils/newrole/newrole.c
+++ b/policycoreutils/newrole/newrole.c
@@ -388,6 +388,7 @@ static int authenticate_via_shadow_passwd(const char *uname)
/* Ask user to input unencrypted password */
if (!(unencrypted_password_s = getpass(PASSWORD_PROMPT))) {
fprintf(stderr, _("getpass cannot open /dev/tty\n"));
+ memzero(p_shadow_line->sp_pwdp, strlen(p_shadow_line->sp_pwdp));
return 0;
}
@@ -398,11 +399,13 @@ static int authenticate_via_shadow_passwd(const char *uname)
memzero(unencrypted_password_s, strlen(unencrypted_password_s));
if (errno || !encrypted_password_s) {
fprintf(stderr, _("Cannot encrypt password.\n"));
+ memzero(p_shadow_line->sp_pwdp, strlen(p_shadow_line->sp_pwdp));
return 0;
}
ret = streq_constant(encrypted_password_s, p_shadow_line->sp_pwdp);
memzero(encrypted_password_s, strlen(encrypted_password_s));
+ memzero(p_shadow_line->sp_pwdp, strlen(p_shadow_line->sp_pwdp));
return ret;
}
#endif /* if/else USE_PAM */
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [RFC PATCH 3/3] newrole: use ROWHAMMER resistant values
2024-04-08 15:30 [RFC PATCH 1/3] newrole: constant time password comparison Christian Göttsche
2024-04-08 15:30 ` [RFC PATCH 2/3] newrole: cleanse shadow data hold by libc Christian Göttsche
@ 2024-04-08 15:30 ` Christian Göttsche
2024-04-09 17:56 ` [RFC PATCH 1/3] newrole: constant time password comparison Stephen Smalley
2 siblings, 0 replies; 4+ messages in thread
From: Christian Göttsche @ 2024-04-08 15:30 UTC (permalink / raw
To: selinux; +Cc: Christian Göttsche
From: Christian Göttsche <cgzones@googlemail.com>
Use values for success and failure that are more resistant to bit flips,
to harden against potential ROWHAMMER attacks.
Inspired by [1].
[1]: https://github.com/sudo-project/sudo/commit/7873f8334c8d31031f8cfa83bd97ac6029309e4f
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
policycoreutils/newrole/newrole.c | 35 +++++++++++++++++--------------
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/policycoreutils/newrole/newrole.c b/policycoreutils/newrole/newrole.c
index 59a5caa3..618c4101 100644
--- a/policycoreutils/newrole/newrole.c
+++ b/policycoreutils/newrole/newrole.c
@@ -89,6 +89,9 @@
#define PACKAGE "policycoreutils" /* the name of this package lang translation */
#endif
+#define ALLOW 0x52a2925
+#define DENY 0xad5d6da
+
#define TRUE 1
#define FALSE 0
@@ -174,8 +177,8 @@ static const char *service_name = "newrole";
* out: nothing
* return: value condition
* ----- ---------
- * 1 PAM thinks that the user authenticated themselves properly
- * 0 otherwise
+ * ALLOW PAM thinks that the user authenticated themselves properly
+ * DENY otherwise
*
* This function uses PAM to authenticate the user running this
* program. This is the only function in this program that makes PAM
@@ -184,7 +187,7 @@ static const char *service_name = "newrole";
static int authenticate_via_pam(const char *ttyn, pam_handle_t * pam_handle)
{
- int result = 0; /* set to 0 (not authenticated) by default */
+ int result = DENY; /* set to DENY (not authenticated) by default */
int pam_rc; /* pam return code */
const char *tty_name;
@@ -210,7 +213,7 @@ static int authenticate_via_pam(const char *ttyn, pam_handle_t * pam_handle)
/* Ask PAM to verify acct_mgmt */
pam_rc = pam_acct_mgmt(pam_handle, 0);
if (pam_rc == PAM_SUCCESS) {
- result = 1; /* user authenticated OK! */
+ result = ALLOW; /* user authenticated OK! */
}
out:
@@ -348,12 +351,12 @@ static int streq_constant(const char *userinput, const char *secret)
s_len = strlen(secret);
if (u_len != s_len)
- return 0;
+ return DENY;
for (i = 0; i < u_len; i++)
ret |= x[i] ^ y[i];
- return ret == 0;
+ return ret == 0 ? ALLOW : DENY;
}
/* authenticate_via_shadow_passwd()
@@ -362,9 +365,9 @@ static int streq_constant(const char *userinput, const char *secret)
* out: nothing
* return: value condition
* ----- ---------
- * 1 user authenticated themselves properly according to the
+ * ALLOW user authenticated themselves properly according to the
* shadow passwd file.
- * 0 otherwise
+ * DENY otherwise
*
* This function uses the shadow passwd file to thenticate the user running
* this program.
@@ -382,14 +385,14 @@ static int authenticate_via_shadow_passwd(const char *uname)
if (!(p_shadow_line)) {
fprintf(stderr, _("Cannot find your entry in the shadow "
"passwd file.\n"));
- return 0;
+ return DENY;
}
/* Ask user to input unencrypted password */
if (!(unencrypted_password_s = getpass(PASSWORD_PROMPT))) {
fprintf(stderr, _("getpass cannot open /dev/tty\n"));
memzero(p_shadow_line->sp_pwdp, strlen(p_shadow_line->sp_pwdp));
- return 0;
+ return DENY;
}
/* Use crypt() to encrypt user's input password. */
@@ -400,7 +403,7 @@ static int authenticate_via_shadow_passwd(const char *uname)
if (errno || !encrypted_password_s) {
fprintf(stderr, _("Cannot encrypt password.\n"));
memzero(p_shadow_line->sp_pwdp, strlen(p_shadow_line->sp_pwdp));
- return 0;
+ return DENY;
}
ret = streq_constant(encrypted_password_s, p_shadow_line->sp_pwdp);
@@ -416,7 +419,7 @@ static int authenticate_via_shadow_passwd(const char *uname)
*/
static int verify_shell(const char *shell_name)
{
- int found = 0;
+ int found = DENY;
const char *buf;
if (!(shell_name && shell_name[0]))
@@ -429,7 +432,7 @@ static int verify_shell(const char *shell_name)
/* check the shell skipping newline char */
if (!strcmp(shell_name, buf)) {
- found = 1;
+ found = ALLOW;
break;
}
}
@@ -479,7 +482,7 @@ static int extract_pw_data(struct passwd *pw_copy)
goto out_free;
}
- if (verify_shell(pw->pw_shell) == 0) {
+ if (verify_shell(pw->pw_shell) != ALLOW) {
fprintf(stderr, _("Error! Shell is not valid.\n"));
goto out_free;
}
@@ -1182,9 +1185,9 @@ int main(int argc, char *argv[])
goto err_free;
}
- if (!authenticate_via_pam(ttyn, pam_handle))
+ if (authenticate_via_pam(ttyn, pam_handle) != ALLOW)
#else
- if (!authenticate_via_shadow_passwd(pw.pw_name))
+ if (authenticate_via_shadow_passwd(pw.pw_name) != ALLOW)
#endif
{
fprintf(stderr, _("newrole: incorrect password for %s\n"),
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH 1/3] newrole: constant time password comparison
2024-04-08 15:30 [RFC PATCH 1/3] newrole: constant time password comparison Christian Göttsche
2024-04-08 15:30 ` [RFC PATCH 2/3] newrole: cleanse shadow data hold by libc Christian Göttsche
2024-04-08 15:30 ` [RFC PATCH 3/3] newrole: use ROWHAMMER resistant values Christian Göttsche
@ 2024-04-09 17:56 ` Stephen Smalley
2 siblings, 0 replies; 4+ messages in thread
From: Stephen Smalley @ 2024-04-09 17:56 UTC (permalink / raw
To: cgzones; +Cc: selinux
On Mon, Apr 8, 2024 at 11:31 AM Christian Göttsche
<cgoettsche@seltendoof.de> wrote:
>
> From: Christian Göttsche <cgzones@googlemail.com>
>
> Perform the password hash comparison in a time constant way to avoid
> leaking the length of the identical prefix via a side-channel.
> Since only hashes are compared leaking the total length is non critical.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Should we just require PAM for newrole and be done with it?
> ---
> policycoreutils/newrole/newrole.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/policycoreutils/newrole/newrole.c b/policycoreutils/newrole/newrole.c
> index 5a1a1129..1e01d2ef 100644
> --- a/policycoreutils/newrole/newrole.c
> +++ b/policycoreutils/newrole/newrole.c
> @@ -338,6 +338,24 @@ static void memzero(void *ptr, size_t size)
> }
> }
>
> +static int streq_constant(const char *userinput, const char *secret)
> +{
> + const volatile char *x = userinput, *y = secret;
> + size_t i, u_len, s_len;
> + int ret = 0;
> +
> + u_len = strlen(userinput);
> + s_len = strlen(secret);
> +
> + if (u_len != s_len)
> + return 0;
> +
> + for (i = 0; i < u_len; i++)
> + ret |= x[i] ^ y[i];
> +
> + return ret == 0;
> +}
> +
> /* authenticate_via_shadow_passwd()
> *
> * in: uname - the calling user's user name
> @@ -383,7 +401,7 @@ static int authenticate_via_shadow_passwd(const char *uname)
> return 0;
> }
>
> - ret = !strcmp(encrypted_password_s, p_shadow_line->sp_pwdp);
> + ret = streq_constant(encrypted_password_s, p_shadow_line->sp_pwdp);
> memzero(encrypted_password_s, strlen(encrypted_password_s));
> return ret;
> }
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-04-09 17:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-08 15:30 [RFC PATCH 1/3] newrole: constant time password comparison Christian Göttsche
2024-04-08 15:30 ` [RFC PATCH 2/3] newrole: cleanse shadow data hold by libc Christian Göttsche
2024-04-08 15:30 ` [RFC PATCH 3/3] newrole: use ROWHAMMER resistant values Christian Göttsche
2024-04-09 17:56 ` [RFC PATCH 1/3] newrole: constant time password comparison Stephen Smalley
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).