Bug 208676 - lprm is currently not honoring MLS restrictions
lprm is currently not honoring MLS restrictions
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: cups (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tim Waugh
: Reopened
Depends On:
Blocks: FC6Blocker 199636
  Show dependency treegraph
 
Reported: 2006-09-29 23:53 EDT by Matt Anderson
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version: 1.2.4-9
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-10 06:51:30 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Updated cups-lspp.patch to address lprm issue (64.07 KB, patch)
2006-09-29 23:53 EDT, Matt Anderson
no flags Details | Diff
Updated cups-lspp.patch which sets the mls levels correctly (68.79 KB, patch)
2006-10-02 04:07 EDT, Matt Anderson
no flags Details | Diff
Updated patch to address the issues raised by Tim (69.41 KB, patch)
2006-10-02 14:25 EDT, Matt Anderson
no flags Details | Diff
An else was mistakenly removed from the last patch (69.45 KB, patch)
2006-10-02 16:11 EDT, Matt Anderson
no flags Details | Diff
Ensure that the security context of a job or connection is always set (70.35 KB, patch)
2006-10-03 13:15 EDT, Matt Anderson
no flags Details | Diff
Check that context_new returned non-null before proceeding (71.15 KB, patch)
2006-10-04 12:03 EDT, Matt Anderson
no flags Details | Diff

  None (edit)
Description Matt Anderson 2006-09-29 23:53:41 EDT
Description of problem:
lprm currently does not support MLS

Version-Release number of selected component (if applicable):
1.2.4-2

How reproducible:
Everytime

Steps to Reproduce:
1. Enqueue a job in CUPS in MLS enforcing
2. lprm that job from another role
3. Note that the job was removed
  
Actual results:
The job can be removed from another role

Expected results:
Only the role that enqueued the job, and sysadm_r should be able to remove jobs

Additional info:
Please see attached patch as an updated cups-lspp.patch which, along with policy
updates that are being sent to the maintainer, resolves this issue.

*** The attached patch also obsoletes lspp-access so that needs to be removed
from the spec file.
Comment 1 Matt Anderson 2006-09-29 23:53:43 EDT
Created attachment 137462 [details]
Updated cups-lspp.patch to address lprm issue
Comment 2 Matt Anderson 2006-09-30 00:18:56 EDT
The earlier attached patch does handle roles correctly, but does not take into
consideration the job's level.  This needs to be addressed before the issue can
be considered resolved.
Comment 3 Matt Anderson 2006-10-02 04:07:18 EDT
Created attachment 137523 [details]
Updated cups-lspp.patch which sets the mls levels correctly
Comment 4 Tim Waugh 2006-10-02 07:05:24 EDT
Thanks for the updated patch.  I took a read through it to check for any problems.

This bit looks like it might leak some context_t objects on error.

+          tmpcon = context_new(spoolcon);
+          clicon = context_new(con->scon);
+          if (context_range_set(tmpcon, (context_range_get(clicon))) == -1)
+          {
+            cupsdSendError(con, HTTP_SERVER_ERROR);
+            return (cupsdCloseClient(con));
+          }
+          if (setfilecon(con->filename, context_str(tmpcon)) == -1)
+          {
+            cupsdSendError(con, HTTP_SERVER_ERROR);
+            return (cupsdCloseClient(con));
+          }
+          cupsdLogMessage(CUPSD_LOG_DEBUG2, "cupsdReadClient: %s set to %s",
+                          con->filename, context_str(tmpcon));
+          context_free(tmpcon);
+          context_free(clicon);

Similarly in ipp.c with printercon which never gets freed as far as I can tell,
and then later with jobcon and tmpcon.  There are several places like this in
the patch.

There are also some getfilecon() calls without a matching freecon().

This addition seems to be stray:
+  int                  remote_job;     /* Remote print job? */

There seems to be disagreement between different parts of the code about whether
to strip 'file:/' or 'file://' from the URI.
Comment 5 Matt Anderson 2006-10-02 14:25:53 EDT
Created attachment 137583 [details]
Updated patch to address the issues raised by Tim
Comment 6 Matt Anderson 2006-10-02 16:11:18 EDT
Created attachment 137595 [details]
An else was mistakenly removed from the last patch

The previous patch removed an else from printers.c which was needed when
attempting to perform a getfilecon() on a file that was not currently on the
system.  Such as a FileDevice printer where the file does not exist.
Comment 7 Tim Waugh 2006-10-03 08:29:24 EDT
Thanks!  Applied in 1.2.4-5.
Comment 8 Matt Anderson 2006-10-03 12:27:45 EDT
The previous patch did not check to make sure there was level information before
setting the level of the spool files.  Without that check all non-MLS
connections would error, and not be allowed to print.  I'm in the process of
testing the updated patch now.
Comment 9 Matt Anderson 2006-10-03 13:15:45 EDT
Created attachment 137672 [details]
Ensure that the security context of a job or connection is always set

The following patch ensures that when not running in an MLS mode the security
context is set.  Also it ensures that there is a valid security context before
completing any actions which operate on it.
Comment 10 Tim Waugh 2006-10-04 05:15:12 EDT
Matt, I'm going to have to pull this patch.  It's crashing cupsd now because
context_new("UNKNOWN SL") == 0 and there's no check on the result before it's used.
Comment 11 Tim Waugh 2006-10-04 05:17:13 EDT
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1208510240 (LWP 12431)]
0x001a2d67 in context_range_get () from /lib/libselinux.so.1
(gdb) up
#1  0x00bad996 in cupsdReadClient (con=0x9da6cd8) at client.c:1873
1873              if (context_range_set(tmpcon, (context_range_get(clicon))) == -1)
(gdb) info locals
line = "POST /printers/deskjet HTTP/1.1", '\0' <repeats 2125 times>,
"\n\000\000\000\200[��\030��\000\000\000\000\000�_�����\000\227_��\226[���c�����\000\227_��\226[��\001\004\000\000\000\000hoopoe.elk.\000lk.",
'\0' <repeats 1010 times>, "hoopoe.elk.\000lk.", '\0' <repeats 650 times>,
"d��\000\204b���g��\003\000\000\000xc��\204b��\f\000\000\000\204b�����\000\f\000\000\000yb��\200c��\223��\000yb�"...
operation =
"POST\000���!\203�\0008L�\t\234\030�\000\000\000\000\000\000\000\000\000\000�Y\035�\177�\000�����\t�\0008L�\t\000�Y\035�����0j\000�]�\t"
version =
"HTTP/1.1\000\237.\0006\001\000\0006\001\000\000t���W�\035\000\b�.\000\000�Y\035x����0j\000@M�\t���9\000�Y\035�\177�\000\000�Y\035"
locale = "\000\000\000\000\000\020\000\000P�.\000@\231.\000�\237.\000
�.\000HY�\tH���0�!\000 �.\000��\033\000\220���PY�\t����!\203�\000��\035"
major = 1
minor = 1
status = <value optimized out>
ipp_state = <value optimized out>
bytes = <value optimized out>
filename = 0x0
buf =
"P��\000\000��\000��\005\000x\234_\000�\177�\000PY�\t�\223�\000\210����n\201\000\000���\000roo�<`\000�<`\000�=`\000)f�\000\b\000\000\000PY�\t
\000\000\000\000\004\000\000�r�\000�\027�\t\000\000\000\000ܽl\000x���t���p���l���HY�\t\000\000\000\000\b\000\000\000�Y�\t\234Y�\tdS�\t/var/run/cups/certs/0\000�\t\030S�\t,S�\t\004S�\tEn\237vY��\216^�\233�vyQG�Fʵ",
'\0' <repeats 44 times>, "20"...
---Type <return> to continue, or q <return> to quit---
filestats = {st_dev = 0, __pad1 = 0, __st_ino = 0, st_mode = 0, st_nlink = 0,
  st_uid = 0, st_gid = 0, st_rdev = 0, __pad2 = 0, st_size = 0,
  st_blksize = 0, st_blocks = 0, st_atim = {tv_sec = 0, tv_nsec = 0},
  st_mtim = {tv_sec = 0, tv_nsec = 0}, st_ctim = {tv_sec = 0, tv_nsec = 0},
  st_ino = 0}
type = <value optimized out>
p = <value optimized out>
spoolcon = 0x9da6c60 "\230l�\t:object_r:print_spool_t"
clicon = (context_t) 0x0
tmpcon = (context_t) 0x9d8b678
request_id = 1
Comment 12 Matt Anderson 2006-10-04 11:04:23 EDT
(In reply to comment #10)
> Matt, I'm going to have to pull this patch.  It's crashing cupsd now because
> context_new("UNKNOWN SL") == 0 and there's no check on the result before it's
used.

Was that with patch id=137595 that you pulled into 1.2.4-5?  This is the bug I
was referencing when I re-opened the bug and sent patch id=137672.

Please let me know if you are seeing this issue with the updated patch.
Comment 13 Tim Waugh 2006-10-04 11:21:46 EDT
I see that bug with id=137672 as well.

+            clicon = context_new(con->scon);
+            tmpcon = context_new(spoolcon);
+            freecon(spoolcon);
+            if (context_range_set(tmpcon, (context_range_get(clicon))) == -1)

clicon is 0x0 at this point, and con->scon was "UNKNOWN SL".
Comment 14 Matt Anderson 2006-10-04 11:40:41 EDT
I can add the additional checks, but just before that section con->scon should
have been compared to UNKNOWN_SL, and that code wouldn't have been executed.
Comment 15 Matt Anderson 2006-10-04 12:03:38 EDT
Created attachment 137751 [details]
Check that context_new returned non-null before proceeding

The attached patch ensures that context_new returned a valid result before
operating on the context_t.  It also cleans up a compiler warning in client.c
that was a result of a change in CUPS proper, and updates the UNLABELED export
check to compare job->scon to UNKNOWN_SL in addition to null instead of just
null.
Comment 16 Tim Waugh 2006-10-04 12:14:34 EDT
I just re-tested this.  The package I built which should have had the id=137672
patch in.. didn't somehow. :-(

Thanks for id=137751; I'll check that in to CVS.  I'm afraid it's too late now
for Fedora Core 6 (bad timing :-( ), so we'll do a Fedora Core update to add in
the LSPP patch shortly after FC6 release.
Comment 17 Tim Waugh 2006-10-05 11:51:51 EDT
Fixed package is cups-1.2.4-8.
Comment 18 Tim Waugh 2006-10-10 06:51:30 EDT
..and now cups-1.2.4-9 has made it into the current Fedora Core 6 tree.

Note You need to log in before you can comment on or make changes to this bug.