/shadow

Make shadow more robust in hostile environments #4

Merged
merged 5 commits into from Nov 13, 2015

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@xnox @hallyn
@xnox
Contributor

xnox commented Mar 20, 2015

I'm patching shadow to be more robust when operating in a-typical environments, but these improvements are general enough, that I believe warrant inclusion upstream by default.

Specifically there are deployments that use nss-altfiles / nss-extrausers and thus ship alternative group/passwd/shadow/gshadow files elsewhere on the filesystem (e.g. /var/lib/passwd). In such configurations admin modifiable files /etc/passwd, /etc/group and so on may not exist. Furthermore if one is bootstrapping a new distribution from scratch, it would be nice to point shadow utilities at an empty /etc and start creating default system accounts with useradd/usermod/groupadd/etc utilities without writing initial files by hand. Hence these changes:

  • create all databases, with correct (?! used typical permissions here, please correct if different defaults are desired) permissions, even if they are missing / are empty.
  • next /etc/shadow file existence is used as a flag file, whether or not shadow passwords should be used. I think this is very odd, hence I add FORCE_SHADOW option to make sure shadow/gshadow are used, even if those files are non-existent.
  • during testing I have noticed that when shadow is compiled with PAM support, settings that are not-applicable any more, but present in the stock /etc/login.defs are being complained about. So I made a change for shadow to not complain about well-known settings, which are not in effect when compiled with PAM support. If this is undesirable, maybe instead we would want to pre-process login.defs at compile time to make sure they do not contain any unknown settings in a given configuration (with/without pam, with/without subuids, etc.)
  • Lastly, I noticed that login command would bail out and exit, if /etc/login.defs file is not present on disk. I find that very odd - given that there are compiled in defaults for every single value for all shadow utils to operate normally. Thus I made it non fatal for login to operate without /etc/login.defs.

Overall my goal is to have fully usable system with empty /etc and with these initial patches this is achievable. At the moment I'm also working on adding full usermod support, when operating with nss-altfiles.

Please review and consider including these patches.

xnox added some commits Feb 27, 2015

@xnox
Do not bail out on missing login.defs. 
For most operations tools have compiled-in defaults, and thus can
operate without login.defs present.
This user has not uploaded their public key yet.
GPG key ID: 43388EE7C151C9C9 Learn about signing commits
51c1fc9
@xnox
Do not report unknown settings, when compiled with PAM. 
When compiled with PAM certain settings are not used, however they are
still defined in the stock login.defs file. Thus every command reports
them as "unknown setting contact administrator".

Alternative would be to parse stock login.defs and comment out/remove
settings that are not applied, when compiled with PAM.
This user has not uploaded their public key yet.
GPG key ID: 43388EE7C151C9C9 Learn about signing commits
71c6165
@xnox
Do not fail on missing files in /etc/, create them instead. 
passwd, shadow, group, gshadow etc. can be managed via nss -
e.g. system default accounts can be specified using nss_altfiles,
rather than in /etc/. Thus despite having default accounts, these
files can be missing on disk and thus should be opened with O_CREATE
whenever they are attempted to be opened in O_RDWR modes.
This user has not uploaded their public key yet.
GPG key ID: 43388EE7C151C9C9 Learn about signing commits
ee43f47
@xnox
Force use shadow, even if missing.
This user has not uploaded their public key yet.
GPG key ID: 43388EE7C151C9C9 Learn about signing commits
46a72bc
@xnox
Create dbs with correct permissions.
This user has not uploaded their public key yet.
GPG key ID: 43388EE7C151C9C9 Learn about signing commits
bab349b
@hallyn

hallyn commented on 46a72bc Mar 20, 2015

This one concerns me because it could cause change of behavior on existing (admittedly weird) systems, right?
Owner

xnox replied Mar 31, 2015

Sure, but no such system exist in a functional state. Everyone ships /etc/shadow in e.g. base-files package or similar, and it's accidental removal renders system unusable.
Owner

xnox replied Apr 1, 2015

Well, as implemented here, FORCE_SHADOW is set to false by default, and thus current behaviour is preserved. In my deployments I will be setting it to "yes".
@hallyn

hallyn commented on ee43f47 Mar 20, 2015

Each of the *_open files hooked here has a corresponding *_close file which calls commonio_close, which will set the new file's perms. However, it looks to me like it sets the new file's perms to be the perms on the open file. So I think this patch needs something more to set the appropriate perms on each file.

Also looks like perhaps this should go into commonio_open() instead of each of the callers? Or are there cases where we dont' want this done?
Owner

xnox replied Apr 1, 2015

"However, it looks to me like it sets the new file's perms to be the perms on the open file." - I don't follow this comment. If commonio_open is called with O_CREATE option, yet said database does not exist on disk, the database is "opened" but no file is created on disk / no file pointer is created. Then at commonio_close time, the database will be created with desired default permissions. Otherwise, if database does exist, it is opened for real (fp exists) and at close time an atomic update is performed without adjustment of the file permissions.

I do not want to modified the permissions, as e.g. other tools have other default permissions for these files. E.g. PAM uses 000 for shadow file.

hallyn replied Apr 6, 2015

@hallyn
Contributor

hallyn commented Aug 10, 2015

Sorry, I lost track of this, I thought you were discussing it with someone else. Should we merge this at this point?

hallyn added a commit that referenced this pull request Nov 13, 2015

@hallyn
Merge pull request #4 from xnox/master 
Make shadow more robust in hostile environments
e01bad7

@hallyn hallyn merged commit e01bad7 into shadow-maint:master Nov 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment