insecure tmp file handling in hpcupsfax.cpp
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
| HPLIP |
High
|
Sanjay Kumar |
Bug Description
hplip-3.11.5 prnt/hpijs/
-------
if (iLogLevel & SAVE_PCL_FILE)
{
fp = fopen ("/tmp/
system ("chmod 666 /tmp/hpcupsfax.
}
-------
This insecure tmp file handling results
potential read/write of arbitrary files.
CVE References
Johannes Meixner (jsmeix) wrote : | #1 |
Sanjay Kumar (sanjay-kumar14) wrote : | #2 |
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/
#define BASIC_LOG 1
#define SAVE_PCL_FILE 2
#define SAVE_INPUT_RASTERS 4
#define SEND_TO_
2) iLogLevel is read from the file /etc/cups/
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/
Therefore "if (iLogLevel & SAVE_PCL_FILE) " condition will be True only when /etc/cups/
Thanks,
Sanjay
Changed in hplip: | |
status: | New → Invalid |
assignee: | nobody → Sanjay Kumar (sanjay-kumar14) |
Johannes Meixner (jsmeix) wrote : | #3 |
Many thanks for your analysis
and in particular very many thanks
for your thorough description which
helps a lot to understand the issue!
security vulnerability: | yes → no |
visibility: | private → public |
Marcus Meissner (meissner) wrote : | #4 |
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) wrote : | #5 |
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) wrote : | #6 |
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
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/
and then when
system ("chmod 666 /tmp/hpcupsfax.
would be run as root (I don't know under which user it runs)
it would do an evil thing.
When "/tmp/hpcupsfax
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.
When "/tmp/hpcupsfax
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.
{ return 1;
}
fp = fopen ("/tmp/
system ("chmod 666 /tmp/hpcupsfax.
}
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/
But I am not a security expert to finally decide about it.
Sanjay Kumar (sanjay-kumar14) wrote : | #7 |
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.
Changed in hplip: | |
status: | Invalid → Confirmed |
importance: | Undecided → High |
status: | Confirmed → Fix Committed |
Changed in hplip: | |
status: | Fix Committed → Fix Released |
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.