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

Describe user-context-related caveat for screenlock table #7649

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

zhumo
Copy link
Contributor

@zhumo zhumo commented Jun 24, 2022

Problem

Since Apple locked down the screenlock data, Kollide and osquery implemented a workaround (https://www.kolide.com/blog/how-kolide-built-its-macos-screenlock-check). The screenlock table is only for the current user context, but the table spec did not add the user_data=True attribute to reflect it. This PR adds that attribute.

@zhumo zhumo marked this pull request as ready for review June 24, 2022 14:16
@zhumo zhumo requested review from a team as code owners June 24, 2022 14:16
directionless
directionless previously approved these changes Jun 24, 2022
Smjert
Smjert previously requested changes Jun 24, 2022
Copy link
Member

@Smjert Smjert left a comment

Choose a reason for hiding this comment

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

@zhumo @directionless This only adds a warning to pass the user as a constraint, but the table is not actually implementing the logic.

@directionless directionless dismissed their stale review June 24, 2022 17:22

Apparently not!

@zhumo
Copy link
Contributor Author

zhumo commented Jun 24, 2022

Yikes. I won't know how to do that.

So does this mean we don't need this doc change and the table is working fine as is? Or does this mean, someone should implement this functionality in the table?

@Smjert
Copy link
Member

Smjert commented Jun 24, 2022

Yikes. I won't know how to do that.

So does this mean we don't need this doc change and the table is working fine as is? Or does this mean, someone should implement this functionality in the table?

Well, I think it's a matter of what is the expected behavior?
On my macbook with 12.4 this seems to work fine, I can see it detect it's enabled and the grace period is 0, but was the expectation that one could read the settings for another user on the same machine or?
This is totally missing from the table (and anyway, it seems from your report that this is not possible anymore anyway).

@directionless
Copy link
Member

This table is kinda magic, I think we documented it in the original PR. Basically, the underlying library needs to run in a user's context. And the only way we've found to get it, is launchd runas. So there's no simple way, inside osquery, to pass a uid in.

@Smjert
Copy link
Member

Smjert commented Jul 3, 2022

This table is kinda magic, I think we documented it in the original PR. Basically, the underlying library needs to run in a user's context. And the only way we've found to get it, is launchd runas. So there's no simple way, inside osquery, to pass a uid in.

Oh I see, I realize now that I did the test incorrectly, because I was not running osquery via launchd, which I presume is what causes the issue, but I was launching it from terminal or using sudo.
There's something similar on Windows where there are some registry hives (of other users) that you cannot access unless you're logged in with that user.

I know we currently have some functionality that attempts to "drop" privileges (and sometimes actually change the osquery user to another user, not nobody), but it doesn't seem to fully work on macOS, only the gid gets changed, and the uid remains root (0).
I wonder if there's another way and if that would somehow solve the issue, or if you really need something else to be initialized as that user.

@directionless
Copy link
Member

I wonder if there's another way and if that would somehow solve the issue, or if you really need something else to be initialized as that user.

Dunno. launchd runas was the only thing we found. The user also has to have been recently logged in, so the underlying data is decrypted and available

@mike-myers-tob
Copy link
Member

It sounds like this PR may not improve the confusion around user context-specific tables. Does someone want to open a blueprint issue and maybe it can be added to the table specs or highlighted somehow in a new way? The user-context-specific problem is a big one for osquery but maybe there's a way to make it less confusing for users.

@zhumo zhumo force-pushed the mark-tables-with-user-context branch from c1a3a90 to d8f080c Compare July 13, 2022 19:57
@zhumo
Copy link
Contributor Author

zhumo commented Jul 13, 2022

Alright, I tried one more time to capture somewhat tersely what was discussed above.

@zhumo zhumo changed the title add user_data=True for screenlock table Describe user-context-related caveat for screenlock table Jul 19, 2022
@Smjert Smjert dismissed their stale review July 29, 2022 19:54

My concerns have been resolved

@zhumo
Copy link
Contributor Author

zhumo commented Aug 1, 2022

@Smjert @directionless Hello! wanted to ping you all again here... WDYT?

@Smjert
Copy link
Member

Smjert commented Aug 2, 2022

Closing and reopening to update the CI logic.

@Smjert Smjert closed this Aug 2, 2022
@Smjert Smjert reopened this Aug 2, 2022
@mike-myers-tob mike-myers-tob added this to the 5.5.0 milestone Aug 2, 2022
@mike-myers-tob mike-myers-tob merged commit 85d6d2f into osquery:master Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants