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>
