Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libnss_tcb: Match interfaces with NSS documentation. #31

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

libnss_tcb: Match interfaces with NSS documentation. #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,23 @@
2024-12-20 Björn Esser <besser82 at fedoraproject.org>

libnss_tcb: Match interfaces with NSS documentation.
According to [1] the interfaces provided are of return-type
'enum nss_status', of which the _nss_database_getXXX_r() interfaces
are mandated to take four parameters: a pointer to the result,
a pointer to an additional buffer, the size of the buffer supplied,
and a pointer to an integer to report error values (errnop). The
returned status values are meant to be accompanied by a corresponding
error value [2] passed through the errnop pointer.
[1] https://www.gnu.org/software/libc/manual/html_node/NSS-Module-Function-Internals.html
[2] https://www.gnu.org/software/libc/manual/html_node/NSS-Modules-Interface.html
* libs/nss.c (_nss_tcb_setspent): Adapt return-type and use return
values from documented macro.
(_nss_tcb_endspent): Likewise.
(_nss_tcb_getspnam_r): Adapt return-type, change fourth parameter
to be 'int *errnop', use return values from documented macros, and
set proper errno in errnop before returning.
(_nss_tcb_getspent_r): Likewise.

libnss_tcb: Disallow potentially-malicious user names in getspnam(3).
IEEE Std 1003.1-2001 allows only the following characters to appear
in group- and usernames: letters, digits, underscores, periods,
Expand Down
68 changes: 48 additions & 20 deletions libs/nss.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,27 +13,27 @@

static __thread DIR *tcbdir = NULL;

int _nss_tcb_setspent(void)
enum nss_status _nss_tcb_setspent(void)
{
if (!tcbdir) {
tcbdir = opendir(TCB_DIR);
if (!tcbdir)
return NSS_STATUS_UNAVAIL;

return 1;
return NSS_STATUS_SUCCESS;
}

rewinddir(tcbdir);
return 1;
return NSS_STATUS_SUCCESS;
}

int _nss_tcb_endspent(void)
enum nss_status _nss_tcb_endspent(void)
{
if (tcbdir) {
closedir(tcbdir);
tcbdir = NULL;
}
return 1;
return NSS_STATUS_SUCCESS;
}

/******************************************************************************
Expand Down Expand Up @@ -91,57 +91,80 @@ static FILE *tcb_safe_open(const char *file, const char *name)
return f;
}

int _nss_tcb_getspnam_r(const char *name, struct spwd *__result_buf,
char *__buffer, size_t __buflen, struct spwd **__result)
enum nss_status _nss_tcb_getspnam_r(const char *name,
struct spwd *__result_buf, char *__buffer, size_t __buflen, int *__errnop)
{
FILE *f;
char *file;
int retval, saved_errno;

/* Disallow potentially-malicious user names */
if (!is_valid_username(name)) {
errno = ENOENT;
/* we don't serve an entry here */
*__errnop = ENOENT;
return NSS_STATUS_NOTFOUND;
}

if (asprintf(&file, TCB_FMT, name) < 0)
if (asprintf(&file, TCB_FMT, name) < 0) {
/* retry, as malloc or another resource has failed */
*__errnop = EAGAIN;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should maybe be *__errnop = errno, because asprintf may set perhaps ENOMEM and it'd be right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the specs EAGAIN looks correct to me, as NSS_STATUS_TRYAGAIN with errnop EAGAIN means: "One of the functions used ran temporarily out of resources or a service is currently not available."

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation says that in case of NSS_STATUS_TRYAGAIN only ERANGE has a special meaning, and everything else is as good as EAGAIN. The implementation agrees with the documentation, there is some special handling for ERANGE, but besides that all errno values are equal. In fact, __nss_getent_r returns EAGAIN in case of NSS_STATUS_TRYAGAIN regardless of errno value.

return NSS_STATUS_TRYAGAIN;
}

f = tcb_safe_open(file, name);
free(file);
if (!f)
return errno == ENOENT ? NSS_STATUS_NOTFOUND : NSS_STATUS_UNAVAIL;
if (!f) {
/* $user/shadow not existing nor readable */
*__errnop = ENOENT;
return NSS_STATUS_UNAVAIL;
}
Comment on lines -111 to +120
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original logic seems to be correct: ENOENT means that the shadow file doesn't exist, in that case NSS_STATUS_NOTFOUND (The requested entry is not available) should be returned. If the shadow file is not available for other reasons, than NSS_STATUS_UNAVAIL should be returned.


retval = fgetspent_r(f, __result_buf, __buffer, __buflen, __result);
retval = fgetspent_r(f, __result_buf, __buffer,
__buflen, &__result_buf);
Comment on lines -114 to +123
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure, this is a bug fix: when fgetspent_r returns a nonzero value, it writes NULL at the address specified by its last argument. Before this change, that address used to be &errno.

saved_errno = errno;
fclose(f);
errno = saved_errno;
if (!retval)
return NSS_STATUS_SUCCESS;

switch (saved_errno) {
/* real error number is retval from fgetspent_r(),
by NSS spec errnop *MUST NOT* be set to 0 */
if (retval)
*__errnop = retval;

switch (retval) {
Comment on lines -121 to +133
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, the error code returned by fgetspent_r is currently either ENOENT or EAGAIN, it doesn't necessarily have to match the value of errno.

case 0:
/* no error, entry found */
return NSS_STATUS_SUCCESS;

case ENOENT:
/* if the file would not exist nor be readable, we would
have already bailed out with ENOENT/NSS_STATUS_UNAVAIL
immediately after the call to tcb_safe_open() */
return NSS_STATUS_NOTFOUND;

case EAGAIN:
/* ressources are temporary not available */
return NSS_STATUS_TRYAGAIN;

case ERANGE:
/* buffer too small */
return NSS_STATUS_TRYAGAIN;

default:
/* something else, e.g. parser error, but we can't help it */
return NSS_STATUS_UNAVAIL;
}
}

int _nss_tcb_getspent_r(struct spwd *__result_buf,
char *__buffer, size_t __buflen, struct spwd **__result)
enum nss_status _nss_tcb_getspent_r(struct spwd *__result_buf,
char *__buffer, size_t __buflen, int *__errnop)
{
struct dirent *result;
off_t currpos;
int retval, saved_errno;

if (!tcbdir) {
errno = ENOENT;
/* tcbdir does not exist */
*__errnop = ENOENT;
return NSS_STATUS_UNAVAIL;
}

Expand All @@ -154,21 +177,26 @@ int _nss_tcb_getspent_r(struct spwd *__result_buf,
closedir(tcbdir);
errno = saved_errno;
tcbdir = NULL;
/* cannot iterate tcbdir */
*__errnop = ENOENT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that our previous errno logic here was right, so maybe ENOENT is right here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as we cannot iterate over tcbdir for unknown reasons, so we should indicate "A necessary input file cannot be found."

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that in practice __errnop == &errno, assigning different values to errno and *__errnop doesn't make sense.

return NSS_STATUS_UNAVAIL;
}
if (!result) {
closedir(tcbdir);
errno = ENOENT;
errno = saved_errno;
tcbdir = NULL;
/* we have no more entries in tcbdir */
*__errnop = ENOENT;
Comment on lines -161 to +189
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise.

return NSS_STATUS_NOTFOUND;
}
errno = saved_errno;
} while (!strcmp(result->d_name, ".") ||
!strcmp(result->d_name, "..") || result->d_name[0] == ':');

retval = _nss_tcb_getspnam_r(result->d_name, __result_buf, __buffer,
__buflen, __result);
__buflen, __errnop);

/* errnop has already been set by _nss_tcb_getspnam_r() */
switch (retval) {
case NSS_STATUS_SUCCESS:
return NSS_STATUS_SUCCESS;
Expand Down
Loading