Bug 447444

Summary: AUDIT-0: cups-pk-helper
Product: [openSUSE] openSUSE 11.1 Reporter: Vincent Untz <vuntz>
Component: SecurityAssignee: Vincent Untz <vuntz>
Status: RESOLVED FIXED QA Contact: E-mail List <qa-bugs>
Severity: Normal    
Priority: P3 - Medium CC: security-team
Version: Factory   
Target Milestone: ---   
Hardware: Other   
OS: Other   
Whiteboard:
Found By: --- Services Priority:
Business Priority: Blocker: ---
Marketing QA Status: --- IT Deployment: ---
Bug Depends on:    
Bug Blocks: 439286    

Description Vincent Untz 2008-11-21 08:02:42 UTC
As requested by Ludwig in bug 439286, here's a bug to get cups-pk-helper audited. I'm sorry this comes so late -- I thought the requirement was known and that something was already going on :/

cups-pk-helper is a small PolicyKit helper that offers a dbus interface to configure cups. There are around 3000 lines of C code (using glib and cups). Various PolicyKit actions are now available, to help with the various configuration needs (eg, configure local printers without a password).

Note: please review version 0.0.3 -- old versions don't have some checks for arguments passed over dbus. It's available in GNOME:Factory and waiting for review in the openSUSE:Factory queue (submission #3894).
Comment 1 Ludwig Nussel 2008-11-21 08:05:37 UTC
the audit will quite certainly not be finished in time for 11.1 anymore
Comment 4 Sebastian Krahmer 2009-01-26 11:02:47 UTC
will have a look
Comment 5 Sebastian Krahmer 2009-03-30 09:54:33 UTC
Eventually, I found some time to finish this.
The code itself has good quality and I dont see any problems
with it like overflows, races or alike.

However, the security of our cups setup in future will
depend very much on the configuration of the PolicyKit rules
for cups and its helpers. If you allow user to add
stuff to cups config w/o requiring admin password you
are probably toast.

This is since I see inlining problems here which you
probably cant filter out all. This is due to the internal
parsing of CUPS' config-files inside cups itself.

config-files are parsed line by line, by reading
in a buffer of 1024 (or HTTPMAX_BUFFER, depending
whether it reads config or printers file etc.) bytes.
For cups after this chunk a new line begins. No matter of \n.
So imagine if you submit a config-tag that
has junk until 1024th byte, you can add a "Include" or
any other evil option to it which will receive cups-config-parser
like it was entered in a new line. So, in effect, even though you corretly
filter out \n characters via g_ascii_isprint(), you
have the chance to 'fake' cups a newline and arbitrary
config-options.
If you can do this as user you can trick it to load evil
filters, obtaining root privs.
Comment 6 Vincent Untz 2009-04-09 11:18:49 UTC
(In reply to comment #5)
> This is since I see inlining problems here which you
> probably cant filter out all. This is due to the internal
> parsing of CUPS' config-files inside cups itself.
> 
> config-files are parsed line by line, by reading
> in a buffer of 1024 (or HTTPMAX_BUFFER, depending
> whether it reads config or printers file etc.) bytes.
> For cups after this chunk a new line begins. No matter of \n.
> So imagine if you submit a config-tag that
> has junk until 1024th byte, you can add a "Include" or
> any other evil option to it which will receive cups-config-parser
> like it was entered in a new line. So, in effect, even though you corretly
> filter out \n characters via g_ascii_isprint(), you
> have the chance to 'fake' cups a newline and arbitrary
> config-options.

Ah, good to know. We could certainly limit the size to 256 characters, eg. The only case where I can really imagine this be a limitation is for properties which include a "reason" (like when putting a job on hold). But 256 characters is still a lot...

Don't know if that would be enough for you?
Comment 7 Vincent Untz 2009-04-21 16:05:45 UTC
I apparently forgot to NEEDINFO Sebastian.
Comment 8 Sebastian Krahmer 2009-04-22 06:35:12 UTC
Oh, sorry. Yes, limiting the input string to 256 bytes would
probably a good idea. I dont think there are longer printer names
or job queues etc.
Comment 9 Sebastian Krahmer 2009-05-06 11:42:08 UTC
After applying this to stable, can you close this bug?
Comment 10 Vincent Untz 2009-05-13 15:16:43 UTC
Sure. Just need some 30 free minutes to fix this ;-)
Comment 11 Sebastian Krahmer 2009-08-11 07:34:05 UTC
can we close this now?
Comment 12 Sebastian Krahmer 2009-11-09 14:00:44 UTC
ping
Comment 13 Vincent Untz 2010-02-16 20:47:50 UTC
Fixed in git, will be in the next update of the package. Let's just close the bug.