[cups.bugs] Fixing local job user name verification

Michael Sweet mike at easysw.com
Fri Jan 13 08:39:19 PST 2006


Martin Pitt wrote:
> Hi Cups developers!
> 
> For a fair while already (also in 1.2 snapshot 4892) cupsd does not
> check the request user against the job owner, i. e. any user can e. g.
> cancel jobs from other users. This is not desirable, and the code even
> tries to check that a user can only remove his own job. (At least
> that's the default setting in cupsd.conf).

First, thanks for finding and reporting this and getting as far as
you did - it will be easy to fix now...

> I tried to track this down a little:
> 
>  - cancel_job() calls validate_user().
>  - in validate_user(), con->username is empty since it's a local
>    connection. so it determines the request user name with
>    ippFindAttribute(con->request, "requesting-user-name", but doesn't
>    copy it to con->username.
>  - validate_user() calls cupsdCheckPolicy(), which calls
>    cupsdIsAuthorized().
>  - cupsdIsAuthorized() executes this bit of code which is totally
>    unclear to me:

Basically, we have an unauthenticated username in the IPP request
(the requested-user-name attribute) and possibly an authenticated
username from the HTTP request (the con->username member).

If we haven't authenticated (best->type is AUTH_NONE and this is
an IPP request) then the next best source for the username is the
IPP attribute.  Looking at it again, I think we want to check for
an authenticated username here as well (and only lookup the IPP
username if it isn't present...)

> 
> |  if (best->type == AUTH_NONE && best->limit == AUTH_LIMIT_IPP)
> |  {
> |   /*
> |    * Check for unauthenticated username...
> |    */
> |
> |    ipp_attribute_t     *attr;          /* requesting-user-name attribute */
> |
> |
> |    attr = ippFindAttribute(con->request, "requesting-user-name", IPP_TAG_NAME);
> |    if (attr)
> |    {
> |      cupsdLogMessage(CUPSD_LOG_DEBUG2,
> |                      "cupsdIsAuthorized: requesting-user-name=\"%s\"",
> |                      attr->values[0].string.text);
> |      return (HTTP_OK);
> |    }
> |  }
> 
>   This is the reason why every user can cancel other people's jobs:
>   the connection user is determined, but never checked against the
>   job's owner.

Right, we need to fall through and check the username and group
membership.

>  - If this part of the code is removed, then this code makes *every*
>    local request (even valid ones) fail, since con->username is empty:

Right, we still need that code, but we need to validate the username.

> |  if (!con->username[0])
> |  {
> |    if (best->satisfy == AUTH_SATISFY_ALL || auth == AUTH_DENY)
> |      return (HTTP_UNAUTHORIZED);       /* Non-anonymous needs user/pass */
> |    else
> |      return (HTTP_OK);                 /* unless overridden with Satisfy */
> |  }
> 
> So I created a crappy band-aid patch which fixes this for now: It
> disables the first quoted code part, and validate_user() copies the
> determined connection user into con->username, so that
> cupsdIsAuthorized() has a chance to compare it against the job owner.
> It's a minimally invasive patch, but not a really clean solution.

Right, and it has some side-effects for authentication that aren't
nice.

The attached patch is now in r4929 and fixes things...

Thanks, again, and let me know if you run into any other problems...

-- 
______________________________________________________________________
Michael Sweet, Easy Software Products           mike at easysw dot com
Internet Printing and Document Software          http://www.easysw.com
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: auth.patch
URL: <http://lists.cups.org/pipermail/cups-devel/attachments/20060113/fccd8554/attachment.ksh>


More information about the cups-devel mailing list