Except for the added (and sorted) includes, the removal of redundant
parentheses, a few cases that have been refactored for readability, and
a couple of non-string cases that I've left out of the change, this
patch can be approximated with the following semantic patch:
$ cat ~/tmp/spatch/streq.sp
@@
expression s;
@@
- '\0' == *s
+ streq(s, "")
@@
expression s;
@@
- '\0' == s[0]
+ streq(s, "")
@@
expression s;
@@
- *s == '\0'
+ streq(s, "")
@@
expression s;
@@
- s[0] == '\0'
+ streq(s, "")
$ find contrib/ lib* src/ -type f \
| xargs spatch --in-place --sp-file ~/tmp/spatch/streq.sp;
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We were reusing a leftover from parsing a previous line if
(i == NFIELDS-1). A few lines below this check, we use read the element
in [3] (that is, [NFIELDS-1]), without having written it in this call.
Be stricter, and require that all NFIELDS fields are found.
Fixes: 45c6603cc8 (2007-10-07, "[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Closes: <https://github.com/shadow-maint/shadow/issues/1144>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Instead of reallocating 1 more meber per iteration, calculate the total
amount that we want by counting the number of commas (delimiters) in the
string, plus one for the last element, plus one for the terminating
NULL.
This might result in overallocation of one element if the string is an
empty string, or if there's a trailing comma; however, that's not an
issue. We can afford overallocating one element in certain cases, and
we get in exchange a much simpler function.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We've simplified the function so much in the previous commits, that now
$2 is rather useless. It only sets the output parameter to the same
value that the function returns. It's simpler if the caller just sets
it itself after the call.
This removes the only 3-star pointer in the entire project. :)
Signed-off-by: Alejandro Colomar <alx@kernel.org>
0 is a horrible null-pointer constant. Don't use it.
Especially, when just a few lines above, in the same function,
we've used NULL for the same thing.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Use instead automatic variables as much as possible.
This reduces the number of dereferences, enhancing readability.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This makes build_list() less dependent on the context.
It starts from clean, whatever the state before the call was.
I was having a hard time understanding the reallocation,
until I saw that we were zeroing everything right before the call.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
If n was 0, it doesn't hurt to set it again to 0;
and the list would be NULL, so it doesn't hurt free(3)ing it
and setting to NULL again either.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It was hard to understand what each variable is. Use a consistent
scheme, where a 'p' means a pointer, 'l' means list, and 'n' means
number of elements. Those should be obvious from the name of the
function and the context, and will make it easier to read the code.
Also, the shorter names will allow focusing on the rest of the code.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
list ($2) is a pointer to a list of strings. We were declaring it as an
array of pointers to strings, which was bogus. It worked out of luck,
because array parameters are transformed into pointers by the compiler,
but it was incorrect. Just look at how we're calling this function.
$ grep build_list lib/gshadow.c
build_list(char *s, char ***list, size_t *nlist)
sgroup.sg_adm = build_list (fields[2], &admins, &nadmins);
sgroup.sg_mem = build_list (fields[3], &members, &nmembers);
$ grep '^static .*\<admins\>' lib/gshadow.c
static /*@null@*//*@only@*/char **admins = NULL;
$ grep '^static .*\<members\>' lib/gshadow.c
static /*@null@*//*@only@*/char **members = NULL;
Fixes: 8e167d28af ("[svn-upgrade] Integrating new upstream version, shadow (4.0.8)")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
list cannot be NULL in the first iteration, so we don't need a do-while.
Just in case it's not obvious: we know it's not NULL in the first
iteration because right above, in line 772, we've already dereferenced
it.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
strsep(3) is stateless, and so is easier to reason about.
It also has a slight difference: strtok(3) jumps over empty fields,
while strsep(3) respects them as empty fields. In most of the cases
where we were using strtok(3), it makes more sense to respect empty
fields, and this commit probably silently fixes a few bugs.
In other cases (most notably filesystem paths), contiguous delimiters
("//") should be collapsed, so strtok(3) still makes more sense there.
This commit doesn't replace such strtok(3) calls.
While at this, remove some useless variables used by these calls, and
reduce the scope of others.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
When running groupadd or groupmod with the -U|--user option, also update
the group shadow database if it is used.
Fixes: 342c934a (2020-08-09, "add -U option to groupadd and groupmod")
Closes: <https://github.com/shadow-maint/shadow/issues/1124>
Except for the added (and sorted) includes, and the removal of redundant
parentheses, and one special case, this patch can be approximated with
the following semantic patch:
$ cat ~/tmp/spatch/strneq.sp;
@@
expression a, b;
@@
- strcmp(a, b) != 0
+ !streq(a, b)
@@
expression a, b;
@@
- 0 != strcmp(a, b)
+ !streq(a, b)
$ find contrib/ lib* src/ -type f \
| xargs spatch --sp-file ~/tmp/spatch/strneq.sp --in-place;
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Except for the added (and sorted) includes, and the removal of redundant
parentheses, this patch can be approximated with the following semantic
patch:
$ cat ~/tmp/spatch/streq.sp;
@@
expression a, b;
@@
- strcmp(a, b) == 0
+ streq(a, b)
@@
expression a, b;
@@
- 0 == strcmp(a, b)
+ streq(a, b)
@@
expression a, b;
@@
- !strcmp(a, b)
+ streq(a, b)
$ find contrib/ lib* src/ -type f \
| xargs spatch --sp-file ~/tmp/spatch/streq.sp --in-place;
$ git restore lib/string/strcmp/streq.h;
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We don't need the heavy stdio for getting a few bytes from
</dev/urandom>. Let's use the simpler POSIX API.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The instructions are written so that this script should be run from the
root of the repository. Specify the path from the root of the repo.
Before this fix, the command needed to be run from within <share/>.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
If a job in a matrix fails we don't want to cancel all jobs, thus we
need to set `fail-fast: false` as a strategy property.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
The get_map_ranges function shall support the whole accepted range
as specified in user_namespaces(7), i.e. upper and lower from 0 to
UINT_MAX - 1 as well as range from 1 to UINT_MAX. The actual limit of
range depends on values of upper and lower and adding the range
to either upper or lower shall never overflow UINT_MAX.
Fixes: 7c43eb2c4e (2024-07-11, "lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call")
Fixes: ff2baed5db (2016-08-14, "idmapping: add more checks for overflow")
Fixes: 94da3dc5c8 (2016-08-14, "also check upper for wrap")
Fixes: 7f5a14817d (2016-07-31, "get_map_ranges: check for overflow")
Co-authored-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We already have sgetspent(), with identical semantics, defined in
<lib/sgetspent.c>.
$ diff -u <(grepc sgetspent .) <(grepc my_sgetspent .)
--- /dev/fd/63 2024-11-11 11:56:55.444055921 +0100
+++ /dev/fd/62 2024-11-11 11:56:55.444055921 +0100
@@ -1,23 +1,19 @@
-./lib/sgetspent.c:struct spwd *
-sgetspent(const char *string)
+./lib/shadow.c:static struct spwd *my_sgetspent (const char *string)
{
- static char spwbuf[PASSWD_ENTRY_MAX_LENGTH];
- static struct spwd spwd;
- char *fields[FIELDS];
- char *cp;
- int i;
+ int i;
+ char *fields[FIELDS];
+ char *cp;
+ static char spwbuf[BUFSIZ];
+ static char empty[] = "";
+ static struct spwd spwd;
/*
* Copy string to local buffer. It has to be tokenized and we
* have to do that to our private copy.
*/
- if (strlen (string) >= sizeof spwbuf) {
- fprintf (shadow_logfd,
- "%s: Too long passwd entry encountered, file corruption?\n",
- shadow_progname);
- return NULL; /* fail if too long */
- }
+ if (strlen (string) >= sizeof spwbuf)
+ return 0;
strcpy (spwbuf, string);
stpsep(spwbuf, "\n");
@@ -30,14 +26,16 @@
fields[i] = strsep(&cp, ":");
if (i == (FIELDS - 1))
- fields[i++] = "";
+ fields[i++] = empty;
if (cp != NULL || (i != FIELDS && i != OFIELDS))
- return NULL;
+ return 0;
/*
* Start populating the structure. The fields are all in
- * static storage, as is the structure we pass back.
+ * static storage, as is the structure we pass back. If we
+ * ever see a name with '+' as the first character, we try
+ * to turn on NIS processing.
*/
spwd.sp_namp = fields[0];
@@ -46,13 +44,13 @@
/*
* Get the last changed date. For all of the integer fields,
* we check for proper format. It is an error to have an
- * incorrectly formatted number.
+ * incorrectly formatted number, unless we are using NIS.
*/
if (fields[2][0] == '\0')
spwd.sp_lstchg = -1;
else if (a2sl(&spwd.sp_lstchg, fields[2], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* Get the minimum period between password changes.
@@ -61,7 +59,7 @@
if (fields[3][0] == '\0')
spwd.sp_min = -1;
else if (a2sl(&spwd.sp_min, fields[3], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* Get the maximum number of days a password is valid.
@@ -70,7 +68,7 @@
if (fields[4][0] == '\0')
spwd.sp_max = -1;
else if (a2sl(&spwd.sp_max, fields[4], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* If there are only OFIELDS fields (this is a SVR3.2 /etc/shadow
@@ -93,7 +91,7 @@
if (fields[5][0] == '\0')
spwd.sp_warn = -1;
else if (a2sl(&spwd.sp_warn, fields[5], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* Get the number of days of inactivity before an account is
@@ -103,7 +101,7 @@
if (fields[6][0] == '\0')
spwd.sp_inact = -1;
else if (a2sl(&spwd.sp_inact, fields[6], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* Get the number of days after the epoch before the account is
@@ -113,7 +111,7 @@
if (fields[7][0] == '\0')
spwd.sp_expire = -1;
else if (a2sl(&spwd.sp_expire, fields[7], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* This field is reserved for future use. But it isn't supposed
@@ -123,8 +121,7 @@
if (fields[8][0] == '\0')
spwd.sp_flag = SHADOW_SP_FLAG_UNSET;
else if (str2ul(&spwd.sp_flag, fields[8]) == -1)
- return NULL;
+ return 0;
return (&spwd);
}
-./lib/prototypes.h:extern struct spwd *sgetspent (const char *string);
Closes: <https://github.com/shadow-maint/shadow/issues/1114>
Link: <https://www.youtube.com/watch?v=IpbvtSQvgWM>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The reason for that code seems to be some ancient AIX version that
defined a value that was too small (32). We don't support such systems.
In the link below, I found the following comment and code:
/*
* Some AIX versions advertise a too small MAXHOSTNAMELEN value (32).
* Result: long hostnames would be truncated, and connections would be
* dropped because of host name verification failures. Adrian van Bloois
* (A.vanBloois@info.nic.surfnet.nl) figured out what was the problem.
*/
#if (MAXHOSTNAMELEN < 64)
#undef MAXHOSTNAMELEN
#endif
/* In case not defined in <sys/param.h>. */
#ifndef MAXHOSTNAMELEN
#define MAXHOSTNAMELEN 256 /* storage for host name */
#endif
Today's systems seem to be much better regarding this macro. Rely on
them.
Link: <https://sources.debian.org/src/tcp-wrappers/7.6.q-33/workarounds.c/?hl=36#L36>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This fix addresses an issue in is_valid_user_list() where the free
operation was attempted on an address not allocated with malloc(). By
duplicating the pointer with xstrdup(users) into dup, and using dup as
the original pointer, we ensure that only the valid pointer is freed,
avoiding an invalid free operation.
This bug was introduced when changing some code that used strchrnul(3)
to use strsep(3) instead. strsep(3) advances the pointer, unlike the
previous code.
This unconditionally leads to a bug:
- Passing NULL to free(3), if the last field in the
colon-separated-value list is non-empty. This results in a memory
leak.
- Passing a pointer to the null byte ('\0') that terminates the string,
if the last element of the colon-separated-value list is empty. The
most obvious reproducer of such a bogus free(3) call is:
free(strdup("foo:") + 4);
This results in Undefined Behavior, and could result in allocator
data corruption.
Fixes: 16cb664865 (2024-07-01, "lib/, src/: Use strsep(3) instead of its pattern")
Suggested-by: <https://github.com/frostb1ten>
Reported-by: <https://github.com/frostb1ten>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Christian Brauner <christian@brauner.io>
The new fedora 41 has been released and some things have changed. Make
sure to install python and python3-dnf and specify the dnf version in
the roles.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Closes#1088
We can't be sure whether a github runner will have new- or old-
style sources.list, so check whether the new exists, else use
the old style.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
During coverity scan, there are reported four issues
with unbounded source buffer for each usage of input arg
directly with syslog function.
Sample coverity test report for chsh.c file:
1. string_size_argv: argv contains strings with unknown size.
int main (int argc, char **argv)
[...]
4. var_assign_var: Assigning: user = argv[optind]. Both are now tainted.
user = argv[optind];
[...]
CID 5771784: (#1 of 1): Unbounded source buffer (STRING_SIZE)
15. string_size: Passing string user of unknown size to syslog.
SYSLOG ((LOG_INFO, "changed user '%s' shell to '%s'", user, loginsh));
Similar issue is reported three times more:
File: chfn.c, function: main, variable: user
File: passwd.c, function: main, variable: name
File: newgrp.c, function: main, variable: group
This commit is the first approach to fix the reported issues.
The proposed changes add conditions, which verify
the user and group names arguments, including their lengths.
This will not silence the coverity reports, but the change causes
that they are irrelevant and could be ignored.
This is in preparation for the following commit, which will need this
shorter parameter name to avoid breaking long lines.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Run this workflow instead of replicating the script every time we need
to install the dependencies.
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Recently Ubuntu updated its repositories configuration file from
`/etc/apt/sources.list` to `/etc/apt/sources.list.d/ubuntu.source`.
Thus, we need to update its location to be able to install all the
package dependencies.
In addition, the CI script was trying to uncomment the lines starting
with `deb-src`, but there is none in the new configuration file format.
Replace `Types: deb` by `Types: deb deb-src` at the beginning of the
line instead.
This commit merges all dependency installation scripts into a single
workflow, which will be called from all sites that have to install
dependencies.
Link: https://linuxconfig.org/ubuntus-repository-configuration-ubuntu-sources-have-moved-to-etc-apt-sources-list-d-ubuntu-sources
Closes: https://github.com/shadow-maint/shadow/issues/1088
Reported-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
I forgot to remove the setting of errno when I switched from
strtoul_noneg() to str2ul(). strtoul(3) needs errno for determining
success, but str2ul() does not.
Fixes: f3a1e1cf09 ("src/check_subid_range.c: Call str2ul() instead of strtoul_noneg()")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The groupadd utility does not set information in subgid. Instead, list
all programs which actually can do so.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
The mode of the file should be 644, but mkstemp(2) was transforming it
to 600.
To do this, we need a function that accepts a mode parameter. While we
don't need a flags parameter, to avoid confusion with mkostemp(2), let's
add both a flags and a mode parameter.
Link: <https://github.com/shadow-maint/shadow/pull/1080>
Reported-by: kugarocks <kugacola@gmail.com>
Suggested-by: kugarocks <kugacola@gmail.com>
Tested-by: kugarocks <kugacola@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The tz function is only called if ENV_TZ starts with a slash.
If the specified file cannot be read, the code implies that ENV_TZ
would be returned if it does not start with a slash.
Since we know that it DOES start with a slash, the code can be
simplified to state that "TZ=CST6CDT" is returned as a default if
the specified file cannot be read.
Benefit of this change is that strcpy's use case here can be
easier verified.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
The run_part function is only used in run_part.c itself, so no
need to expose it to other files.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Use shadow_logfd for logging instead of fixed stderr to use
shadow's own logging infrastructure.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Since:
- utmpx APIs are used in non-Linux code blocks
- <utmpx.h> is already unconditionally included in Linux parts in other
files
then unconditionally include it in this file as well.
Signed-off-by: Pino Toscano <toscano.pino@tiscali.it>
If localtime_r(3) fails, just print future, as we do in day_to_str().
It should only fail for unrealistic dates, if at all.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
%F is specified by ISO C99. It adds semantic meaning as printing an
ISO 8601 date.
Scripted change:
$ cat ~/tmp/spatch/strftime_F.sp
@@
@@
- "%Y-%m-%d"
+ "%F"
$ find contrib/ lib* src/ -type f \
| xargs spatch --sp-file ~/tmp/spatch/strftime_F.sp --in-place
Signed-off-by: Alejandro Colomar <alx@kernel.org>
cppw, cpgr were Debian-only tools, which I've dropped in a recent upload.
Upstream should have never had tests for them.
Signed-off-by: Chris Hofstaedtler <zeha@debian.org>
Distribution to run can be selected when running `ansible-playbook` by
appending `-e 'distribution=fedora'` to the command.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Using a dockerfile to build, install and test the code can be
problematic as we can't capture the log files to check what failed in
case of failure. This PR converts the fedora dockerfile to Ansible, an
open source IT automation tool. The tool can be used on the developers
and the CI system to check whether a piece of code can be built,
installed and tested.
This is the first patch in a series, where I will convert the existing
PR workflows to use Ansible instead of dockerfiles.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Align on variable name BUILD_BASE_DIR for overriding the toplevel
directory. This is the same name as accepted by tests/common/config.sh.
Without this, the test libsubid/04_nss fails in Debian's autopkgtests.
Signed-off-by: Chris Hofstaedtler <zeha@debian.org>
The extra paragraph for --users mentions a -N option. groupmod has no -N
option.
Prevent confusion and remove its appearance.
Signed-off-by: Sebastian Gross <sgross@emlix.com>
The --users list option expect a string of comma separated values.
While this might be obvious to some others it is certainly not for others.
Remove this ambiguity.
Closes#848
Signed-off-by: Sebastian Gross <sgross@emlix.com>
--append has no argument in groupmod.c but the man pages states GID as
parameter.
In order to avoid confusion remove it from man page.
Signed-off-by: Sebastian Gross <sgross@emlix.com>
For a pointer iterator used often, a single-letter identifier is more
appropriate. That reduces the length of lines considerably, avoiding
unnecessary line breaks. And since we initialize it with
m = mappings;
it's clear what it is.
Link: <ff2baed5db (r136635300)>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The tty names field and the user names field have the same formatting:
a CSV terminated by a ':'. Thus, we can --and should-- use the same
exact code for parsing both.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Otherwise, the line is invalidly formatted, and we ignore it.
Detailed explanation:
There are two conditions on which we break out of the loops that precede
these added checks:
- j is too big (we've exhausted the space in the static arrays)
$ grep -r -e PORT_TTY -e PORT_IDS lib/port.*
lib/port.c: static char *ttys[PORT_TTY + 1]; /* some pointers to tty names */
lib/port.c: static char *users[PORT_IDS + 1]; /* some pointers to user ids */
lib/port.c: for (cp = buf, j = 0; j < PORT_TTY; j++) {
lib/port.c: if ((',' == *cp) && (j < PORT_IDS)) {
lib/port.h: * PORT_IDS - Allowable number of IDs per entry.
lib/port.h: * PORT_TTY - Allowable number of TTYs per entry.
lib/port.h:#define PORT_IDS 64
lib/port.h:#define PORT_TTY 64
- strpbrk(3) found a ':', which signals the end of the comma-sepatated
list, and the start of the next colon-separated field.
If the first character in the remainder of the string is not a ':', it
means we've exhausted the array size, but the CSV list was longer, so
we'd be truncating it. Consider the entire line invalid, and skip it.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This label means we detected a bogus line, and want to skip it and jump
to the next one; rename it accordingly. 'again' seemed to say that it
was somehow looping on the same line.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This requires changing isspace(3) calls to an explicit accept string,
and I chose " \t\n" for it (as is done in other parts of this project),
which isn't exactly the same, but we probably don't want other
isspace(3) characters in those files, so it should work.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Handle negative values as errors from a2sl(), and reuse its
error-handling code.
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
spwd.sp_flag is an unsigned long, which can never be negative.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The limit, since it's an unsigned int, should have been UINT_MAX, not
INT_MAX. By calling a2ui() we can fix that and simplify too.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
STRNDUPA() is equivalent to automatic storage allocation (alloca(3))
+ ZUSTR2STP().
The benefits of this refactor are:
- The allocation size is always correct, and needs no comments, since
it's now automatically calculated by the macro.
- STRNDUPA() is probably more familiar, since
- strndupa(3) is a libc function,
- STRNDUPA() is the obvious wrapper that
calculates the size based on the input array.
- We can remove ZUSTR2STP().
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The program was happily ignoring ENOMEM errors.
Fixes: 7f9e196903 ("* NEWS, src/newusers.c, src/Makefile.am: Added support for")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We are setting `sgrent.sg_adm[1] = NULL;`, so we need 2 elements.
Fixes: 87b56b19fb ("* NEWS, src/groupmems.c, man/groupmems.8.xml: Added support for [...]")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Before this patch, the function looped while (s != NULL && *s != '\0').
However, nothing was modifying that string if REALLOC() failed, so the
loop was forever.
Fixes: 8e167d28af ("[svn-upgrade] Integrating new upstream version, shadow (4.0.8)")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This should have gone into the #else'd branch in 8451bed8b0, and
should have been removed in 3e602b58a2.
Fixes: 8451bed8b0 ("[svn-upgrade] Integrating new upstream version, shadow (4.0.13)")
Fixes: 3e602b58a2 ("Remove HAVE_STRFTIME ifdefs")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
See time(2):
BUGS
Error returns from this system call are indistinguishable from
successful reports that the time is a few seconds before the
Epoch, so the C library wrapper function never sets errno as a re‐
sult of this call.
The tloc argument is obsolescent and should always be NULL in new
code. When tloc is NULL, the call cannot fail.
Fixes: 45c6603cc8 ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The functions that set these strings --do_rlogin() and login_prompt()--
make sure to terminate them with a NUL.
Fixes: 3704745289 ("* lib/defines.h: Define USER_NAME_MAX_LENGTH, based on utmp and [...]")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Now that we use liba2i's const-generic macros, we can (and must) use a
'const char **' endp where the input string is 'const char *'.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Implement it as an inline function, and add restrict and ATTR_STRING()
and ATTR_ACCESS() as appropriate.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Implement it as an inline function, and add restrict and ATTR_STRING()
and ATTR_ACCESS() as appropriate.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These functions were open-coding get_gid(). Use the actual function.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Implement it as an inline function, and add restrict and ATTR_STRING()
and ATTR_ACCESS() as appropriate.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
atoi(3) easily triggers Undefined Behavior. Replace it by str2[u]l(),
which are safe from that, and add type safety too.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
In the case of is_unsigned() and is_signed(), the natural thing would be
to compare to 0:
#define is_unsigned(x) (((typeof(x)) -1) > 0)
#define is_signed(x) (((typeof(x)) -1) < 0)
However, that would trigger -Wtype-limits, so we compare against 1,
which silences that, and does the same job.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
When we run for instance
check_subid_range ubuntu u 100000 65536
when ubuntu user is defined and has that range, it returns no entries
because the subid db is not opened. Open it in have_range if needed.
I haven't figured out why this ever worked.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
volatile needs to be casted away behind a [[gnu::noipa]] function, to
make that invisible to the compiler. Otherwise, the compiler can see
that it is being discarded, and is free to abuse Undefined Behavior.
Closes: <https://github.com/shadow-maint/shadow/issues/1028>
Reported-by: Chris Hofstaedtler <zeha@debian.org>
Tested-by: Chris Hofstaedtler <zeha@debian.org>
Reviewed-by: Chris Hofstaedtler <zeha@debian.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
common/config.sh currently tries to find the top directory by looking
for .git. There are also many places under tests/ where we use
hard-coded ../../.. to find things like ${TOP_DIR}/lib.
We don't actually ship the tests with 'make dist'. So we will
be exporting tests/ as a separate tarball. In particular, I want
to then import this in the debian package. However, there it will
be under shadow.git/debian/tests, not shadow.git/tests.
To support this, accept the environment variable BUILD_BASE_DIR,
which should point to shadow.git.
An alternative would be to move the tests to their own git
tree. However, keeping tests in separate git tree tends to
lead to repos getting out of sync. And we'd still need to accept
something like BUILD_BASE_DIR.
Note there are a lot of tests under run-all, which I'm not converting
as they currently are not being run in CI, so I'm more likely to
break something.
Changelog:
2024 05 26: Incorporate feedback from alejandro-colomar
Link: <https://salsa.debian.org/debian/shadow/-/merge_requests/21>
Link: <https://salsa.debian.org/debian/shadow/-/merge_requests/22>
Cc: Chris Hofstaedtler <zeha@debian.org>
Signed-off-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Groupmod -U may cause crashes because of double free. If without -a, the first free of (*ogrp).gr_mem is in gr_free_members(&grp), and then in gr_update without -n or gr_remove with -n.
Considering the minimal impact of modifications on existing code, delete gr_free_members(&grp) to avoid double free.Although this may seem reckless, the second free in two different positions will definitely be triggered, and the following two test cases can be used to illustrate the situation :
[root@localhost src]# ./useradd u1
[root@localhost src]# ./useradd u2
[root@localhost src]# ./useradd u3
[root@localhost src]# ./groupadd -U u1,u2,u3 g1
[root@localhost src]# ./groupmod -n g2 -U u1,u2 g1
Segmentation fault
This case would free (*ogrp).gr_mem in gr_free_members(&grp) due to assignment statements grp = *ogrp, then in if (nflg && (gr_remove (group_name) == 0)), which finally calls gr_free_members(grent) to free (*ogrp).gr_mem again.
[root@localhost src]# ./useradd u1
[root@localhost src]# ./useradd u2
[root@localhost src]# ./useradd u3
[root@localhost src]# ./groupadd -U u1,u2,u3 g1
[root@localhost src]# ./groupmod -U u1,u2 g1
Segmentation fault
The other case would free (*ogrp).gr_mem in gr_free_members(&grp) too, then in if (gr_update (&grp) == 0), which finally calls gr_free_members(grent) too to free (*ogrp).gr_mem again.
So the first free is unnecessary, maybe we can drop it.
Fixes: 342c934a35 ("add -U option to groupadd and groupmod")
Closes: <https://github.com/shadow-maint/shadow/issues/1013>
Link: <https://github.com/shadow-maint/shadow/pull/1007>
Link: <https://github.com/shadow-maint/shadow/pull/271>
Link: <https://github.com/shadow-maint/shadow/issues/265>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: lixinyun <li.xinyun@h3c.com>
Per https://tdg.docbook.org/tdg/4.5/term, term is a word being
defined in a varlistentry. The 'high uid' description is not a
varlistentry, so <term> and </term> show up in the processed
manpage. See debian Bug#1072297.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
`PKG_CONFIG` variable needs to be set for `PKG_CHECK_MODULES` to
succeed, but this wasn't happening in Fedora because the first
appearance of `PKG_CHECK_MODULES` was conditionally skipped because this
distribution is compiled without `libbsd` support. Thus, moving the
cmocka library detection before libbsd fixes the problem.
Suggested-by: Lukas Slebodnik <lslebodn@fedoraproject.org>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
All call sites have been replaced by functions from "atoi/a2i.h" and
"atoi/str2i.h" recently.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It is a simpler call, with more type safety.
A consequence of this change is that the program now accepts numbers in
bases 8 and 16. That's not a problem here, I think.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
time_t isn't necessarily unsigned (in fact, it's likely to be signed.
Therefore, parse the number as the right type, via a2i(time_t, ...).
Still, reject negative numbers, just to be cautious. It was done
before (strtoull_noneg()), so it shouldn't be a problem. (However,
strtoull_noneg() was only introduced recently, and before that we called
strtoull(3), which silently accepted negative values.)
Remove the limitation of ULONG_MAX, which seems arbitrary. It probably
was written in times where 'time_t' had the same length of 'long', and
this was thus a test that the value didn't overflow 'time_t'. Such a
test is implicit in the a2i() call, so forget about it.
Unify the error messages into a single one that provides all the info
(except the value of 'fallback').
Link: <cb610d54b4 (r136407772)>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Chris Lamb <lamby@debian.org>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Instead of raw sysconf(_SC_LOGIN_NAME_MAX) calls, which was being used
without error handling.
Fixes: 3b7cc05387 ("lib: replace `USER_NAME_MAX_LENGTH` macro")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Keep the while loop in the outer function, and move the iteration code
to this new helper. This makes it a bit more readable.
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Keep the while loop in the outer function, and move the iteration code
to this new helper. This makes it a bit more readable.
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
After _every_ iteration, 'changed' is always 'false'. We don't need to
have it outside of the loop.
See:
$ grepc update_gshadow_file . \
| grep -e changed -e goto -e continue -e break -e free_ngrp -e '{' -e '}' \
| pcre2grep -v -M '{\n\t*}';
{
bool changed;
changed = false;
while ((sgrp = sgr_next ()) != NULL) {
if (!was_member && !was_admin && !is_member) {
continue;
}
if (was_admin && lflg) {
changed = true;
}
if (was_member) {
if ((!Gflg) || is_member) {
if (lflg) {
changed = true;
}
} else {
changed = true;
}
} else if (is_member) {
changed = true;
}
if (!changed)
goto free_nsgrp;
changed = false;
}
}
This was already true in the commit that introduced the code:
$ git show 45c6603cc:src/usermod.c \
| grepc update_gshadow \
| grep -e changed -e goto -e break -e continue -e '\<if\>' -e '{' -e '}' \
| pcre2grep -v -M '{\n\t*}';
{
int changed;
changed = 0;
while ((sgrp = sgr_next())) {
* See if the user was a member of this group
* See if the user was an administrator of this group
* See if the user specified this group as one of their
if (!was_member && !was_admin && !is_member)
continue;
if (was_admin && lflg) {
changed = 1;
}
if (was_member && (!Gflg || is_member)) {
if (lflg) {
changed = 1;
}
} else if (was_member && Gflg && !is_member) {
changed = 1;
} else if (!was_member && Gflg && is_member) {
changed = 1;
}
if (!changed)
continue;
changed = 0;
}
}
Fixes: 45c6603cc8 ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This function creates a temporary file, and returns a FILE pointer to
it. This avoids dealing with both a file descriptor and a FILE pointer,
and correctly deallocating the resources on error.
The code before this patch was leaking the file descriptor if fdopen(3)
failed.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
See asprintf(3):
RETURN VALUE
When successful, these functions return the number of bytes
printed, just like sprintf(3). If memory allocation wasn’t possi‐
ble, or some other error occurs, these functions will return -1,
and the contents of strp are undefined.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This will help add other labels in the following commits.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Resources should be freed in the inverse order of the allocation.
This refactor prepares for the following commits, which fix some leaks.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Instead of GNU builtins and extensions, these macros can be implemented
with C11's _Generic(3), and the result is much simpler (and safer, since
it's now an error, not just a warning).
Signed-off-by: Alejandro Colomar <alx@kernel.org>
'endptr' is appropriate internally in strtol(3) because it's a pointer
to 'end', and 'end' itself is a pointer to one-after-the-last character
of the numeric string. In other words,
endptr == &end
However, naming the pointer whose address we pass to strtol(3)'s
'endptr' feels wrong, and causes me trouble while parsing the code; I
need to double check the number of dereferences, because something feels
wrong in my head.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It's doesn't make much sense to break from a switch() just to return.
Let's return early, to simplify.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This means we set the pointees on error, which we didn't do before, but
since we return -1 on error and ignore (don't use) the pointees at call
site, that's fine.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
All 3 non-error paths in the second part resulted in *has_min = true.
Set in once before the switch(), to simplify.
This means we set this variable on error, which we didn't do before,
but since we return -1 on error and ignore (don't use) the pointees at
call site, that's fine.
Also, move a couple of *has_max = true statements to before a comment,
in preparation for future commits.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Set *has_{min,max} = false at the begining, so we only need to set them
to true later.
This means we set these variables on error, which we didn't do before,
but since we return -1 on error and ignore (don't use) the pointees at
call site, that's fine.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
libpam is enabled to provide `passwd` binary from this package, as there
are several password quality checks that are enabled through a PAM
module. Same reason to disable account-tools-setuid.
sssd is disabled because `files provider` has been removed in sssd, and
the underlying functionality in shadow isn't needed anymore.
libcrack dependency was disabled some time ago, but the upstream repo
wasn't updated. Doing it now.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
The manpages for newuidmap and newgidmap had a typo "[pid[" instead
of "[pid]". They were also unclear about what the /proc/pid fd should
be. Fix both.
Closes#977
Reported-by: igo95862@yandex.ru
Signed-off-by: Serge Hallyn <serge@hallyn.com>
If not enough memory is available for more environment variables, treat
it exactly like not enough memory for new environment variable content.
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
This silences a CodeQL warning. We don't care about reentrancy, but
after this patch we don't need to break a long line, so that's a win.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This macro makes sure that the first argument is an array, and
calculates its size.
Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Keep pot creation date out of our po files when we compare them.
Otherwise, we always think they need to be updated.
We prepend a line '# To re-generate, ....' to the shadow-man-pages.pot
file. Do that before we compare the new candidate, because right
now our comparison to see if we've made changes always thinks we have.
Put some of the tempfiles in a mktemp -d'd directory, which we remove when
all's done. This keeps the working tree cleaner.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
def_find can return NULL for unset, not just unknown, config options. So
move the decision of whether to log an error message about an unknown config
option back into def_find, which knows the difference. Only putdef_str()
will pass a char* srcfile to def_find, so only calls from putdef_str will
cause the message, which was the original intent of fa68441bc4.
closes#967
fixes: fa68441bc4 ("Improve the login.defs unknown item error message")
Signed-off-by: Serge Hallyn <serge@hallyn.com>
There are no guarantees that fstatat() does not clobber the stat
buffer on errors.
Use a temporary buffer so that the following code sees correct
attributes of the source entry.
Issue #973
Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
The combination of bzero and free could be optimized away.
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
This change executes `i++` one more time before breaking, so we need to
update the `i+1` after the loop to just `i`.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
cp can only be an empty string literal in that conditional. Use a
string literal to be more explicit.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Performance tests made in 2007 are obsolete. We should assume libc is
reasonably fast today (otherwise, report a bug to libc).
$ git blame -- lib/sgetgrent.c | grep strchr
45c6603cc (nekral-guest 2007-10-07 11:44:02 +0000 30) * WARNING: I profiled this once with and without strchr() calls
6f88bcf58 (nekral-guest 2008-05-26 08:31:14 +0000 97) cp = strchr (cp, ':');
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It's trivial to do the change, and it removes a CodeQL warning.
We don't need to be reentrant, but it doesn't hurt either.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It was always being called with 'day * DAY', so do that internally and
simplify. This grabs some code from print_day_as_date().
Cc: Tobias Stoeckmann <tobias@stoeckmann.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Amazing that this triggered no warnings at all.
Fixes: 355ad6a9e0 ("Have a single definition of date_to_str()")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Also, it was checking for >=0 for success, but since that code is for
opening a different tty as stdin, that was bogus. But since it's
guaranteed to be either 0 or -1, this commit doesn't add any code to
make sure it's 0 (i.e., we could say !=0 instead of ==-1). That's more
appropriate for a different commit.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Remove /*ARGSUSED*/ comments. Instead, use appropriate declarators for
main(). ISO C allows using int main(void) if the parameters are going
to be unused.
Also, do some cosmetic changes in the uses of argc and argv, to show
where they are used.
And use *argv[], instead of **argv. Array notation is friendlier, IMO.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
OPENLOG() already sets the program name as the prefix.
This resulted in entries like:
$ journalctl 2>/dev/null | grep passwd
Mar 03 01:09:47 debian passwd[140744]: passwd: can't view or modify password information for root
Fixes: 8e167d28af ("[svn-upgrade] Integrating new upstream version, shadow (4.0.8)")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Previously, we were performing the following two checks:
- if (ranges != ((argc + 2) / 3)) {
- if ((ranges * 3) > argc) {
Let's draw a table of the possible input that would pass the first check:
argc: 0 1 2 3 4 5 6 7 8 9
rng: 0 1 1 1 2 2 2 3 3 3
a+2/3*3:0 3 3 3 6 6 6 9 9 9 <-- this is roundup(argc, 3);
a+2/3: 0 1 1 1 2 2 2 3 3 3 <-- this is roundup(argc, 3) / 3;
rng*3: 0 3 3 3 6 6 6 9 9 9
From those, let's extract those that would also pass the second check:
argc: 0 3 6 9
rng: 0 1 2 3
rng*3: 0 3 6 9
We can see that there's a simple check for this input:
+ if (ranges * 3 != argc) {
As a sanity check, let's draw a table of the acceptable input with that
check:
rng: 0 1 2 3
rng*3: 0 3 6 9
argc: 0 3 6 9
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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 function should never be used; it's always used via its wrapper
macro. To simplify, and reduce chances of confusion: remove the
function, and implement the macro directly in terms of
stpcpy(mempcpy(strnlen())).
Update the documentation, and improve the example, which was rather
confusing.
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
A difference between 'struct utmp' and 'struct utmpx' is that
the former uses UT_LINESIZE for the size of its array members,
while the latter doesn't have a standard variable to get its
size. Therefore, we need to get the number of elements in
the array with NITEMS().
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This macro is useful to get the size of a member of a structure
without having a variable of that type.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
utmpx is specified by POSIX as an XSI extension. That's more portable
than utmp, which is unavailable for example in musl libc. The manual
page specifies that in Linux (but it probably means in glibc), utmp and
utmpx (and the functions that use them) are identical, so this commit
shouldn't affect glibc systems.
Assume utmpx is always present.
Also, if utmpx is present, POSIX guarantees that some members exist:
- ut_user
- ut_id
- ut_line
- ut_pid
- ut_type
- ut_tv
So, rely on them unconditionally.
Fixes: 170b76cdd1 ("Disable utmpx permanently")
Closes: <https://github.com/shadow-maint/shadow/issues/945>
Reported-by: Firas Khalil Khana <firasuke@gmail.com>
Reported-by: "A. Wilfox" <https://github.com/awilfox>
Tested-by: Firas Khalil Khana <firasuke@gmail.com>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The passwd silently truncated the password length to PASS_MAX.
This patch introduces check that prints an error message
and exits the call.
Signed-off-by: Tomas Halman <tomas@halman.net>
The passwd utility had hardcoded limit for password lenght set
to 200 characters. In the agetpass.c is used PASS_MAX for
this purpose.
This patch moves the PASS_MAX definition to common place
and uses it in both places.
Signed-off-by: Tomas Halman <tomas@halman.net>
If (maxsize == -1), then ((size_t)maxsize == SIZE_MAX). And no size can
ever be >= SIZE_MAX, so it will never return false if sysconf(3) reports
an unlimited user-name size via returning -1. Well, to be pedantic,
that disallows a user-name siz of precisely SIZE_MAX bytes when
sysconf(3) returns -1. However, that's probably a good thing; such a
long user name might trigger Undefined Behavior somewhere else, so be
cautious and disallow it. I hope nobody will be using the entire
address space for a user name.
The commit that introduced that check missed that this code had always
supported unlimited user-name sizes since it was introduced by Iker in
3b7cc05387 ("lib: replace `USER_NAME_MAX_LENGTH` macro"), and
6be85b0baf ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare
warning") even clarified this in the commit message.
So, while the code in 6a1f45d932 ("lib/chkname.c: Support unlimited
user name lengths") wasn't bad per se, the commit message was incorrect.
What that patch did was adding code for handling EINVAL (or any other
errors that a future kernel might add).
To be more pedantically correct, that commit also allowed (under certain
circumstances, user names of SIZE_MAX bytes, but those were originally
allowed (by accident), and only became disallowed in 403a2e3771
("lib/chkname.c: Take NUL byte into account"). But again, let's
disallow those, just to be cautious.
Link: <https://github.com/shadow-maint/shadow/pull/935>
Link: <https://github.com/shadow-maint/shadow/pull/935#discussion_r1477429492>
See-also: 6be85b0baf ("lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning")
Fixes: 6a1f45d932 ("lib/chkname.c: Support unlimited user name lengths")
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Tobias Stoeckmann <tobias@stoeckmann.org>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Before 3b7cc05387 ("lib: replace `USER_NAME_MAX_LENGTH` macro"), this
code did use a length. It used a utmp(5) fixed-width buffer, so the
length matches the buffer size (there was no terminating NUL byte).
However, sysconf(_SC_LOGIN_NAME_MAX) returns a buffer size that accounts
for the terminating null byte; see sysconf(3). Thus, the commit that
introduced the call to sysconf(3), should have taken that detail into
account.
403a2e3771 ("lib/chkname.c: Take NUL byte into account"), by Tobias,
caught that bug in <lib/chkname.c>, but missed that the same commit that
introduced that bug, introduced the same bug in two other places.
This fixes all remaining calls to sysconf(_SC_LOGIN_NAME_MAX).
I still observe some suspicious code after this fix:
if (do_rlogin(hostname, username, max_size - 1, term, sizeof(term)))
...
login_prompt(username, max_size - 1);
We're passing size-1 to functions that want a size. But since the fix
to those will be different, let's do that in the following commits.
Link: <https://github.com/shadow-maint/shadow/pull/935>
Link: <https://github.com/shadow-maint/shadow/issues/920#issuecomment-1926002209>
Link: <https://github.com/shadow-maint/shadow/pull/757>
Link: <https://github.com/shadow-maint/shadow/issues/674>
See-also: 403a2e3771 ("lib/chkname.c: Take NUL byte into account")
Fixes: 3b7cc05387 ("lib: replace `USER_NAME_MAX_LENGTH` macro")
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Tobias Stoeckmann <tobias@stoeckmann.org>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It is slightly confusing to allow adding these only to later refuse them.
Here is a (lightly tested :) patch to also refuse them when adding.
Signed-off-by: Tycho Andersen <tycho@tycho.pizza>
Days officially roll over at 00:00 UTC, not at 12:00 UTC. I see no
reason to add that half day.
Also, remove the comment. It's likely to get stale.
So, get_date() gets the number of seconds since the Epoch. I wonder how
that thing works, but I'll assume it's something similar to getdate(3)
+ mktime(3). After that, we need to convert seconds since Epoch to days
since Epoch. That should be a simple division, AFAICS, since Epoch is
"1970‐01‐01 00:00:00 +0000 (UTC)". See mktime(3).
Fixes: 45c6603cc8 ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Link: <https://github.com/shadow-maint/shadow/issues/939>
Reported-by: Michael Vetter <jubalh@iodoru.org>
Tested-by: Gus Kenion <https://github.com/kenion>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Very large values in /etc/shadow could lead to overflows. Make sure
that these calculations are saturated at LONG_MAX. Since entries are
based on days and not seconds since epoch, saturating won't hurt anyone.
Co-developed-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The conversion from day to seconds can be done in print_date
(renamed to print_day_as_date for clarification). This has the nice
benefit that DAY multiplication and long to time_t conversion are done
at just one place.
Co-developed-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
If the system does not have a user name length limit, support it
accordingly. If the system has no _SC_LOGIN_NAME_MAX, use
LOGIN_NAME_MAX constant instead.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
The _SC_LOGIN_NAME_MAX value includes space for the NUL byte. The length
of name must smaller than this value to be valid.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
The password returned by agetpass can be used directly without copying
it into a char array first.
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
Clarify how this endless while(true) loop can be stopped by using a
boolean variable as condition and turn it into a do-while loop.
Suggested-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
New option --stdin/-t is available for root user. It is useful
for automation/setup and it makes shadow utils passwd more versatile.
Signed-off-by: Tomas Halman <tomas@halman.net>
There is an inconsistent use of the MAYBE_UNUSED macro. Sometimes the
`int unused(x)` form is used form and others the `unused int x`. We'd
like to use the second form always.
Related-To: https://github.com/shadow-maint/shadow/issues/918
Suggested-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Pablo Saavedra <psaavedra@igalia.com>
This fixes build with glibc-2.33 (newer glibc merged libdl and libpthread
into libc):
```
libtool: link: x86_64-pc-linux-gnu-gcc -isystem /usr/include/bsd -DLIBBSD_OVERLAY -O2 -pipe -Wl,-O1 -o login login.o login_nopam.o -Wl,--as-needed ../lib/.libs/libshadow.a -lcrypt -lsystemd -lpam -lpam_misc -lbsd
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: ../lib/.libs/libshadow.a(libshadow_la-nss.o): undefined reference to symbol 'dlclose@@GLIBC_2.2.5'
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: /lib64/libdl.so.2: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
```
In Debian, the needed macro from libtool seems to be in libltdl-dev.
Signed-off-by: Sam James <sam@gentoo.org>
argv is passed to execve(3), which for historic reasons is non-const,
but doesn't modify the strings.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Uses of this macro indicate a code smell, but in some cases, libc
functions require breaking const correctness. Use this macro to wrap
casts in such cases, so that we limit the danger of the cast.
It only permits discarding const. Discarding any other qualifiers, or
doing other type changes should result in a compile-time error.
Link: <https://software.codidact.com/posts/286575/287345#answer-287345>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
strtou[l]l(3) silently converts negative numbers into positive. This
behavior is wrong: a negative value should be parsed as a negative
value, which would underflow unsigned (long) long, and so would return
the smallest possible value, 0, and set errno to ERANGE to report an
error.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These functions reject negative numbers, instead of silently converting
them into unsigned, which strtou[l]l(3) do.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The sulogin program calls pw_entry in a loop while incorrect root
passwords are entered.
Free the previously allocated memory to avoid memory exhaustion.
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
We don't need 'static', because it's in main(), which is only called
once. However, we will need initialization as if it were 'static', so
use ={} to initialize it. This will allow freeing the pointers before
they have been allocated.
Cc: Samanta Navarro <ferivoz@riseup.net>
Suggested-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Those variables are only used in main(). Restrict their scope.
Keep them static (.bss), as changing that may be dangerous.
Suggested-by: Samanta Navarro <ferivoz@riseup.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Add a variadic macro addsl() that accepts an arbitrary number of
addends, instead of having specific versions like addsl2() or addsl3().
It is internally implemented by the addslN() function, which itself
calls addsl2(). addsl3() is now obsolete and thus removed.
Code should just call addsl().
Link: <https://github.com/shadow-maint/shadow/pull/882#discussion_r1437155212>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This is for consistency with addsl3(), and in preparation for the
following commit, which will unify the interface into a single addsl()
macro.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
ISO C guarantees that #undef is a no-op if there is no such macro.
C11::6.10.3.5p2:
> A preprocessing directive of the form
>
> # undef identifier new-line
>
> causes the specified identifier no longer to be defined as a macro
> name. It is ignored if the specified identifier is not currently
> defined as a macro name.
Link: <http://port70.net/~nsz/c/c11/n1570.html#6.10.3.5p2>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These functions (e.g., gr_free()), explicitly dereference the pointer
and read the pointee.
The /@out@/ comment, which is (almost) analogous to the
[[gnu::access(write_only, ...)]] attribute, means that the pointee can
be uninitialized, since it won't read it. There's a difference between
/@out@/ and the GCC attribute: the attribute doesn't require that the
call writes to the pointee, while /@out@/ requires that the pointee be
fully initialized after the call, so it _must_ write to it.
A guess of why it was used is that these functions are similar to
free(3), which does not read the memory it frees, and so one would
assume that if it doesn't read, write_only (or equivalents) are good.
That's wrong in several ways:
- free(3) does not read _nor_ write to the memory, so it would
be slightly inappropriate to use write_only with it. It wouldn't be
"wrong", but [[gnu::access(none, ...)]] would be more appropriate.
- Because /@out@/ requires that the call writes to the pointee, it
would be wrong to use it in free(3), which doesn't write to the
pointee.
- Our functions are similar to free(3) conceptually, but they don't
behave like free(3), since they do read the memory (pointee) (and
also write to it), and thus they're actually read_write.
Link: <https://splint.org/manual/manual.html#undefined>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
If reallocation fails in function list, then reset the size to 0 again.
Without the reset, the next call assumes that `members` points to
a memory location with reserved space.
Also use size_t instead of int for size to prevent signed integer
overflows. The length of group lines is not limited.
Fixes 45c0003e53 (4.14 release series)
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
The size of time_t varies across systems, but since data type long is
more than enough to calculate with days (precision of shadow file),
use it instead.
Just in case a shadow file contains huge values, check for a possible
signed integer overflow.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Link: <https://github.com/shadow-maint/shadow/pull/876>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Contrary to the comment in ENV_HZ.xml, ENV_HZ is not even used in
sulogin (anymore) if PAM support is enabled.
Skip paragraphs of sulogin if PAM support is enabled, since they would
be empty now.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Special care has to be taken for 32 bit systems with a 64 bit time_t,
since their long data type is still 32 bit.
Since this macro expresses a number of seconds, and seconds are in units
of 'time_t' in C, the appropriate type for the multiplication is
'time_t'.
Reported-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The values are retrieved from login.defs files, which normally do not
contain negative values. In fact, negative value -1 is used in many
code places as "feature disabled", which is normally achieved by
simply commenting out the key from the file.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.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>
These functions are like [v]snprintf(3), but return -1 on truncation,
which makes it easier to test. In fact, the API of swprintf(3), which
was invented later than snprintf(3), and is the wide-character version
of it, is identical to this snprintf_().
snprintf(3) is iseful in two cases:
- We don't care if the output is truncated. snprintf(3) is fine for
those, and the return value can be ignored. But snprintf_() is also
fine for those.
- Truncation is bad. In that case, it's as bad as a hard error (-1)
from snprintf, so merging both problems into the same error code
makes it easier to handle errors. Return the length if no truncation
so that we can use it if necessary.
Not returning the whole length before truncation makes a better API,
which need not read the entire input, so it's less vulnerable to DoS
attacks when a malicious user controls the input.
Use these functions to implement SNPRINTF().
Cc: Samanta Navarro <ferivoz@riseup.net>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It wraps snprintf(3) so that it performs some steps that one might
forget, or might be prone to accidents:
- It calculates the size of the destination buffer, and makes sure it's
an array (otherwise, using sizeof(s) would be very bad).
- It calculates if there's truncation or an error, returning -1 if so.
BTW, this macro doesn't have any issues of double evaluation, because
sizeof() doesn't evaluate its argument (unless it's a VLA, but then the
static_assert(3) within NITEMS() makes sure VLAs are not allowed).
This macro is very similar to STRTCPY(), defined in
<lib/string/strtcpy.h>.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
commonio.c: In function 'commonio_unlock':
commonio.c:487:49: warning: '.lock' directive output may be truncated writing 5 bytes into a region of size between 1 and 1024 [-Wformat-truncation=]
487 | snprintf (lock, sizeof lock, "%s.lock", db->filename);
| ^~~~~
commonio.c:487:17: note: 'snprintf' output between 6 and 1029 bytes into a destination of size 1024
487 | snprintf (lock, sizeof lock, "%s.lock", db->filename);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Check for close(2) failure at more places closing a file descriptor
written to.
Also ignore failures with errno set to EINTR (see man:close(2) for
details).
sprintf(3) does not take the destination buffer into account. Although
the destination in these case is large enough, sprintf(3) indicates a
code smell.
Use the xasprintf() wrapper.
Group them at the end of the list of variable definitions, and use
'#if defined()' instead of '#if[n]def'. Also indent nested ones.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
ITI_AGING is not set through any build environment. If it would be set,
then timings in /etc/shadow would not fit anymore.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
su.c:678:26: warning: format ‘%s’ expects argument of type ‘char *’, but argument 4 has type ‘const void *’ [-Wformat=]
su.c:681:44: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘const void *’ [-Wformat=]
su.c:683:46: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘const void *’ [-Wformat=]
Reported-by: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We don't need to terminate them manually after the call. Remove all
that paranoid code, which in some cases was even wrong. While at it,
let's do a few more things:
- Use sizeof(buf) for the size of the buffer. I found that a few cases
were passing one less byte (probably because the last one was
manually zeroed later). This caused a double NUL. snprintf(3) wants
the size of the entire buffer to properly terminate it. Passing the
exact value hardcoded is brittle, so use sizeof().
- Align and improve style of variable declarations. This makes them
appear in this diff, which will help review the patch.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These functions consume a source string. Document that. There's no way
to mark that they also produce a string in dst, though. That will be up
to the static analyzer to guess.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It signals that a function parameter is a string _before_ the call.
Suggested-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
- Set errno = 0 before the call. Otherwise, it may contain anything.
- ERANGE is not the only possible errno value of these functions. They
can also set it to EINVAL.
- Any errno value after these calls is bad; just compare against 0.
- Don't check for the return value; just errno. This function is
guaranteed to not modify errno on success (POSIX).
- Check endptr == str, which may or may not set EINVAL.
Suggested-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Since failure() is [[noreturn]], we can invert the conditional so that
we don't need an else. This silences a -Wunused-parameter warning.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This simplifies the code a little bit, and prepares for the next
commits, which will clean up further.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The multiplication was already invoking UB. The test was flawed.
Use __builtin_mul_overflow() instead.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
strtol(3) doesn't specify a return value if (value == endptr).
It is always an error, if (value==endptr).
Signed-off-by: Alejandro Colomar <alx@kernel.org>
I used size_t because:
sysconf(3) can return -1 if the value is not supported, but then it can
only mean that there's no limit. Having no limit is the same as having
a limit of SIZE_MAX (to which -1 is converted).
Signed-off-by: Alejandro Colomar <alx@kernel.org>
By writing the terminating null byte via stpcpy(3), we take advantage of
_FORTIFY_SOURCE for the last byte, which was unprotected before this
commit.
Reported-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We were using strncpy(3), which is designed to copy from a string into a
(null-padded) fixed-size character array. However, we were doing the
opposite: copying from a known-size array (which was a prefix of a
string), into a string. That's why we had to manually zero the buffer
afterwards.
Use instead mempcpy(3) to copy the non-null bytes, and then terminate
with a null byte with stpcpy(..., "").
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Upcoming `gcc-14` enabled a few warnings into errors, like
`-Wimplicit-function-declaration`. This caused `shadow` build to fail
as:
pwunconv.c: In function 'main':
pwunconv.c:132:13: error: implicit declaration of function 'getdef_bool' [-Wimplicit-function-declaration]
132 | if (getdef_bool("USE_TCB")) {
| ^~~~~~~~~~~
The change adds missing include headers.
If an entry in /etc/shells is not an absolute path (comments or
partial reads due to fgets), the line should not be considered as
a valid login shell.
In general all systems should have getusershells, but let's better
be safe than sorry.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
For arrays of char, both NITEMS() and SIZEOF_ARRAY() return the same
value. However, NITEMS() is more appropriate. Think of wide-character
equivalents of the same code; with NITEMS(), they would continue to be
valid, while with SIZEOF_ARRAY(), they would be wrong.
In the implementation of ZUSTR2STP(), we want SIZEOF_ARRAY() within the
static assert, because we're just comparing the sizes of the source and
destination buffers, and we don't care if we compare sizes or numbers of
elements, and using sizes is just simpler. But we want NITEMS() in the
zustr2stp() call, where we want to copy a specific number of characters.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We've recently fixed several bugs in the calculation of the size in this
function call. Use this wrapper to prevent similar mistakes in the
future.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This wrapper calculates the destination buffer's size, to avoid errors
in the size calculation.
A curious fact: this macro did exist in Version 7 Unix (with a slightly
different name). I found it by chance, investigating the origins of
strncpy(3) and strncat(3) in V7, after Branden suggested me to do so,
related to recent discussions about string_copying(7).
alx@debian:~/src/unix/unix/Research-V7$ grepc SCPYN .
./usr/src/cmd/login.c:#define SCPYN(a, b) strncpy(a, b, sizeof(a))
Our implementation is slightly better, because using nitems() we're
protected against passing a pointer instead of an array, and it's also
conceptually more appropriate: for wide characters, it would be
#define WCSNCPY(dst, src) wcsncpy(dst, src, NITEMS(dst))
Cc: "G. Branden Robinson" <branden@debian.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We're not even zeroing the last byte after this call. This was a
completely gratuitous truncation of one byte, and the resulting
character array still wasn't guaranteed to be null terminated, because
strncpy(3) can't do that.
Just to clarify, none of these structures needed zeroing, as they are
treated as null-padded fixed-size character arrays. Calling strncpy(3)
was actually the correct call, and the only problem was unnecessarily
truncating strings by one byte more than necessary.
Cc: Matthew House <mattlloydhouse@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This call was too clever. It relied on the last byte of ll_line
being 0 due to a previous memzero() and not writing to it later.
Write an explicit terminating null byte, by using STRTCPY().
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This call was way too clever. It relied on the last byte of fail_line
being 0 due to it being in a static structure and never writing to it.
Write an explicit terminating null byte, by using STRTCPY().
Cc: Matthew House <mattlloydhouse@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We were copying from a (zero-padded) fixed-width character array to a
string, but strncpy(3) is meant to do the opposite thing. ZUSTR2STP()
is designed to be used in this case (like strncat(3)).
Fixes: f40bdfa66a ("libmisc: implement `get_session_host()`")
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These functions don't seem to exist anymore. I can't find them in
Debian, nor in a web search. They probably were functions from an
ancient implementation of cracklib that doesn't exist anymore.
$ git remote -v
origin git@github.com:cracklib/cracklib.git (fetch)
origin git@github.com:cracklib/cracklib.git (push)
$ grep -rni fascisthistory
$ git log --grep FascistHistory
$ git log -S FascistHistory
Closes: <https://codesearch.debian.net/search?q=FascistHistory&literal=1>
Cc: Mike Frysinger <vapier@gentoo.org>
Acked-by: Michael Vetter <jubalh@iodoru.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
strftime(3) makes no guarantees about the contents of the buffer if the
formatted string wouldn't fit in the buffer. It simply returns 0, and
it's the programmer's responsibility to do the right thing after that.
Let's write the string "future" if there's an error, similar to what we
do with gmtime(3)'s errors.
Also, `buf[size - 1] = '\0';` didn't make sense. If the copy fits,
strftime(3) guarantees to terminate with NUL. If it doesn't, the entire
contents of buf are undefined, so adding a NUL at the end of the buffer
would be dangerous: the string could contain anything, such as
"gimme root access now". Remove that, now that we set the string to
"future", as with gmtime(3) errors. This setting to '\0' comes from the
times when we used strncpy(3) in the implementation, and should have
been removed when I changed it to use strlcpy(3); however, I didn't
check we didn't need it anymore.
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
ut_line doesn't hold a string. It is a null-padded fixed-width array.
Luckily, I don't think there has ever existed a ut_line ("/dev/tty*")
that was 32 bytes long. That would have resulted in a buffer overrun.
Anyway, do the right thing, which is copying into a temporary string.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
There's been a very long and interesting discussion in linux-man@ and
libc-alpha@, where we've discussed all the string-copying functions,
their pros and cons, when should each be used and avoided, etc.
Paul Eggert pointed out an important problem of strlcpy(3): it is
vulnerable to DoS attacks if an attacker controls the length of the
source string. And even if it doesn't control it, the function is dead
slow (because its API forces it to calculate strlen(src)).
We've agreed that the general solution for a truncating string-copying
function is to write a wrapper over strnlen(3)+memcpy(3), which is
limited to strnlen(src, sizeof(dst)). This is not vulnerable to DoS,
and is very fast for all buffer sizes. string_copying(7) has been
updated to reflect this, and provides a reference implementation for
this wrapper function.
This strtcpy(3) (t for truncation) wrapper happens to have the same API
that our strlcpy_() function had, so replace it with the better
implementation. We don't need to update callers nor tests, since the
API is the same.
A future commit will rename STRLCPY() to STRTCPY(), and replace
remaining calls to strlcpy(3) by calls to this strtcpy(3).
Link: <https://lore.kernel.org/linux-man/ZU4SDh-Se5gjPny5@debian/T/#mfb5a3fdeb35487dec6f8d9e3d8548bd0d92c4975/>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This test fails now, due to a bug: the return type of strlcpy_() is
size_t, but it should be ssize_t. The next commit will pass the test,
by fixing the bug.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
libbsd is unwanted in Fedora and RHEL, and the recently released Fedora
39 doesn't contain this dependency in the base image.
shadow removed libbsd from its dependencies for Fedora 39, so let's
build without it to avoid compilation errors.
Resolves: https://github.com/shadow-maint/shadow/issues/839
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
free(NULL) is valid; there's no need to check for NULL. Simplify.
Fixes: 5178f8c5af ("utmp: call prepare_utmp() even if utent is NULL")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
copydir.c:429:4: warning: Value stored to 'err' is never read [deadcode.DeadStores]
Also reduce indentation by bailing out early.
(cherry picked from commit d89f2fb06d1b81b56299f9d0bfe7a927a2282f19)
Closes#746
Only print the 'unknown item' message to syslog if we are
actually parsing a login.defs. Prefix it with "shadow:" to make
it clear in syslog where it came from.
Also add the source filename to the console message. I'm not
quite clear on the econf API, so not sure whether in that path we
will end up actually having the path, or printing ''.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
This is not just a style issue. This should be a hard error, and never
compile. ISO C89 already had this feature as deprecated. ISO C99
removed this deprecated feature, for good reasons. If we compile
ignoring this warning, shadow is not going to behave well.
Cc: Sam James <sam@gentoo.org>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
asprintf(3) is non-standard, but is provided by GNU, the BSDs, and musl.
That makes it portable enough for us to use.
This function is much simpler than the burdensome code for allocating
the right size. Being simpler, it's thus safer.
I took the opportunity to fix the style to my preferred one in the
definitions of variables used in these calls, and also in the calls to
free(3) with these pointers. That isn't gratuituous, but has a reason:
it makes those appear in the diff for this patch, which helps review it.
Oh, well, I had an excuse :)
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
As other x...() wrappers around functions that allocate, these wrappers
are like [v]asprintf(3), but exit on failure.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Badnames still accepted, note that previously usage already stated
singular form, whilst manpage and real one was plural only.
Fixes: 45d6746219 ("src: correct "badname" option")
Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com>
The file isn't up to date with the latest development, the last change
was made 15 years ago, so I'm removing it.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Another file that I remove with sadness. We were unable to complete the
first item but we are working hard on it.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
I remove this file with sadness, as it contains data from old times.
Unfortunately, this data is no longer relevant. The source code
management tool will keep it in memory.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
I guess we are keeping this for historical purposes more than anything
else. If so, anybody can check the git history to recover the
specification.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Having source code in a compressed file doesn't seem like a good idea. I
checked several distributions and they don't distribute this binary, so
let's remove it.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
The patch is never applied upstream. If I were to take a gamble, I would
even say that it throws an error when trying to patch.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Not sure what this file is exactly, but there's already a groupmems.c
that should generate the binary responsible for managing the members of
a user's primary group.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
AFAIK, it isn't included in any distribution and it isn't used
internally in the project, so let's remove it.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
And do not set 'clear' to point to the empty string. After this commit,
'clear' only stores the result of getpass(3). This will be useful to
change the code to use agetpass().
$ grep '\<clear\>' lib/pwauth.c;
char *clear = NULL;
clear = getpass (prompt);
input = (clear == NULL) ? "" : clear;
clear = getpass (prompt);
input = (clear == NULL) ? "" : clear;
if (NULL != clear) {
strzero (clear);
Signed-off-by: Alejandro Colomar <alx@kernel.org>
There are no users of 'clear_pass' and 'wipe_clear_pass'.
$ grep -rn '\<clear_pass\>'
lib/pwauth.c:35:/*@null@*/char *clear_pass = NULL;
lib/pwauth.c:199: * not wipe it (the caller should wipe clear_pass when it is
lib/pwauth.c:203: clear_pass = clear;
$ grep -rn wipe_clear_pass
lib/pwauth.c:34:bool wipe_clear_pass = true;
lib/pwauth.c:198: * if the external variable wipe_clear_pass is zero, we will
lib/pwauth.c:204: if (wipe_clear_pass && (NULL != clear) && ('\0' != *clear)) {
ChangeLog:3813: * lib/pwauth.c: Use a boolean for wipe_clear_pass and use_skey.
Remove them.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
If the destination buffer is an array, we can check our assumptions.
This adds a readable way to explain that dsize must be strictly > ssize.
The reason is that the destination string is the source + '\0'.
If the destination is not an array, it's up to _FORTIFY_SOURCE or
-fanalyzer to catch newly introduced errors. There's nothing we can do;
at least not portably.
Suggested-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This function is like strlcpy(3), but returns -1 on truncation, which
makes it much easier to test. strlcpy(3) is useful in two cases:
- We don't care if the output is truncated. strlcpy(3) is fine for
those, and the return value can be ignored.
- Truncation is bad. In that case, we just want to signal truncation,
and the length of the original string is quite useless. Return the
length iff no truncation so that we can use it if necessary.
This simplifies the definition of the STRLCPY() macro.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It was blessed by POSIX.1-2001, and GCC says that it won't go away,
possibly ever.
memset(3) is dangerous, as the 2nd and 3rd arguments can be accidentally
swapped --who remembers what's the order of the 2nd and 3rd parameters
to memset(3) without checking the manual page or some code that uses
it?--. Some recent compilers may be able to catch that via some
warnings, but those are not infalible. And even if compiler warnings
could always catch that, the time lost in fixing or checking the docs is
lost for no clear gain. Having a sane API that is unambiguous is the
Right Thing (tm); and that API is bzero(3).
If someone doesn't believe memset(3) is error-prone, please read the
book "Unix Network Programming", Volume 1, 3rd Edition by Stevens, et
al., Section 1.2. See a stackoverflow reference in the link below[1].
bzero(3) had a bad fame in the bad old days, because some ancient
systems (I'm talking of many decades ago) shipped a broken version of
bzero(3). We can assume that all systems in which current shadow utils
can be built, have a working version of bzero(3) --if not, please fix
your broken system; don't blame the programmer--.
One reason that some use today to avoid bzero(3) in favor of memset(3)
is that memset(3) is more often used; but that's a circular reasoning.
Even if bzero(3) wasn't supported by the system, it would need to be
invented. It's the right API.
Another reason that some argue is that POSIX.1-2008 removed the
specification of bzero(3). That's not a problem, because GCC will
probably support it forever, and even if it didn't, we can redefine it
like we do with memzero(). bzero(3) is just a one-liner wrapper around
memset(3).
Link: [1] <https://stackoverflow.com/a/17097978>
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This makes it harder to make mistakes while editing the code. Since the
sizeof's can be autocalculated, let the machine do that. It also
reduces the cognitive load while reading the code.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It's not being used anymore. We got rid of it in favor of better APIs.
Well, it's still being used in one place: a contrib/ patch, but I
explicitly want to break it, so that someone reviews it. I don't want
to modify it, since it's not being tested, so it would be very risky for
me to touch it. Instead, let it bitrot, and if someone cares, they'll
update it correctly.
BTW, the comment that said /* danger -side effects */ was wrong:
sizeof() doesn't evaluate the argument (unless it's a VLA), so there
wasn't really a double-evaluation issue.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It wraps strlcpy(3bsd) so that it performs some steps that one might
forget, or might be prone to accidents:
- It calculates the size of the destination buffer, and makes sure it's
an array (otherwise, using sizeof(dst) would be very bad).
- It calculates if there's truncation, returning an easy-to-use value.
BTW, this macro doesn't have any issues of double evaluation, because
sizeof() doesn't evaluate its argument (unless it's a VLA, but then
the static_assert(3) within SIZEOF_ARRAY() makes sure VLAs are not
allowed).
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It's a wrapper around zustr2stp() that calls SIZEOF_ARRAY() internally.
The function call is usually --in our code base, always-- called with an
array as the second argument. For such an argument, one should call
SIZEOF_ARRAY(). To avoid mistakes, and simplify usage, let's add this
macro that does it internally.
BTW, this macro doesn't have any issues of double evaluation, because
sizeof() doesn't evaluate its argument (unless it's a VLA, but then
the static_assert(3) within SIZEOF_ARRAY() makes sure VLAs are not
allowed).
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These calls were intending to copy from a NUL-padded (possibly
non-NUL-terminated) character sequences contained in fixed-width arrays,
into a string, where extra padding is superfluous. Use the appropriate
call, which removes the superfluous work. That reduces the chance of
confusing maintainers about the intention of the code.
While at it, use the appropriate third parameter, which is the size of
the source buffer, and not the one of the destination buffer. As a side
effect, this reduces the use of '-1', which itself reduces the chance of
off-by-one bugs.
Also, since using sizeof() on an array is dangerous, use SIZEOF_ARRAY().
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
There's no standard function that copies from a null-padded character
sequence into a string.
A few standard functions can be workarounded to do that:
- strncat(3): This function is designed to catenate from a null-padded
character sequence into a string. The catch is that there's no
*cpy() equivalent of it --strncpy(3) is not at all related to
strncat(3); don't be fooled by the confusing name--, so one would
need to zero the first byte before the call to strncat(3). It also
has the inconvenient that it returns a useless value.
- strncpy(3): This function is designed to copy from a string to a
null-padded character sequence; the opposite of what we want to do.
If one passes the size of src instead of the size of dst, and then
manually zeroes the last byte of the dst buffer, something similar
to what we want happens. However, this does more than what we want:
it also padds with NUL the remaining bytes after the terminating NUL.
That extra work can confuse maintainers to believe that it's
necessary. That is exactly what happens in logout.c.
src/logoutd.c-46- /*
src/logoutd.c-47- * ut_user may not have the terminating NUL.
src/logoutd.c-48- */
src/logoutd.c:49: strncpy (user, ut->ut_user, sizeof (ut->ut_user));
src/logoutd.c-50- user[sizeof (ut->ut_user)] = '\0';
In that logout.c case --and in most invocations of strncpy(3), which
is usually a wrong tool-- the extra work is not wanted, so it's
preferrable to use the right tool, a function that does exactly
what's needed and nothing more than that. That tool is zustr2stp().
Read string_copying(7) for a more complete comparison of string copying
functions.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This makes it safe to call sizeof() on an array. Calling sizeof()
directly on an array is dangerous, because if the array changes to be a
pointer, the behavior will unexpectedly change. It's the same problem
as with NITEMS().
Link: <https://stackoverflow.com/a/57537491>
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
There's no need to have these as macros, so use functions, which are a
lot safer: there's no need to worry about multiple evaluation of args,
and there's also more type safety. Compiler warnings are also simpler,
as they don't dump all the nested macros.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
These comments were wrong. Remove them instead of fixing them, since
now that we have this small header file, it's much easier to follow the
preprocessor conditionals.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
sprintf(3) does not take the destination buffer into account. Although
the destination in these case is large enough, sprintf(3) indicates a
code smell.
Use snprintf(3).
commonio.c:522:15: warning: Although the value stored to 'cp' is used in the enclosing expression, the value is never actually read from 'cp' [deadcode.DeadStores]
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Documentation:
- Correct the comment documenting the function:
write_full() doesn't write "up to" count bytes (which is write(2)'s
behavior, and exactly what this function is designed to avoid), but
rather exactly count bytes (on success).
- While fixing the documentation, take the time to add a man-page-like
comment as in other APIs. Especially, since we'll have to document
a few other changes from this patch, such as the modified return
values.
- Partial writes are still possible on error. It's the caller's
responsibility to handle that possibility.
API:
- In write(2), it's useful to know how many bytes were transferred,
since it can have short writes. In this API, since it either writes
it all or fails, that value is useless, and callers only want to know
if it succeeded or not. Thus, just return 0 or -1.
Implementation:
- Use `== -1` instead of `< 0` to check for write(2) syscall errors.
This is wisdom from Michael Kerrisk. This convention is useful
because it more explicitly tells maintainers that the only value
which can lead to that path is -1. Otherwise, a maintainer of the
code might be confused to think that other negative values are
possible. Keep it simple.
- The path under `if (res == 0)` was unreachable, since the loop
condition `while (count > 0)` precludes that possibility. Remove the
dead code.
- Use a temporary variable of type `const char *` to avoid a cast.
- Rename `res`, which just holds the result from write(2), to `w`,
which more clearly shows that it's just a very-short-lived variable
(by it's one-letter name), and also relates itself more to write(2).
I find it more readable.
- Move the definition of `w` to the top of the function. Now that the
function is significantly shorter, the lifetime of the variable is
clearer, and I find it more readable this way.
Use:
- Also use `== -1` to check errors.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
If shadow is built without libbsd support, then readpassphrase() needs
to be provided from the project.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
@@ -56,8 +59,8 @@ static int chown_tree_at (int at_fd,
/*
*Skipthe"."and".."entries
*/
if((strcmp(ent->d_name,".")==0)
||(strcmp(ent->d_name,"..")==0)){
if(streq(ent->d_name,".")
||streq(ent->d_name,"..")){
continue;
}
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
