cups-browsed.c: create_queue() may leave make_model uninitialized

[solardiz]

I just happened to notice the below code snippet appearing twice at

cups-browsed/daemon/cups-browsed.c

Line 7511 in 1d1072a

make_model = (char*)malloc(sizeof(char) * 256);

and

cups-browsed/daemon/cups-browsed.c

Line 7705 in 1d1072a

make_model = (char*)malloc(sizeof(char)*256);

      make_model = (char*)malloc(sizeof(char) * 256);
      printer_attributes = get_cluster_attributes(p->queue_name);
      if ((attr = ippFindAttribute(printer_attributes,
                                   "printer-make-and-model",
                                   IPP_TAG_TEXT)) != NULL)
        strncpy(make_model, ippGetString(attr, 0, NULL),
                sizeof(make_model) - 1);

There are several issues here:

1. If the if condition above is false, the contents of make_model are left completely uninitialized.
2. strncpy doesn't NUL-terminate (it NUL-pads if space permits), so if the string is later used then uninitialized data may be processed after the string end.
3. sizeof(make_model) returns the size of the pointer, not of the memory allocation, so the copying is commonly limited to 7 characters followed by uninitialized memory.
4. If malloc returns NULL (on out-of-memory), this code will crash - but such issues are all over the place in this source file (perhaps a deliberate design decision, even if sloppy), so there's no point in singling out this one occurrence.

In both places, this is followed by code like:

    if (ppdfile == NULL)
    {
      // If we do not want CUPS-generated PPDs or we cannot obtain a
      // CUPS-generated PPD, for example if CUPS does not create a
      // temporary queue for this printer, we generate a PPD by
      // ourselves
      printer_ipp_response = (num_cluster_printers == 1) ? p->prattrs :
        printer_attributes;
      if (!ppdCreatePPDFromIPP2(ppdname, sizeof(ppdname), printer_ipp_response,
                                make_model,

so the potentially fully or partially uninitialized make_model may be actually used.

This may be a security issue, but that is not immediately clear (would need further analysis) and an individual bug like this may not matter much given the overall poor code quality (this is just something I happened to notice quickly).

Regardless, if this code is to stay, the bug (issued 1, 2, 3 above) should be fixed (in both places), e.g. quick-and-dirty (preserving code style):

      make_model = (char*)malloc(sizeof(char) * 256);
      *make_model = 0; /* Empty string for possibly strncat'ing to */
      printer_attributes = get_cluster_attributes(p->queue_name);
      if ((attr = ippFindAttribute(printer_attributes,
                                   "printer-make-and-model",
                                   IPP_TAG_TEXT)) != NULL)
        strncat(make_model, ippGetString(attr, 0, NULL), 255);

Note that this is deliberately strncat rather than strncpy, so that NUL termination and not NUL padding is done.

Ideally, code duplication should be avoided, and whether this code is needed at all reconsidered (if the truncation to 7 or even 3 characters went undetected for years, then perhaps this functionality didn't matter).