insecure tmp file handling in hpcupsfax.cpp

[Johannes Meixner]

hplip-3.11.5 prnt/hpijs/hpcupsfax.cp contains:
------------------------------------------------------------
    if (iLogLevel & SAVE_PCL_FILE)
    {
        fp = fopen ("/tmp/hpcupsfax.out", "w");
        system ("chmod 666 /tmp/hpcupsfax.out");
    }
------------------------------------------------------------

This insecure tmp file handling results
potential read/write of arbitrary files.

[Johannes Meixner (jsmeix)]

I wonder why there is not any kind of response
from the HPLIP team regarding this security bug?

First and foremost I aks for a response whether or not
the HPLIP developers think it is actually a security bug.

As far as I see the crucial point to decide this is to know
under which exact circumstances the condition
"if (iLogLevel & SAVE_PCL_FILE)" becomes "true".

If "iLogLevel & SAVE_PCL_FILE" is true in in normal operation
(i.e. when it is true by default), it is a security bug.

In contrast if "iLogLevel & SAVE_PCL_FILE" is true only
under special circumstances it may be no security bug.

E.g. when only the user "root" can set someting
so that "iLogLevel & SAVE_PCL_FILE" is true
(e.g. somehow enable debugging mode which is
by default disabled for normal users)
when the issue is no security bug.

I do not have the knowledge to decide this.
Therefore an analysis from the HPLIP team is needed.

[Sanjay Kumar (sanjay-kumar14)]

Hi,

Sorry for the delay in response. Regarding the issue mentioned by you, here are my comments.

1) SAVE_PCL_FILE is a macro defined in ./prnt/hpcups/CommonDefinitions.h of HPLIP Source code. ie
#define BASIC_LOG 1
#define SAVE_PCL_FILE 2
#define SAVE_INPUT_RASTERS 4
#define SEND_TO_PRINTER_ALSO 8

2) iLogLevel is read from the file /etc/cups/cupsd.conf if following line is present in the file.

hpLogLevel <NUMBER>

Where <NUMBER> is the integer value betwwen 1-15 based on our need.
Note: This line is not present in the above file by default. For debugging purpose only root can add this line in /etc/cups/cupsd.conf and then collect the logs.

Therefore "if (iLogLevel & SAVE_PCL_FILE) " condition will be True only when /etc/cups/cupsd.conf file is modified in root mode and then cups is restarted. Since "iLogLevel & SAVE_PCL_FILE" is true only under special circumstances it should not be a security bug.

Thanks,
Sanjay

[Johannes Meixner (jsmeix)]

Many thanks for your analysis
and in particular very many thanks
for your thorough description which
helps a lot to understand the issue!

[Marcus Meissner (meissner)]

Sanjay, your explanation as given is not an excuse of writing insecure code, as there is
no way to "secure" enable this option on systems with potential malicious users.

A simple symlink left in /tmp/hpcupsfax.out to /etc/shadow or other relevant system files could be used by local attackers to make that file writable and ultimately gain root privileges.

[Sanjay Kumar (sanjay-kumar14)]

Hello Marcus,

Thanks for your comments. The code which you are seeing above is exercised only when there is a problem and we need to generate the log files and debug information. The log levels are enabled by the users and are exercised only temporarily to generate the logs. After logs are generated, they will revert it back to original settings.These files can be cleaned up by the users after generating the logs. Please let us know if this is still an issue.

[Johannes Meixner (jsmeix)]

Sanjay,
I wished there were comments in the HPLIP code which tell
how the stuff is meant (i.e. what the purpose and idea behind is).

Because I don't know for sure what the purpose and idea behind is
I can only make assumptions according to how I understand the code.

Based on this assumptions I think the following:

From my point of view "/tmp/hpcupsfax.out" is not meant
as a temporary file but as output file for debugging purpose
which (unfortunately) exists in a directory (/tmp)
where any user can create a symbolic link like for example
  /tmp/hpcupsfax.out -> /etc/passwd
and then when
  system ("chmod 666 /tmp/hpcupsfax.out")
would be run as root (I don't know under which user it runs)
it would do an evil thing.

When "/tmp/hpcupsfax.out" is meant as output file for debugging purpose
it would be not nice when the debugging output file name is not
a fixed name which is known in advance but instead it would be some
secure but awkward "mktemp" name like /tmp/hpcupsfax.out.XXXXXXXXXX

When "/tmp/hpcupsfax.out" is meant as output file for debugging purpose
I think it should be o.k. to remove an existing file or symbolic link
with this name via something like:

  if (iLogLevel & SAVE_PCL_FILE)
  {
    if (system ("rm -f /tmp/hpcupsfax.out"))
    { return 1;
    }
    fp = fopen ("/tmp/hpcupsfax.out", "w");
    system ("chmod 666 /tmp/hpcupsfax.out");
  }

But because of the stick bit in the /tmp/ directory
"rm -f /tmp/hpcupsfax.out" works only for root and for the user
who had created /tmp/hpcupsfax.out

I think this could be o.k. because when the stuff is run as root
it would enforce "the right thing" and when it is run as non-root
it would do "the right thing" when /tmp/hpcupsfax.out from
the same user already exists and otherwise it would return
something like a "failed" state as far as I guess the meaning
of "return 1" in prnt/hpijs/hpcupsfax.cpp

But I am not a security expert to finally decide about it.

[Sanjay Kumar (sanjay-kumar14)]

Hi Johannes,

Thanks for the detailed analysis of the problem. We will remove the hard coded name "hpcupsfax.out" for the temp file creation and instead create temp file using mkstemp() call. Please let us know if there is any issue.