Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dest.c: Don't look for user config in cupsGetNamedDest as root #1076

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

zdohnal
Copy link
Member

@zdohnal zdohnal commented Oct 7, 2024

We were still looking into ~/.cups/lpoptions as root in the function, which IMHO is not expected.

Copy link
Member

@michaelrsweet michaelrsweet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess. cupsSetDests never writes there so it should just be a no-op already, but it doesn't hurt to add a check. For readability, maybe just put the #ifdef around the whole if, e.g.:

#ifdef _WIN32
  if (cg->userconfig)
#else 
  if (cg->userconfig && getuid())
#endif // _WIN32

We were still looking into ~/.cups/lpoptions as root in the function,
which IMHO is not expected.
@zdohnal
Copy link
Member Author

zdohnal commented Oct 8, 2024

@michaelrsweet thx, I have applied that - additionally I found another place where we should add the check - see the updated PR.

What I experience there is Selinux AVC when an application which runs as a root (f.e. cups-browsed) uses libcups dest API and attempts to access /root/.cups/lpoptions, which is not set in policy as allowed. And since we want to access /etc/cups/lpoptions as root, it makes more sense to fix libcups than adding new selinux policy rule.

Copy link
Member

@michaelrsweet michaelrsweet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, looks good to me.

@zdohnal zdohnal merged commit b2a3880 into OpenPrinting:master Oct 9, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants