Make parsing of gecos field more robust and strict

[alejandro-colomar]

I finally understood what this function was doing. It was over-engineered, for supporting gecos fields that nobody should be writing in the first place. It was also bogus: if the gecos field was missing some subfield, it left the buffers with garbage. Even more buggy: if any of the first 4 CSV fields contained any '=' in them, those would be appended to the slop, regardless of oflg, which is inconsistent with the last lines, which parse the remaining slop depending on oflg. This code was very broken.

---

Revisions:

v2

• wfix
• Fix more bugs, and improve readability.

$ git range-diff shadow/master gh/gecos gecos 
1:  91431a96 ! 1:  8cfc1c19 src/chfn.c: Do not allow the 'slop' fields to appear before any non-slop gecos fields
    @@ Commit message
         2)  Building and room number or contact person
         3)  Office telephone number
         4)  Home telephone number
    -    5)  Any other contact information (pager number, fax, external e-mail address, etc.)
    +    5+) Any other contact information (pager number, fax, external e-mail address, etc.)
     
         But our code supported the "other contact information", which we call
         slop, and which is composed of an arbitrary number of key=value fields,
    @@ Commit message
     
         After this patch, the GECOS field is treated as a CSV, blindly copying
         the fields as they appear, where the first 4 fields are as specified
    -    above, and anything after them is the slop (5th field, any other contact
    +    above, and anything after them is the slop (5+ fields, any other contact
         information).
     
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
2:  d523246f = 2:  286739a2 src/chfn.c: Use strsep(3) and strcpy(3) instead of its pattern
3:  d624fbf0 = 3:  aa6755fe src/chfn.c: Write an empty string if there's nothing in the GECOS field
-:  -------- > 4:  f4b1950b src/chfn.c: slop: Reduce buffer size
-:  -------- > 5:  81e28291 src/chfn.c: Simplify checking for a long GECOS field
-:  -------- > 6:  4ac43002 src/chfn.c: Use stpeprintf() to improve readability

v2b

• Shorten comment.

$ git range-diff shadow/master gh/gecos gecos 
1:  8cfc1c19 = 1:  8cfc1c19 src/chfn.c: Do not allow the 'slop' fields to appear before any non-slop gecos fields
2:  286739a2 = 2:  286739a2 src/chfn.c: Use strsep(3) and strcpy(3) instead of its pattern
3:  aa6755fe = 3:  aa6755fe src/chfn.c: Write an empty string if there's nothing in the GECOS field
4:  f4b1950b = 4:  f4b1950b src/chfn.c: slop: Reduce buffer size
5:  81e28291 = 5:  81e28291 src/chfn.c: Simplify checking for a long GECOS field
6:  4ac43002 ! 6:  f01f131b src/chfn.c: Use stpeprintf() to improve readability
    @@ Commit message
         This allows us to split the formation of the string into several
         s*printf() calls.
     
    +    Shorten comment, to make it fit in one line.
    +
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## src/chfn.c ##
    @@ src/chfn.c: static void check_fields (void)
      
        sanitize_env ();
     @@ src/chfn.c: int main (int argc, char **argv)
    -    * Build the new GECOS field by plastering all the pieces together,
    -    * if they will fit ...
         */
    +   check_fields ();
    + 
    +-  /*
    +-   * Build the new GECOS field by plastering all the pieces together,
    +-   * if they will fit ...
    +-   */
     -  ret = SNPRINTF(new_gecos, "%s,%s,%s,%s%s%s",
     -                 fullnm, roomno, workph, homeph,
     -                 (!streq(slop, "")) ? "," : "", slop);
     -  if (ret == -1) {
    ++  /* Build the new GECOS field by plastering all the pieces together.  */
     +  p = new_gecos;
     +  e = new_gecos + countof(new_gecos);
     +  p = stpeprintf(p, e, "%s", fullnm);

v2c

• Add missing include.

$ git range-diff shadow/master gh/gecos gecos 
1:  8cfc1c19 = 1:  8cfc1c19 src/chfn.c: Do not allow the 'slop' fields to appear before any non-slop gecos fields
2:  286739a2 = 2:  286739a2 src/chfn.c: Use strsep(3) and strcpy(3) instead of its pattern
3:  aa6755fe = 3:  aa6755fe src/chfn.c: Write an empty string if there's nothing in the GECOS field
4:  f4b1950b = 4:  f4b1950b src/chfn.c: slop: Reduce buffer size
5:  81e28291 = 5:  81e28291 src/chfn.c: Simplify checking for a long GECOS field
6:  f01f131b ! 6:  bbdcc006 src/chfn.c: Use stpeprintf() to improve readability
    @@ Commit message
     
      ## src/chfn.c ##
     @@
    + #include "pwauth.h"
      #include "pwio.h"
      #include "shadowlog.h"
    ++#include "sizeof.h"
      #include "sssd.h"
     -#include "string/sprintf/snprintf.h"
     +#include "string/sprintf/stpeprintf.h"

[hallyn]

Hm, interesting. Yes, based on the code, I just assumed there were programs that wrote 'other' fields anywhere in the string. But I can't find anything that does. More interesting - the util-linux version of chfn does not allow changing the 'extra' field (as it calls it) at all!

So I do think this is safe.