With glibc we can use "e" in mode argument to set O_CLOEXEC on
opened files. The /etc/shadow and /etc/gshadow file handles should
be protected to make sure that they are never passed to child
processes by accident.
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Calling exit might trigger cleanup functions registered through
atexit. Since some programs use this mechanism, be extra cautious to
never release passwd/group locks too early.
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
GNU Hurd doesn't define LOGIN_NAME_MAX. GNU Hurd recommends having no
system limits. When a program needs a limit, because it needs to
validate user input, it is recommended that each program defines its own
limit macros. The rationale is that this avoids hard-coded limits in
ABIs, which cannot be modified ever.
However, that doesn't mean that programs should have no limits at all.
We use this limit for validating user input, and so we shouldn't allow
anything just because the system doesn't want to set a limit.
So, when sysconf(2) returns -1, either due to an error or due to a claim
for no limits, we must fall back to the LOGIN_NAME_MAX value. And if
the system doesn't define that value, we must define it ourselves (we're
more or less free to choose any value, so let's pick the one that glibc
provides nowadays).
Fixes: 6a1f45d932 (2024-02-04; "lib/chkname.c: Support unlimited user name lengths")
Closes: <https://github.com/shadow-maint/shadow/issues/1166>
Cc: Chris Hofstaedtler <zeha@debian.org>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Reviewed-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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>
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>
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 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>
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>
