Re: passwd: fix error paths and undefined behaviour
On Mon, 08 May 2023 16:17:51 -, Tobias Stoeckmann wrote: > Turns out that we have yet another possibility to trigger a theoretical > signed integer overflow if pwd_tries is INT_MAX. This one avoids such > situation as well. OK millert@ - todd
Re: passwd: fix error paths and undefined behaviour
I have committed the error handling aspects of the patch.
Turns out that we have yet another possibility to trigger a theoretical
signed integer overflow if pwd_tries is INT_MAX. This one avoids such
situation as well.
Okay?
Index: local_passwd.c
===
RCS file: /cvs/src/usr.bin/passwd/local_passwd.c,v
retrieving revision 1.63
diff -u -p -u -p -r1.63 local_passwd.c
--- local_passwd.c 10 Feb 2022 13:06:46 - 1.63
+++ local_passwd.c 8 May 2023 16:13:37 -
@@ -202,7 +202,7 @@ getnewpasswd(struct passwd *pw, login_ca
pwd_tries = pwd_gettries(lc);
- for (newpass[0] = '\0', tries = 0;;) {
+ for (newpass[0] = '\0', tries = -1;;) {
char repeat[1024];
p = readpassphrase("New password:", newpass, sizeof(newpass),
@@ -217,7 +217,7 @@ getnewpasswd(struct passwd *pw, login_ca
continue;
}
- if ((tries++ < pwd_tries || pwd_tries == 0) &&
+ if ((pwd_tries == 0 || ++tries < pwd_tries) &&
pwd_check(lc, p) == 0)
continue;
p = readpassphrase("Retype new password:", repeat,
sizeof(repeat),
Re: passwd: fix error paths and undefined behaviour
On Fri, May 05, 2023 at 11:00:12AM -0600, Todd C. Miller wrote:
> This looks OK but I'd like to see an error message if waitpid()
> really does fail. How about something like this, which also avoid
> needing the extra variable?
Yes, looks much better!
Index: local_passwd.c
===
RCS file: /cvs/src/usr.bin/passwd/local_passwd.c,v
retrieving revision 1.63
diff -u -p -u -p -r1.63 local_passwd.c
--- local_passwd.c 10 Feb 2022 13:06:46 - 1.63
+++ local_passwd.c 5 May 2023 17:03:43 -
@@ -217,7 +217,7 @@ getnewpasswd(struct passwd *pw, login_ca
continue;
}
- if ((tries++ < pwd_tries || pwd_tries == 0) &&
+ if ((pwd_tries == 0 || tries++ < pwd_tries) &&
pwd_check(lc, p) == 0)
continue;
p = readpassphrase("Retype new password:", repeat,
sizeof(repeat),
Index: pwd_check.c
===
RCS file: /cvs/src/usr.bin/passwd/pwd_check.c,v
retrieving revision 1.17
diff -u -p -u -p -r1.17 pwd_check.c
--- pwd_check.c 28 Aug 2021 06:46:49 - 1.17
+++ pwd_check.c 5 May 2023 17:03:43 -
@@ -114,6 +114,8 @@ pwd_check(login_cap_t *lc, char *passwor
switch (child = fork()) {
case -1:
warn("fork");
+ close(pipefds[0]);
+ close(pipefds[1]);
goto out;
case 0:
(void)signal(SIGINT, SIG_DFL);
@@ -184,8 +186,10 @@ pwd_check(login_cap_t *lc, char *passwor
/* get the return value from the child */
while (waitpid(child, &res, 0) == -1) {
- if (errno != EINTR)
- break;
+ if (errno != EINTR) {
+ warn("waitpid");
+ goto out;
+ }
}
if (WIFEXITED(res) && WEXITSTATUS(res) == 0) {
free(checker);
Re: passwd: fix error paths and undefined behaviour
On Fri, 05 May 2023 17:05:05 -, Tobias Stoeckmann wrote: > On Fri, May 05, 2023 at 11:00:12AM -0600, Todd C. Miller wrote: > > This looks OK but I'd like to see an error message if waitpid() > > really does fail. How about something like this, which also avoid > > needing the extra variable? > > Yes, looks much better! OK millert@ - todd
Re: passwd: fix error paths and undefined behaviour
On Fri, 05 May 2023 16:46:51 -, Tobias Stoeckmann wrote:
> In getnewpasswd we increment "tries" every time we try to enter a new
> password. The code allows this to be repeated endlessly by defining
> passwordtries to be 0 in /etc/login.conf. But unfortunately we even
> increment the int "tries" if pwd_tries is 0, which eventually would lead
> to a signed integer overflow (and we would stop asking for passwords
> after billion times).
>
> In pwd_check we should close pipe file descriptors if fork fails. Also
> it might be possible that waitpid fails if the root user tries to
> sabotage the own passwd call. Let's just handle the error case here
> as well to avoid accessing undefined content of "res".
This looks OK but I'd like to see an error message if waitpid()
really does fail. How about something like this, which also avoid
needing the extra variable?
- todd
Index: /usr/src/usr.bin/passwd/pwd_check.c
===
RCS file: /cvs/src/usr.bin/passwd/pwd_check.c,v
retrieving revision 1.17
diff -u -p -u -r1.17 pwd_check.c
--- /usr/src/usr.bin/passwd/pwd_check.c 28 Aug 2021 06:46:49 - 1.17
+++ /usr/src/usr.bin/passwd/pwd_check.c 5 May 2023 16:59:20 -
@@ -184,8 +184,10 @@ pwd_check(login_cap_t *lc, char *passwor
/* get the return value from the child */
while (waitpid(child, &res, 0) == -1) {
- if (errno != EINTR)
- break;
+ if (errno != EINTR) {
+ warn("waitpid");
+ goto out;
+ }
}
if (WIFEXITED(res) && WEXITSTATUS(res) == 0) {
free(checker);
passwd: fix error paths and undefined behaviour
Hi,
this patch fixes error paths and an undefined behaviour:
In getnewpasswd we increment "tries" every time we try to enter a new
password. The code allows this to be repeated endlessly by defining
passwordtries to be 0 in /etc/login.conf. But unfortunately we even
increment the int "tries" if pwd_tries is 0, which eventually would lead
to a signed integer overflow (and we would stop asking for passwords
after billion times).
In pwd_check we should close pipe file descriptors if fork fails. Also
it might be possible that waitpid fails if the root user tries to
sabotage the own passwd call. Let's just handle the error case here
as well to avoid accessing undefined content of "res".
Okay?
Tobias
Index: local_passwd.c
===
RCS file: /cvs/src/usr.bin/passwd/local_passwd.c,v
retrieving revision 1.63
diff -u -p -u -p -r1.63 local_passwd.c
--- local_passwd.c 10 Feb 2022 13:06:46 - 1.63
+++ local_passwd.c 5 May 2023 16:35:58 -
@@ -217,7 +217,7 @@ getnewpasswd(struct passwd *pw, login_ca
continue;
}
- if ((tries++ < pwd_tries || pwd_tries == 0) &&
+ if ((pwd_tries == 0 || tries++ < pwd_tries) &&
pwd_check(lc, p) == 0)
continue;
p = readpassphrase("Retype new password:", repeat,
sizeof(repeat),
Index: pwd_check.c
===
RCS file: /cvs/src/usr.bin/passwd/pwd_check.c,v
retrieving revision 1.17
diff -u -p -u -p -r1.17 pwd_check.c
--- pwd_check.c 28 Aug 2021 06:46:49 - 1.17
+++ pwd_check.c 5 May 2023 16:35:58 -
@@ -91,7 +91,7 @@ pwd_check(login_cap_t *lc, char *passwor
char *checker;
char *argp[] = { "sh", "-c", NULL, NULL};
int pipefds[2];
- pid_t child;
+ pid_t child, c;
uid_t uid;
gid_t gid;
@@ -114,6 +114,8 @@ pwd_check(login_cap_t *lc, char *passwor
switch (child = fork()) {
case -1:
warn("fork");
+ close(pipefds[0]);
+ close(pipefds[1]);
goto out;
case 0:
(void)signal(SIGINT, SIG_DFL);
@@ -183,11 +185,11 @@ pwd_check(login_cap_t *lc, char *passwor
}
/* get the return value from the child */
- while (waitpid(child, &res, 0) == -1) {
+ while ((c = waitpid(child, &res, 0)) == -1) {
if (errno != EINTR)
break;
}
- if (WIFEXITED(res) && WEXITSTATUS(res) == 0) {
+ if (c == child && WIFEXITED(res) && WEXITSTATUS(res) == 0) {
free(checker);
return (1);
}
