Rework shadow transitions and access #902
Conversation
aerusso
force-pushed
the
mrs/rework-auth-shadow
branch
from
09199c4 to
89766d5
Compare
|
I adjusted the rw_files_pattern macro to allow searching /etc (etc_t), and I think that's causing the lint failure. Should I just have it grant the useless dir search permission for shadow_lock_t, and expect that the etc_t search permission is granted by some other rule? |
policy/modules/system/authlogin.if
Outdated
| files_etc_filetrans($1, shadow_lock_t, file, ".pwd.lock") | ||
| files_etc_filetrans($1, shadow_lock_t, file, "passwd.lock") | ||
| files_etc_filetrans($1, shadow_lock_t, file, "group.lock") | ||
| rw_files_pattern($1, etc_t, shadow_lock_t) |
There was a problem hiding this comment.
I would try:
allow $1 shadow_lock_t:file rw_file_perms;
Probably already has enough permissions on etc_t directories.
There was a problem hiding this comment.
I separated this change out into another patch and moved the filetrans logic to another macro, so this issue can be handled separately.
policy/modules/system/authlogin.if
Outdated
| rw_files_pattern($1, shadow_lock_t, shadow_lock_t) | ||
| files_etc_filetrans($1, shadow_lock_t, file, ".pwd.lock") | ||
| files_etc_filetrans($1, shadow_lock_t, file, "passwd.lock") | ||
| files_etc_filetrans($1, shadow_lock_t, file, "group.lock") |
There was a problem hiding this comment.
Maybe these uses of 'files_etc_filetrans' belong in a new interface instead (something line auth_filetrans_shadow_lock). These give permission to transition on file creation, but not to actually create the file.
There was a problem hiding this comment.
Done, and I did something similar for the other file transitions. Let me know if this is what you were thinking of/if it's what you want.
There was a problem hiding this comment.
After further consideration, I think we should keep the filetranses in the original interfaces and use ifelse, like systemd_tmpfilesd_managed:
ifelse(`$2',`',`
files_etc_filetrans($1, shadow_lock_t, file, ".pwd.lock")
[...]
',`
refpolicywarn(`$0($*) second parameter is deprecated.')
files_etc_filetrans($1, shadow_t, file, $2)
')
|
With these changes have to tested changing a users' password (twice - due to files created after the first change)? And keep in mind that things behave differently in enforcing vs. permissive. |
policy/modules/system/authlogin.if
Outdated
| auth_etc_filetrans_shadow($1, "shadow-") | ||
| auth_etc_filetrans_shadow($1, "gshadow-") |
There was a problem hiding this comment.
Please update the existing auth_etc_filetrans_shadow instead.
There was a problem hiding this comment.
I handled this slightly differently, but hopefully it addresses your concern.
aerusso
force-pushed
the
mrs/rework-auth-shadow
branch
from
89766d5 to
2333d1c
Compare
|
I've also included some dpkg-specific changes, but (despite running Debian) have not tested them. This is in the final patch, and is motivated by a a read of the update-passwd.c source file. |
There are no directories labeled shadow_lock_t, and therefore is no reason to grant dir:search on shadow_lock_t. Signed-off-by: Antonio Enrico Russo <[email protected]>
shadow access is tightly controlled, with separate types for the shadow files and the locks. This patch distinguishes the two by enumerating the backup filenames and lock file names in their associated file transition rules. Prior to this, the overbroad file transition rules would cause various shadow-manipulating tools to create lock files with the incorrect shadow_t label. Signed-off-by: Antonio Enrico Russo <[email protected]>
aerusso
force-pushed
the
mrs/rework-auth-shadow
branch
from
2333d1c to
5ad8ee0
Compare
|
I think this is in good shape, though there were a few points I was unclear on. Sorry if I'm being dense on those! |
| ifelse(`$2',`',` | ||
| files_etc_filetrans($1, shadow_lock_t, file, ".pwd.lock") | ||
| files_etc_filetrans($1, shadow_lock_t, file, "passwd.lock") | ||
| files_etc_filetrans($1, shadow_lock_t, file, "group.lock") | ||
| ',` | ||
| refpolicywarn(`$0($*) second parameter is deprecated.') | ||
| files_etc_filetrans($1, shadow_t, file, $2) | ||
| ') |
There was a problem hiding this comment.
This is the right idea, but I meant to put it in auth_etc_filetrans_shadow(), making the changes in the other files unnecessary.
shadow access is tightly controlled, with separate types for the shadow files and the locks. This patch distinguishes the two by enumerating the backup filenames and lock file names in their associated file transition rules.
Prior to this, the overbroad file transition rules would cause various shadow-manipulating tools to create lock files with the incorrect shadow_t label.