[cups.bugs] Re: [LOW] STR #1777: _cupsAdminGetServerSettings and _cupsAdminSetServerSettings are broken

[Roger Leigh]

Michael Sweet <mike at easysw.com> writes:

> Keep in mind that _cupsAdminGet/SetServerSettings are PRIVATE functions and
> not meant for general consumption yet.

Sure.  I'm only looking at them with respect to their use in
cgi-bin/admin.c in this case.  I probably didn't give enough context
in the original report:

I'm only refering to their use by this part of the web interface in
admin.tmpl:

<FORM METHOD="POST" ACTION="/admin">

<P><B>Basic Server Settings:</B></P>

<P><INPUT TYPE="HIDDEN" NAME="OP" VALUE="config-server">
<INPUT TYPE="CHECKBOX" NAME="REMOTE_PRINTERS" {?remote_printers}> Show printers shared by other systems<BR>
<INPUT TYPE="CHECKBOX" NAME="SHARE_PRINTERS" {?share_printers}> Share published printers connected to this system<BR>
<INPUT TYPE="CHECKBOX" NAME="REMOTE_ADMIN" {?remote_admin}> Allow remote administration<BR>
<INPUT TYPE="CHECKBOX" NAME="USER_CANCEL_ANY" {?user_cancel_any}> Allow users to cancel any job (not just their own)<BR>
<INPUT TYPE="CHECKBOX" NAME="DEBUG_LOGGING" {?debug_logging}> Save debugging information for troubleshooting</P>

<P><INPUT TYPE="IMAGE" SRC="/images/button-change-settings.gif" ALT="Change Settings"></P>

</FORM>}

The bug is only concerned with those 5 checkboxes, and is not
addressing the cupsd.conf editor here, though I do have concerns about
that.

> That said, in reply to your comments:
>
>     * does not cater for Include directives, so the settings it
>       returns are not representative of the system configuration.
>
> We cannot, for a lot of important security reasons, handle Include
> directives in the _cupsAdminGetServerSettings.

For setting, I can see several security issues.  Getting (simply
reading the existing config) doesn't appear too dangerous.  Could you
elaborate why that isn't supported?

> It is highly unlikely that we will ever support Include in any
> high-level configuration tools or interfaces provided with CUPS, and
> I highly recommend *against* using includes for this purpose!

The current Debian packages have files like these:

cat /etc/cups/cups.d/browse.conf
Browsing on

$ cat /etc/cups/cups.d/ports.conf
Listen /var/run/cups/cups.sock
Listen localhost:631
Listen hardknott.home.whinlatter.ukfsn.org:631

(This is my local, customised version of what is provided).

These are both included by cupsd.conf.

> As for:
>
>     * it should return the actual configuration in use by the server,
>       rather than parsing the config files.  What it parses isn't
>       necessarily in use.
>
> I don't see how we can do anything different unless we change cupsd to
> write a "current settings" status file somewhere and read that
> instead.

Does cupsd not have the configuration settings cached in memory?  I
fail to understand why we are reading it in at all, unless I'm missing
something.  After all, for the settings like LogLevel, Browsing etc.,
these are already known by cupsd, so why read them again?

(I'm referring just to those 5 checkboxes here.)

>     When writing out the config, it would be preferable if
>     _cupsAdminSetServerSettings wrote out individual settings such
>     as browsing, sharing, debugging were wrote out into separate
>     files which are then Included in the main cupsd.conf.  In Debian
>     these are in /etc/cups/cups.d/.  This would make it impossible
>     for the user's configuration to be damaged, while at the same
>     time being much simpler and reliable.
>
> Impossible?  You can still put bogus or incomplete data in any of those
> files, resulting in a damaged configuration.

Agreed, but perhaps if I demonstrate what I was thinking with an
example.

In _cupsAdminSetServerSettings:

if (debug_logging)
  write "LogLevel debug" to /etc/cups/cups.d/loglevel.conf
else
  write "LogLevel warning" to /etc/cups/cups.d/loglevel.conf

if (share_printers)
  write "Browsing on" to /etc/cups/cups.d/browse.conf
else
  write "Browsing off" to /etc/cups/cups.d/browse.conf

Having all these settings in separate files means that the
configuration can be managed safely by a number of methods

- the user
- the web front end
- scripts
- debconf (Debian package configuration)

and none of these methods are going to cause problems with any other.

> _cupsAdminSetServerSettings uses a HTTP PUT request to the server to
> upload a new cupsd.conf file to the server via /admin/conf.  You CAN'T
> create or access files in subdirectories of /etc/cups via HTTP requests,
> and we don't write files directly because a) the web interface programs
> don't have write permission (security), and b) doing it through HTTP
> provided enhanced access control, separate from the UNIX model.

Sorry, but I'm a little confused here.  I can see the problems you are
pointing out, but I don't understand why the Includes aren't being
expanded when it gets read in by the server.  I can understand the
issues of writing them back, but it could do the next best thing and
save cupsd.conf with them expanded in it (already expanded from when
it was read in before sending to the user for editing).

> IMHO, the proper "fix" for this issue is to remove support for the Include
> directive entirely, or have _cupsAdminGet/SetServerSettings error out if
> Include is used...

I do find it useful, so I hope you don't remove it just yet!  Erroring
out in this case is the best choice here, but I think Include could be
better catered for when simply reading (getting) the configuration.


Regards,
Roger

-- 
Roger Leigh
                Printing on GNU/Linux?  http://gutenprint.sourceforge.net/
                Debian GNU/Linux        http://www.debian.org/
                GPG Public Key: 0x25BFB848.  Please sign and encrypt your mail.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 188 bytes
Desc: not available
URL: <https://lists.cups.org/pipermail/cups/attachments/20060614/8fcd44d1/attachment.bin>