-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
IPC security - Allow policy targets to be symlinks #1173
IPC security - Allow policy targets to be symlinks #1173
Conversation
46c7f6b
to
c60d202
Compare
Before I consider this I think we also need to enforce that the program that ends up being executed is owned by root and not world-writable. |
Gladly, but can you clue me in on the reasoning? I can't make out the attack vector. If you're going to nuke it from space, what's the purpose of having such fine-grain control over IPC interactions per program? |
c60d202
to
6a59049
Compare
If you can write to a program that has security policies, then you can just overwrite it with a program whose behavior is more sinister. |
common/util.c
Outdated
if (r == -1 || r > sb.st_size) { | ||
goto failed; | ||
} | ||
resolved[r] = '\0'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about recursive symlinks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I was assuming readlink
handled this, but I realize now that doesn't make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
sway/commands/permit.c
Outdated
char *program = NULL; | ||
|
||
if (!(program = resolve_path(name))) { | ||
sway_abort("Unable to allocate security policy"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abort is probably not the correct action here, use sway_assert and return a policy with no permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolve_path returns error only if malloc fails...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
sway/commands/permit.c
Outdated
@@ -54,6 +60,10 @@ static struct feature_policy *get_policy(const char *name) { | |||
} | |||
list_add(config->feature_policies, policy); | |||
} | |||
sway_assert(!strcmp(program, policy->program), | |||
"!! DANGER !! symlink modified during policy allocation."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case you need to return no permissions as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I don't think this ever happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Time of check/Time of use. The symlink could be switched under us from the time we validate to the time we create the policy. alloc_policy goes through the resolve independently, on purpose (for error reporting).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, sway_assert
doesn't abort??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it logs the error and returns the first parameter. Sway should never abort at runtime, because that would kill your whole session and cause data loss and things like that. It's just a utility function to add conditional logging, the user has to implement a safe exit path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, you should be storing the fully resolved symlink. When we validate the policy we're going to get the fully resolved path from /proc/[pid]/exe
, so we need to compare against that to get the appropriate policy. All symlink resolution should happen when the policy is parsed and never again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, you should be storing the fully resolved symlink.
All symlink resolution should happen when the policy is parsed and never again.
That's what's stored in policy->program, yeah.
Sway should never abort at runtime,
got it. but this was during config parsing/startup. Anyway, the race condition couldn't be solved this way (it just moved it elsewhere) so I had to do things differently and this check eliminated now.
Sure but if you can't write to the program, why go through all the effort to sandbox it ? defense in depth I guess. |
6a59049
to
eae3a19
Compare
A number of reasons. First, because we can limit what the program can access in the first place, so you can give limited permissions to mixed trust programs. More importantly, though, if we didn't have access controls at all then you could leverage bash or sed or something similar to arbitrarily work around restrictions. |
e9efcd1
to
daf1768
Compare
common/util.c
Outdated
free(resolved); | ||
resolved = NULL; | ||
} | ||
failed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably return NULL in case of failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was deliberate because it simplified handling of "*", but I've changed it.
8b27daa
to
5c21514
Compare
sway/security.c
Outdated
return true; | ||
|
||
failed: | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for the failed label and all the gotos in this function, just return false wherever you would goto failed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
sway/security.c
Outdated
if (!policy) { | ||
sway_abort ("Unable to allocate security policy"); | ||
} | ||
list_add (config->feature_policies, policy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit, don't put a space between function name and parenthesis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Muscle memory. Until Sway becomes my focus project my default clang-format belongs to something else.
Fixed.
@SirCmpwn, ready for review. I reworked the whole thing and some positive collateral cleanup to existing code resulted.
The final commit Here's the test script I used (run from build directory, in X):
I've spent this much time on it because it got delicate and therefore interesting, so fun. But I'd be the first to admit the result was more subtle than anticipated and maybe just barfing a log error on symlink (which was the alternative I suggested in #1172) might have been a suitable and far simpler solution. That said, as this point I think this PR does what it claims, and the test script should provide some confidence. |
I'm not really a huge fan of this distinction, I think it's not very important. It would be better to just resolve symlinks on policy creation and call it a day. No one is going to carry around references to unresolved paths. |
Also, for development purposes it probably makes sense not to enforce that the binary is owned by root for debug builds. Useful for i.e. hacking on swaybar. |
aa54ef0
to
014bca0
Compare
it's really only there for documentation purposes, but I eliminated the renaming and played down the issue. Smaller patch.
That statement is not at odds with what the code already did/does. I was worried that future contributors/changes would overlook the distinction, which is important because canonization needs to be done everywhere or it doesn't work, and this case has security implications. But you're a careful reviewer, so It's probably ok to just let it go.
Wrapped in |
sway/commands/permit.c
Outdated
@@ -84,7 +84,7 @@ struct cmd_results *cmd_reject(int argc, char **argv) { | |||
return error; | |||
} | |||
|
|||
struct feature_policy *policy = get_policy(argv[0]); | |||
struct feature_policy *policy = get_feature_policy(argv[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you need to resolve symlinks here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I missed that.
Scenario: suppose a policy file indicates a reject target against a file which does not exist. if we ignore the resolve fail and use the verbatim policy target, and the target is then created as a symlink to elsewhere, the reject policy won't be found when get_pid_exe is used to look it up.
How do you want to handle that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's niche enough that we don't need to bother handling it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Looks good aside from minor nits, will merge once resolved. Thanks! |
014bca0
to
66e2019
Compare
- When policies are allocated, the ipc target path goes through symlink resolution. The result is used as the canonical for matching pids to policies at runtime. In particular, this matches up with the target of the `/proc/<pid>/exe`. - There's a possible race condition if this isn't done correctly, read below. Originally, validate_ipc_target() always tried to resolve its argument for symlinks, and returned a parogram target string if it validates. This created a possible race condition with security implications. The problem is that get_feature_policy() first independently resolved the policy target in order to check whether a policy already exists. If it didn't find any, it called alloc_feature_policy() which called validate_ipc_target() which resolved the policy target again. In the time between the two checks, the symlink could be altered, and a lucky attacker could fool the program into thinking that a policy doesn't exist for a target, and then switch the symlink to point at another file. At the very least this could allow him to create two policies for the same program target, and possibly to bypass security by associating the permissions for one target with another, or force default permissions to apply to a target for which a more specific rule has been configured. So we don't that. Instead, the policy target is resolved once and that result is used for the rest of the lookup/creation process.
66e2019
to
2ad8850
Compare
Thanks! |
closes #1172