Re: login_passwd.c (etc.) and auth_mkvalue(3) returning NULL

2020-12-31 Thread Todd C . Miller
On Thu, 31 Dec 2020 15:27:02 +1100, Ross L Richardson wrote:

> It could, of course, just use a fixed string rather than the "%s" format,
> although the latter is certainly clear(er) and consistent.

I originally had a fixed string but decided that using the "%s"
format was clearer.

> With auth_mkvalue() not being used, I don't think it needs to include
>  any more.

Good catch.

 - todd



Re: login_passwd.c (etc.) and auth_mkvalue(3) returning NULL

2020-12-30 Thread Ross L Richardson
On Wed, Dec 30, 2020 at 09:08:53AM -0700, Todd C. Miller wrote:
>[...]
> Yes, that should be checked.  In the case of login_passwd.c there
> is really no reason to use auth_mkvalue(3) at all as there is nothing
> that needs to be escaped.  I think the simplest approach is to send
> a reject message if there is a memory allocation error.
> 
>  - todd
> 
> Index: login_passwd/login_passwd.c
> ===
> RCS file: /cvs/src/libexec/login_passwd/login_passwd.c,v
> retrieving revision 1.18
> diff -u -p -u -r1.18 login_passwd.c
> --- login_passwd/login_passwd.c   15 May 2020 17:25:39 -  1.18
> +++ login_passwd/login_passwd.c   30 Dec 2020 16:05:30 -
> @@ -121,7 +121,7 @@ main(int argc, char *argv[])
>   }
>   if (wheel != NULL && strcmp(wheel, "yes") != 0) {
>   fprintf(back, BI_VALUE " errormsg %s\n",
> - auth_mkvalue("you are not in group wheel"));
> + "you are not in group wheel");
>   fprintf(back, BI_REJECT "\n");
>   exit(1);
>   }
>[...]

It could, of course, just use a fixed string rather than the "%s" format,
although the latter is certainly clear(er) and consistent.

With auth_mkvalue() not being used, I don't think it needs to include
 any more.

Thanks,
Ross



Re: login_passwd.c (etc.) and auth_mkvalue(3) returning NULL

2020-12-30 Thread Todd C . Miller
On Wed, 30 Dec 2020 15:34:34 +1100, Ross L Richardson wrote:

> auth_mkvalue(3) may return NULL (if no memory is available), but
> login_passwd.c and friends use the return value without checking.

Yes, that should be checked.  In the case of login_passwd.c there
is really no reason to use auth_mkvalue(3) at all as there is nothing
that needs to be escaped.  I think the simplest approach is to send
a reject message if there is a memory allocation error.

 - todd

Index: login_passwd/login_passwd.c
===
RCS file: /cvs/src/libexec/login_passwd/login_passwd.c,v
retrieving revision 1.18
diff -u -p -u -r1.18 login_passwd.c
--- login_passwd/login_passwd.c 15 May 2020 17:25:39 -  1.18
+++ login_passwd/login_passwd.c 30 Dec 2020 16:05:30 -
@@ -121,7 +121,7 @@ main(int argc, char *argv[])
}
if (wheel != NULL && strcmp(wheel, "yes") != 0) {
fprintf(back, BI_VALUE " errormsg %s\n",
-   auth_mkvalue("you are not in group wheel"));
+   "you are not in group wheel");
fprintf(back, BI_REJECT "\n");
exit(1);
}
Index: login_radius/login_radius.c
===
RCS file: /cvs/src/libexec/login_radius/login_radius.c,v
retrieving revision 1.9
diff -u -p -u -r1.9 login_radius.c
--- login_radius/login_radius.c 27 Apr 2017 20:55:52 -  1.9
+++ login_radius/login_radius.c 30 Dec 2020 16:01:26 -
@@ -183,19 +183,29 @@ main(int argc, char **argv)
strcmp(service, "login") ? challenge : NULL, password, );
 
if (c == 0) {
-   if (*challenge == '\0')
+   if (*challenge == '\0') {
(void)fprintf(back, BI_AUTH "\n");
-   else {
-   (void)fprintf(back, BI_VALUE " challenge %s\n",
-   auth_mkvalue(challenge));
-   (void)fprintf(back, BI_CHALLENGE "\n");
+   exit(0);
+   } else {
+   char *val = auth_mkvalue(challenge);
+   if (val != NULL) {
+   (void)fprintf(back, BI_VALUE " challenge %s\n",
+   val);
+   (void)fprintf(back, BI_CHALLENGE "\n");
+   exit(0);
+   }
+   emsg = "unable to allocate memory";
+   }
+   }
+   if (emsg != NULL) {
+   if (strcmp(service, "login") == 0) {
+   (void)fprintf(stderr, "%s\n", emsg);
+   } else {
+   emsg = auth_mkvalue(emsg);
+   (void)fprintf(back, BI_VALUE " errormsg %s\n",
+   emsg ? emsg : "unable to allocate memory");
}
-   exit(0);
}
-   if (emsg && strcmp(service, "login") == 0)
-   (void)fprintf(stderr, "%s\n", emsg);
-   else if (emsg)
-   (void)fprintf(back, "value errormsg %s\n", auth_mkvalue(emsg));
if (strcmp(service, "challenge") == 0) {
(void)fprintf(back, BI_SILENT "\n");
exit(0);
Index: login_skey/login_skey.c
===
RCS file: /cvs/src/libexec/login_skey/login_skey.c,v
retrieving revision 1.28
diff -u -p -u -r1.28 login_skey.c
--- login_skey/login_skey.c 28 Jun 2019 13:32:53 -  1.28
+++ login_skey/login_skey.c 30 Dec 2020 16:04:16 -
@@ -157,8 +157,14 @@ main(int argc, char *argv[])
case MODE_CHALLENGE:
haskey = (skeychallenge2(fd, , user, challenge) == 0);
strlcat(challenge, "\nS/Key Password:", sizeof(challenge));
-   fprintf(back, BI_VALUE " challenge %s\n",
-   auth_mkvalue(challenge));
+   cp = auth_mkvalue(challenge);
+   if (cp == NULL) {
+   (void)fprintf(back, BI_VALUE " errormsg %s\n",
+   "unable to allocate memory");
+   (void)fprintf(back, BI_REJECT "\n");
+   exit(1);
+   }
+   fprintf(back, BI_VALUE " challenge %s\n", cp);
fprintf(back, BI_CHALLENGE "\n");
if (haskey) {
fprintf(back, BI_FDPASS "\n");
Index: login_token/login_token.c
===
RCS file: /cvs/src/libexec/login_token/login_token.c,v
retrieving revision 1.16
diff -u -p -u -r1.16 login_token.c
--- login_token/login_token.c   28 Jun 2019 13:32:53 -  1.16
+++ login_token/login_token.c   30 Dec 2020 16:04:31 -
@@ -152,8 +152,14 @@ main(int argc, char *argv[])
tt->proper);
(void)sigprocmask(SIG_UNBLOCK, , NULL);
if (mode == 1) {
-