Adding function check_fds to new file fd.c. The function check_fds
should be called in every setuid/setgid program.
Co-developed-by: Alejandro Colomar <alx@kernel.org>
The variable declarations for the buffers have been aligned in this
commit, so that they appear in the diff, making it easier to review.
Some important but somewhat tangent changes included in this commit:
- lib/nss.c: The size was being defined as 65, but then used as 64.
That was a bug, although not an important one; we were just wasting
one byte. Fix that while we replace snprintf() by SNPRINTF(), which
will get the size from sizeof(), and thus will use the real size.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Since newgrp is setuid-root, any write() system calls it does in order
to print error messages will be done as the root user.
Unprivileged users can get newgrp to print essentially arbitrary strings
to any open file in this way by passing those strings as argv[0] when
calling execve(). For example:
$ setpid() { (exec -a $1$'\n:' newgrp '' 2>/proc/sys/kernel/ns_last_pid & wait) >/dev/null; }
$ setpid 31000
$ readlink /proc/self
31001
This is not a vulnerability in newgrp; it is a bug in the Linux kernel.
However, this type of bug is not new [1] and it makes sense to try to
mitigate these types of bugs in userspace where possible.
[1]: https://lwn.net/Articles/476947/
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
If we consider simple objects as arrays of size 1, we can considerably
simplify these APIs, merging the *ARRAY and the non-array variants.
That will produce more readable code, since lines will be shorter (by
not having ARRAY in the macro names, as all macros will consistently
handle arrays), and the allocated size will be also more explicit.
The syntax will now be of the form:
p = MALLOC(42, foo_t); // allocate 42 elements of type foo_t.
p = MALLOC(1, bar_t); // allocate 1 element of type foo_t.
The _array() allocation functions should _never_ be called directly, and
instead these macros should be used.
The non-array functions (e.g., malloc(3)) still have their place, but
are limited to allocating structures with flexible array members. For
any other uses, the macros should be used.
Thus, we don't use any array or ARRAY variants in any code any more, and
they are only used as implementation details of these macros.
Link: <https://software.codidact.com/posts/285898/288023#answer-288023>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The tools newgrp and useradd expect waitpid to behave as described in
its manual page. But the notes indicate that if SIGCHLD is ignored,
waitpid behaves differently.
A user could set SIGCHLD to ignore before starting newgrp through exec.
Children of newgrp would not become zombies and their PIDs could be
reassigned before newgrp could call kill with the child pid and SIGCONT.
The useradd tool is not installed setuid, but I have added the default
there as well (copied from vipw).
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Use of these macros, apart from the benefits mentioned in the commit
that adds the macros, has some other good side effects:
- Consistency in getting the size of the object from sizeof(type),
instead of a mix of sizeof(type) sometimes and sizeof(*p) other
times.
- More readable code: no casts, and no sizeof(), so also shorter lines
that we don't need to cut.
- Consistency in using array allocation calls for allocations of arrays
of objects, even when the object size is 1.
Cc: Valentin V. Bartenev <vbartenev@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
- Every non-const pointer converts automatically to void *.
- Every pointer converts automatically to void *.
- void * converts to any other pointer.
- const void * converts to any other const pointer.
- Integer variables convert to each other.
I changed the declaration of a few variables in order to allow removing
a cast.
However, I didn't attempt to edit casts inside comparisons, since they
are very delicate. I also kept casts in variadic functions, since they
are necessary, and in allocation functions, because I have other plans
for them.
I also changed a few casts to int that are better as ptrdiff_t.
This change has triggered some warnings about const correctness issues,
which have also been fixed in this patch (see for example src/login.c).
Signed-off-by: Alejandro Colomar <alx@kernel.org>
In variadic functions we still do the cast. In POSIX, it's not
necessary, since NULL is required to be of type 'void *', and 'void *'
is guaranteed to have the same alignment and representation as 'char *'.
However, since ISO C still doesn't mandate that, and moreover they're
doing dubious stuff by adding nullptr, let's be on the cautious side.
Also, C++ requires that NULL is _not_ 'void *', but either plain 0 or
some magic stuff.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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;
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>
In case a system uses remote identity server (LDAP) the group lookup
can be very slow. We avoid it when we already know the user has the
group membership.
* Changelog: Update documentation of 2013-07-28 mancha entry.
* lib/prototypes.h, lib/encrypt.c: Update splint marker,
pw_encrypt can return NULL.
* lib/encrypt.c: Fix outdated statement on GNU crypt.
* src/chgpasswd.c: Improve diagnostic to user when pw_encrypt
fails and use fail_exit() instead of exit().
* src/chpasswd.c: Likewise.
* src/newusers.c: Likewise.
* src/passwd.c: Likewise when new password is encrypted.
* src/newgrp.c: Improve diagnostic to user and syslog when
pw_encrypt fails. Do not apply 1s penalty as this is not an
invalid password issue.
* src/passwd.c: Likewise when password is checked.
a salt that violates specs. On Linux, crypt() also fails with
DES/MD5 salts in FIPS140 mode. Rather than exit() on NULL returns
we send them back to the caller for appropriate handling.
* src/login_nopam.c: Limit the scope of variables end, lineno, i,
str_len.
* src/logoutd.c: Limit the scope of variable c.
* src/vipw.c: Re-indent.
* src/vipw.c: Close the file after the creation of the backup.
* src/useradd.c (set_default): Close input file on failure.
* src/useradd.c: Limit the scope of variables spool, file, fd, gr,
gid, mode.
* src/passwd.c: Limit the scope of variables last and ok.
* src/chage.c: Fix typo (non breaking space).
* src/login.c: Limit the scope of variables erasechar killchar, c,
failed.
* src/groups.c: Limit the scope of variable ngroups, pri_grp, i.
* src/id.c: Limit the scope of variable i.
* src/newgrp.c: audit_buf is only used in newgrp. Make it static.
* src/newgrp.c: Ignore the return value of fputs().
* src/newgrp.c: Use exit(EXIT_FAILURE) instead of exit(1).
src/newusers.c, src/pwconv.c, src/chpasswd.c, src/logoutd.c,
src/chfn.c, src/groupmems.c, src/usermod.c, src/pwunconv.c,
src/expiry.c, src/groupdel.c, src/chgpasswd.c, src/useradd.c,
src/su.c, src/groupmod.c, src/passwd.c, src/pwck.c,
src/groupadd.c, src/chage.c, src/login.c, src/grpconv.c,
src/groups.c, src/grpunconv.c, src/chsh.c: Prog is now global (not
static to the file) so that it can be used by the helper functions
of libmisc.
* lib/prototypes.h: Added extern char *Prog.
* libmisc/find_new_gid.c, libmisc/find_new_uid.c: Indicate the
program name with the warning.
src/newgrp.c, src/passwd.c, src/su.c: Use the same stderr and
syslog warnings when the username cannot be determined.
* src/newgrp.c: Reuse the same stderr message for groups which do
not exist in the system.
src/groupmod.c, src/grpck.c, src/login.c, src/logoutd.c,
src/newgrp.c, src/newusers.c, src/passwd.c, src/pwck.c,
src/suauth.c, src/useradd.c, src/userdel.c, src/usermod.c,
src/vipw.c: Complete the switch from the `' quotation style to ''.
Do it also in SYSLOG messages. Quote some parameters. All this
permits to merge some messages.
unsigned long integers.
* src/newgrp.c: The ID argument of audit_logger is an unsigned
int.
* src/newgrp.c: Ignore the return value of signal() (the signal
handlers are assumed to be the default one and are restored
later).
* src/newgrp.c: Do not checl if a pid_t is < 0, check if equal
to (pid_t)-1.
SIGSTOP handling. Raise the signal which stopped the child instead
of always SIGSTOP.
Import Debian patch 406_vipw_resume_properly.
Thanks to Dean Gaudet.
* NEWS, src/vipw.c: Resume properly after ^Z.
