[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