Re: Modify chpass(1) so it can set arbitrary fields

[Jean-Yves Migeon <jeanyves.migeon%free.fr@localhost>]

On 28.11.2011 19:46, Julian Fagir wrote:
> Hi,
>
> recently I needed chpass(1) to do change arbitrary fields without exporting
> intelligence to the caller (i.e., getting the passwd entry, changing the
> appropriate field, reput with -a).
> It seemed like this work was completely prepared, but not done. Why?
>
> Attached are diffs that introduce option -f to change exactly that, you can
> then set the fields, e.g. `chpass -l -f shell:/bin/sh gnrp`.
>
> Tested only locally, I don't have an yp setup. Perhaps it's of some use or
> you can tell my why I don't want to have it that way (as I said, it seemed to
> be prepared).

-       char *arg, *username = NULL;
+       char *arg, *opt, *username = NULL;
[...]
+       op = SINGLEENTRY;
+       arg = calloc(1, sizeof(char)*(7 + strlen(optarg)));
+       arg = strcpy(arg, "shell:");
+       arg = strncat(arg, optarg, sizeof(arg) - 7);
+       break;

First, you could use asprintf(3) instead of the calloc/string dance.

Second, unless mistaken, are you sure that your strncat() call is 
correct? sizeof(arg) - 7 looks like a very nice overflow to me, at least 
for anything that is 32 bits or lower. And for 64 bits, this would just 
copy 1 byte.

Please, be extra-careful when submitting patches to setuid() binaries.

(I did not look at the whole patch, but these two look already quite 
critical to me)

--
Jean-Yves Migeon
jeanyves.migeon%free.fr@localhost