In 9eb191edc4 I included a free() that
frees the members variable, which in turn causes the comma_to_list()
function to return an array of empty elements. The array variable holds
a list of pointers that point to offsets of the members variable. When
the function succeeds freeing members variable causes the elements of
the array variable to point to an empty string.
This is causing several regressions in our internal testing environment.
So, I'm reverting the change.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Error: RESOURCE_LEAK (CWE-772): [#def1]
shadow-4.8.1/lib/commonio.c:320: alloc_fn: Storage is returned from allocation function "fopen_set_perms".
shadow-4.8.1/lib/commonio.c:320: var_assign: Assigning: "bkfp" = storage returned from "fopen_set_perms(backup, "w", &sb)".
shadow-4.8.1/lib/commonio.c:329: noescape: Resource "bkfp" is not freed or pointed-to in "putc".
shadow-4.8.1/lib/commonio.c:334: noescape: Resource "bkfp" is not freed or pointed-to in "fflush".
shadow-4.8.1/lib/commonio.c:339: noescape: Resource "bkfp" is not freed or pointed-to in "fileno".
shadow-4.8.1/lib/commonio.c:342: leaked_storage: Variable "bkfp" going out of scope leaks the storage it points to.
340| || (fclose (bkfp) != 0)) {
341| /* FIXME: unlink the backup file? */
342|-> return -1;
343| }
344|
Error: RESOURCE_LEAK (CWE-772): [#def2]
shadow-4.8.1/libmisc/addgrps.c:69: alloc_fn: Storage is returned from allocation function "malloc".
shadow-4.8.1/libmisc/addgrps.c:69: var_assign: Assigning: "grouplist" = storage returned from "malloc(i * 4UL)".
shadow-4.8.1/libmisc/addgrps.c:73: noescape: Resource "grouplist" is not freed or pointed-to in "getgroups". [Note: The source code implementation of the function has been overridden by a builtin model.]
shadow-4.8.1/libmisc/addgrps.c:126: leaked_storage: Variable "grouplist" going out of scope leaks the storage it points to.
124| }
125|
126|-> return 0;
127| }
128| #else /* HAVE_SETGROUPS && !USE_PAM */
Error: RESOURCE_LEAK (CWE-772): [#def3]
shadow-4.8.1/libmisc/chowntty.c:62: alloc_fn: Storage is returned from allocation function "getgr_nam_gid".
shadow-4.8.1/libmisc/chowntty.c:62: var_assign: Assigning: "grent" = storage returned from "getgr_nam_gid(getdef_str("TTYGROUP"))".
shadow-4.8.1/libmisc/chowntty.c:98: leaked_storage: Variable "grent" going out of scope leaks the storage it points to.
96| */
97| #endif
98|-> }
99|
Error: RESOURCE_LEAK (CWE-772): [#def4]
shadow-4.8.1/libmisc/copydir.c:742: open_fn: Returning handle opened by "open". [Note: The source code implementation of the function has been overridden by a user model.]
shadow-4.8.1/libmisc/copydir.c:742: var_assign: Assigning: "ifd" = handle returned from "open(src, 0)".
shadow-4.8.1/libmisc/copydir.c:748: leaked_handle: Handle variable "ifd" going out of scope leaks the handle.
746| #ifdef WITH_SELINUX
747| if (set_selinux_file_context (dst, NULL) != 0) {
748|-> return -1;
749| }
750| #endif /* WITH_SELINUX */
Error: RESOURCE_LEAK (CWE-772): [#def5]
shadow-4.8.1/libmisc/copydir.c:751: open_fn: Returning handle opened by "open". [Note: The source code implementation of the function has been overridden by a user model.]
shadow-4.8.1/libmisc/copydir.c:751: var_assign: Assigning: "ofd" = handle returned from "open(dst, 577, statp->st_mode & 0xfffU)".
shadow-4.8.1/libmisc/copydir.c:752: noescape: Resource "ofd" is not freed or pointed-to in "fchown_if_needed".
shadow-4.8.1/libmisc/copydir.c:775: leaked_handle: Handle variable "ofd" going out of scope leaks the handle.
773| ) {
774| (void) close (ifd);
775|-> return -1;
776| }
777|
Error: RESOURCE_LEAK (CWE-772): [#def7]
shadow-4.8.1/libmisc/idmapping.c:188: alloc_fn: Storage is returned from allocation function "xmalloc".
shadow-4.8.1/libmisc/idmapping.c:188: var_assign: Assigning: "buf" = storage returned from "xmalloc(bufsize)".
shadow-4.8.1/libmisc/idmapping.c:188: var_assign: Assigning: "pos" = "buf".
shadow-4.8.1/libmisc/idmapping.c:213: noescape: Resource "buf" is not freed or pointed-to in "write".
shadow-4.8.1/libmisc/idmapping.c:219: leaked_storage: Variable "pos" going out of scope leaks the storage it points to.
shadow-4.8.1/libmisc/idmapping.c:219: leaked_storage: Variable "buf" going out of scope leaks the storage it points to.
217| }
218| close(fd);
219|-> }
Error: RESOURCE_LEAK (CWE-772): [#def8]
shadow-4.8.1/libmisc/list.c:211: alloc_fn: Storage is returned from allocation function "xstrdup".
shadow-4.8.1/libmisc/list.c:211: var_assign: Assigning: "members" = storage returned from "xstrdup(comma)".
shadow-4.8.1/libmisc/list.c:217: var_assign: Assigning: "cp" = "members".
shadow-4.8.1/libmisc/list.c:218: noescape: Resource "cp" is not freed or pointed-to in "strchr".
shadow-4.8.1/libmisc/list.c:244: leaked_storage: Variable "cp" going out of scope leaks the storage it points to.
shadow-4.8.1/libmisc/list.c:244: leaked_storage: Variable "members" going out of scope leaks the storage it points to.
242| if ('\0' == *members) {
243| *array = (char *) 0;
244|-> return array;
245| }
246|
Error: RESOURCE_LEAK (CWE-772): [#def11]
shadow-4.8.1/libmisc/myname.c:61: alloc_fn: Storage is returned from allocation function "xgetpwnam".
shadow-4.8.1/libmisc/myname.c:61: var_assign: Assigning: "pw" = storage returned from "xgetpwnam(cp)".
shadow-4.8.1/libmisc/myname.c:67: leaked_storage: Variable "pw" going out of scope leaks the storage it points to.
65| }
66|
67|-> return xgetpwuid (ruid);
68| }
69|
Error: RESOURCE_LEAK (CWE-772): [#def12]
shadow-4.8.1/libmisc/user_busy.c:260: alloc_fn: Storage is returned from allocation function "opendir".
shadow-4.8.1/libmisc/user_busy.c:260: var_assign: Assigning: "task_dir" = storage returned from "opendir(task_path)".
shadow-4.8.1/libmisc/user_busy.c:262: noescape: Resource "task_dir" is not freed or pointed-to in "readdir".
shadow-4.8.1/libmisc/user_busy.c:278: leaked_storage: Variable "task_dir" going out of scope leaks the storage it points to.
276| _("%s: user %s is currently used by process %d\n"),
277| Prog, name, pid);
278|-> return 1;
279| }
280| }
Error: RESOURCE_LEAK (CWE-772): [#def20]
shadow-4.8.1/src/newgrp.c:162: alloc_fn: Storage is returned from allocation function "xgetspnam".
shadow-4.8.1/src/newgrp.c:162: var_assign: Assigning: "spwd" = storage returned from "xgetspnam(pwd->pw_name)".
shadow-4.8.1/src/newgrp.c:234: leaked_storage: Variable "spwd" going out of scope leaks the storage it points to.
232| }
233|
234|-> return;
235|
236| failure:
Error: RESOURCE_LEAK (CWE-772): [#def21]
shadow-4.8.1/src/passwd.c:530: alloc_fn: Storage is returned from allocation function "xstrdup".
shadow-4.8.1/src/passwd.c:530: var_assign: Assigning: "cp" = storage returned from "xstrdup(crypt_passwd)".
shadow-4.8.1/src/passwd.c:551: noescape: Resource "cp" is not freed or pointed-to in "strlen".
shadow-4.8.1/src/passwd.c:554: noescape: Resource "cp" is not freed or pointed-to in "strcat". [Note: The source code implementation of the function has been overridden by a builtin model.]
shadow-4.8.1/src/passwd.c:555: overwrite_var: Overwriting "cp" in "cp = newpw" leaks the storage that "cp" points to.
553| strcpy (newpw, "!");
554| strcat (newpw, cp);
555|-> cp = newpw;
556| }
557| return cp;
Using the random() function to obtain pseudo-random bytes
for generating salt strings is considered to be dangerous.
See CWE-327.
We really should use a more reliable source for obtaining
pseudo-random bytes like /dev/urandom.
Fixes#376.
Signed-off-by: Björn Esser <besser82@fedoraproject.org>
In the previous commit we refactored the functions converting the
rounds number into a string for use with the crypt() function, to
not require any static buffer anymore.
Add some clarifying comments about how the minimum required buffer
length is computed inside of these functions.
Signed-off-by: Björn Esser <besser82@fedoraproject.org>
* Move all pre-processor defines to the top of the file.
* Unify the gensalt() function to be useable for all supported
hash methods.
* Drop the gensalt_{b,yes}crypt() functions in favor of the
previous change.
* Refactor the functions converting the rounds number into
a string for use with the crypt() function, to not require
any static buffer anymore.
* Clarify the comment about how crypt_make_salt() chooses the used
hash method from the settings in the login.defs file.
* Use memset() to fill static buffers with zero before using them.
* Use a fixed amount of 16 random base64-chars for the
sha{256,512}crypt hash methods, which is effectively still less
than the recommendation from NIST (>= 128 bits), but the maximum
those methods can effectively use (approx. 90 bits).
* Rename ROUNDS_{MIN,MAX} to SHA_ROUNDS_{MIN,MAX}.
* Bugfixes in the logic of setting rounds in BCRYPT_salt_rounds().
* Likewise for YESCRYPT_salt_cost().
* Fix formatting and white-space errors.
Signed-off-by: Björn Esser <besser82@fedoraproject.org>
The corresponding functions for the other hash methods all take
a pointer to an integer value as the only paramater, so this
particular function should do so as well.
Signed-off-by: Björn Esser <besser82@fedoraproject.org>
Closes#325
Add a new subid_init() function which can be used to specify the
stream on which error messages should be printed. (If you want to
get fancy you can redirect that to memory :) If subid_init() is
not called, use stderr. If NULL is passed, then /dev/null will
be used.
This patch also fixes up the 'Prog', which previously had to be
defined by any program linking against libsubid. Now, by default
in libsubid it will show (subid). Once subid_init() is called,
it will use the first variable passed to subid_init().
Signed-off-by: Serge Hallyn <serge@hallyn.com>
When uid 0 maps host uid 0 into the child userns newer kernels require
CAP_SETFCAP be retained as this allows the caller to create fscaps that
are valid in the ancestor userns. This was a security issue (in very
rare circumstances). So whenever host uid 0 is mapped, retain
CAP_SETFCAP if the caller had it.
Userspace won't need to set CAP_SETFCAP on newuidmap as this is really
only a scenario that real root should be doing which always has
CAP_SETFCAP. And if they don't then they are in a locked-down userns.
(LXC sometimes maps host uid 0 during chown operations in a helper
userns but will not rely on newuidmap for that. But we don't want to
risk regressing callers that want to rely on this behavior.)
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Search the SELinux selabel database for the file type to be created.
Not specifying the file mode can cause an incorrect file context to be
returned.
Also prepare contexts in commonio_close() for the generic database
filename, not with the backup suffix appended, to ensure the desired
file context after the final rename.
Closes: #322
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
Closes#154
When starting any operation to do with subuid delegation, check
nsswitch for a module to use. If none is specified, then use
the traditional /etc/subuid and /etc/subgid files.
Currently only one module is supported, and there is no fallback
to the files on errors. Several possibilities could be considered:
1. in case of connection error, fall back to files
2. in case of unknown user, also fall back to files
etc...
When non-files nss module is used, functions to edit the range
are not supported. It may make sense to support it, but it also
may make sense to require another tool to be used.
libsubordinateio also uses the nss_ helpers. This is how for instance
lxc could easily be converted to supporting nsswitch.
Add a set of test cases, including a dummy libsubid_zzz module. This
hardcodes values such that:
'ubuntu' gets 200000 - 300000
'user1' gets 100000 - 165536
'error' emulates an nss module error
'unknown' emulates a user unknown to the nss module
'conn' emulates a connection error ot the nss module
Changes to libsubid:
Change the list_owner_ranges api: return a count instead of making the array
null terminated.
This is a breaking change, so bump the libsubid abi major number.
Rename free_subuid_range and free_subgid_range to ungrant_subuid_range,
because otherwise it's confusing with free_subid_ranges which frees
memory.
Run libsubid tests in jenkins
Switch argument order in find_subid_owners
Move the db locking into subordinateio.c
Signed-off-by: Serge Hallyn <serge@hallyn.com>
Issue #297 reported seeing
*** Warning: Linking the shared library libsubid.la against the
*** static library ../libmisc/libmisc.a is not portable!
which commit b5fb1b38ee was supposed
to fix. But a few commits later it's back. So try to fix it
in the way the bug reporter suggested. This broke builds some
other ways, namely a few missing library specifications, so add
those.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
Do not include <sys/prctl.h> we don't have <sys/capability.h>, we don't
need prctl in that case anyway.
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
In case there is a regular user with a process running on a system
with uid falling into a namespaced uid range of another user.
The user with the colliding namespaced uid range will not be
allowed to be deleted without forcing the action with -f.
The user_busy() is adjusted to check whether the suspected process
is really a namespaced process in a different namespace.
With this, it is possible for Linux distributors to store their
supplied default configuration files somewhere below /usr, while
/etc only contains the changes made by the user. The new option
--enable-vendordir defines where the shadow suite should additional
look for login.defs if this file is not in /etc.
libeconf is a key/value configuration file reading library, which
handles the split of configuration files in different locations
and merges them transparently for the application.
new switch added to useradd command, --btrfs-subvolume-home. When
specified *and* the filesystem is detected as btrfs, it will create a
subvolume for user's home instead of a plain directory. This is done via
`btrfs subvolume` command. Specifying the new switch while trying to
create home on non-btrfs will result in an error.
userdel -r will handle and remove this subvolume transparently via
`btrfs subvolume` command. Previosuly this failed as you can't rmdir a
subvolume.
usermod, when moving user's home across devices, will detect if the home
is a subvolume and issue an error messages instead of copying it. Moving
user's home (as subvolume) on same btrfs works transparently.
From <https://github.com/shadow-maint/shadow/pull/71>:
```
The third field in the /etc/shadow file (sp_lstchg) contains the date of
the last password change expressed as the number of days since Jan 1, 1970.
As this is a relative time, creating a user today will result in:
username:17238:0:99999:7:::
whilst creating the same user tomorrow will result in:
username:17239:0:99999:7:::
This has an impact for the Reproducible Builds[0] project where we aim to
be independent of as many elements the build environment as possible,
including the current date.
This patch changes the behaviour to use the SOURCE_DATE_EPOCH[1]
environment variable (instead of Jan 1, 1970) if valid.
```
This updated PR adds some missing calls to gettime (). This was originally
filed by Johannes Schauer in Debian as #917773 [2].
[0] https://reproducible-builds.org/
[1] https://reproducible-builds.org/specs/source-date-epoch/
[2] https://bugs.debian.org/917773
simplify the condition for setting the euid of the process. Now it is
always set when we are running as root, the issue was introduced with
the commit 52c081b02c
Changelog: 2018-11-24 - seh - enforce that euid only gets set to ruid if
it currently == 0 (i.e. really was setuid-*root*).
Closes: https://github.com/genuinetools/img/issues/191
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Serge Hallyn <shallyn@cisco.com>
Commit 1ecca8439d ("new[ug]idmap: not require CAP_SYS_ADMIN in the parent userNS")
does contain a wrong commit message, is lacking an explanation of the
issue, misses some simplifications and hardening features. This commit
tries to rectify this.
In (crazy) environment where all capabilities are dropped from the
capability bounding set apart from CAP_SET{G,U}ID setuid- and
fscaps-based new{g,u}idmap binaries behave differently when writing
complex mappings for an unprivileged user:
1. newuidmap is setuid
unshare -U sleep infinity &
newuidmap $? 0 100000 65536
First file_ns_capable(file, ns, CAP_SYS_ADMIN) is hit. This calls into
cap_capable() and hits the loop
for (;;) {
/* Do we have the necessary capabilities? */
if (ns == cred->user_ns)
return cap_raised(cred->cap_effective, cap) ? 0 : -EPERM;
/*
* If we're already at a lower level than we're looking for,
* we're done searching.
*/
if (ns->level <= cred->user_ns->level)
return -EPERM;
/*
* The owner of the user namespace in the parent of the
* user namespace has all caps.
*/
if ((ns->parent == cred->user_ns) && uid_eq(ns->owner, cred->euid))
return 0;
/*
* If you have a capability in a parent user ns, then you have
* it over all children user namespaces as well.
*/
ns = ns->parent;
}
The first check fails and falls through to the end of the loop and
retrieves the parent user namespace and checks whether CAP_SYS_ADMIN is
available there which isn't.
2. newuidmap has CAP_SETUID as fscaps set
unshare -U sleep infinity &
newuidmap $? 0 100000 65536
The first file_ns_capable() check for CAP_SYS_ADMIN is passed since the
euid has not been changed:
if ((ns->parent == cred->user_ns) && uid_eq(ns->owner, cred->euid))
return 0;
Now new_idmap_permitted() is hit which calls ns_capable(ns->parent,
CAP_SET{G,U}ID). This check passes since CAP_SET{G,U}ID is available in
the parent user namespace.
Now file_ns_capable(file, ns->parent, CAP_SETUID) is hit and the
cap_capable() loop (see above) is entered again. This passes
if (ns == cred->user_ns)
return cap_raised(cred->cap_effective, cap) ? 0 : -EPERM;
since CAP_SET{G,U}ID is available in the parent user namespace. Now the
mapping can be written.
There is no need for this descrepancy between setuid and fscaps based
new{g,u}idmap binaries. The solution is to do a
seteuid() back to the unprivileged uid and PR_SET_KEEPCAPS to keep
CAP_SET{G,U}ID. The seteuid() will cause the
file_ns_capable(file, ns, CAP_SYS_ADMIN) check to pass and the
PR_SET_KEEPCAPS for CAP_SET{G,U}ID will cause the CAP_SET{G,U}ID to
pass.
Fixes: 1ecca8439d ("new[ug]idmap: not require CAP_SYS_ADMIN in the parent userNS")
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
if the euid!=owner of the userns, the kernel returns EPERM when trying
to write the uidmap and there is no CAP_SYS_ADMIN in the parent
namespace.
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
This allows shadow-utils to build on systems like Adélie, which have no
<utmp.h> header or `struct utmp`. We use a <utmpx.h>-based daemon,
utmps[1], which uses `struct utmpx` only.
Tested both `login` and `logoutd` with utmps and both work correctly.
[1]: http://skarnet.org/software/utmps/
