Page MenuHomeFreeBSD
Differential D23198 Diff 67386 lib/libpam/modules/pam_login_access/login_access.c

Changeset View

Standalone View

View Options

lib/libpam/modules/pam_login_access/login_access.c

Context not available.
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include <sys/types.h> #include <sys/types.h>
#include <sys/param.h>
#include <ctype.h> #include <ctype.h>
#include <errno.h> #include <errno.h>
#include <grp.h> #include <grp.h>
#include <netdb.h> #include <netdb.h>
#include <pwd.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
Context not available.
#include "pam_login_access.h" #include "pam_login_access.h"
#define _PATH_LOGACCESS "/etc/login.access"
/* Delimiters for fields and for lists of users, ttys or hosts. */
static char fs[] = ":"; /* field separator */
static char sep[] = ", \t"; /* list-element separator */
/* Constants to be used in assignments only, not in comparisons... */ /* Constants to be used in assignments only, not in comparisons... */
#define YES 1 #define YES true
#define NO 0 #define NO false
static int from_match(const char *, const char *); static int from_match(const char *, const char *, struct pam_login_access_options *);
static int list_match(char *, const char *, static int list_match(char *, const char *,
int (*)(const char *, const char *)); int (*)(const char *, const char *,
struct pam_login_access_options *),
struct pam_login_access_options *);
static int netgroup_match(const char *, const char *, const char *); static int netgroup_match(const char *, const char *, const char *);
static int string_match(const char *, const char *); static int string_match(const char *, const char *);
static int user_match(const char *, const char *); static int user_match(const char *, const char *, struct pam_login_access_options *);
static int group_match(const char *, const char *);
/* login_access - match username/group and host/tty with access control file */ /* login_access - match username/group and host/tty with access control file */
int int
login_access(const char *user, const char *from) login_access(const char *user, const char *from, struct pam_login_access_options *login_access_opts)
bjkUnsubmitted
Done
Inline Actions

It looks like this line is 99 columns?

bjk: It looks like this line is 99 columns?
{ {
FILE *fp; FILE *fp;
char line[BUFSIZ]; char line[BUFSIZ];
Context not available.
* non-existing table means no access control. * non-existing table means no access control.
*/ */
if ((fp = fopen(_PATH_LOGACCESS, "r")) != NULL) { if ((fp = fopen(login_access_opts->accessfile, "r")) != NULL) {
while (!match && fgets(line, sizeof(line), fp)) { while (!match && fgets(line, sizeof(line), fp)) {
lineno++; lineno++;
if (line[end = strlen(line) - 1] != '\n') { if (line[end = strlen(line) - 1] != '\n') {
syslog(LOG_ERR, "%s: line %d: missing newline or line too long", syslog(LOG_ERR, "%s: line %d: missing newline or line too long",
_PATH_LOGACCESS, lineno); login_access_opts->accessfile, lineno);
continue; continue;
} }
if (line[0] == '#') if (line[0] == '#')
Context not available.
line[end] = 0; /* strip trailing whitespace */ line[end] = 0; /* strip trailing whitespace */
if (line[0] == 0) /* skip blank lines */ if (line[0] == 0) /* skip blank lines */
continue; continue;
if (!(perm = strtok(line, fs)) if (!(perm = strtok(line, login_access_opts->fieldsep))
|| !(users = strtok((char *) 0, fs)) || !(users = strtok((char *) 0, login_access_opts->fieldsep))
|| !(froms = strtok((char *) 0, fs)) || !(froms = strtok((char *) 0, login_access_opts->fieldsep))
|| strtok((char *) 0, fs)) { || strtok((char *) 0, login_access_opts->fieldsep)) {
bjkUnsubmitted
Not Done
Inline Actions

Would it be worth caching these in locals to avoid repeated dereferences (and make the code easier to read)?

bjk: Would it be worth caching these in locals to avoid repeated dereferences (and make the code…
cyAuthorUnsubmitted
Done
Inline Actions

That makes sense.

cy: That makes sense.
syslog(LOG_ERR, "%s: line %d: bad field count", _PATH_LOGACCESS, syslog(LOG_ERR, "%s: line %d: bad field count", login_access_opts->accessfile,
lineno); lineno);
continue; continue;
} }
if (perm[0] != '+' && perm[0] != '-') { if (perm[0] != '+' && perm[0] != '-') {
syslog(LOG_ERR, "%s: line %d: bad first field", _PATH_LOGACCESS, syslog(LOG_ERR, "%s: line %d: bad first field", login_access_opts->accessfile,
lineno); lineno);
continue; continue;
} }
match = (list_match(froms, from, from_match) match = (list_match(froms, from, from_match, login_access_opts)
&& list_match(users, user, user_match)); && list_match(users, user, user_match, login_access_opts));
} }
(void) fclose(fp); (void) fclose(fp);
} else if (errno != ENOENT) { } else if (errno != ENOENT) {
syslog(LOG_ERR, "cannot open %s: %m", _PATH_LOGACCESS); syslog(LOG_ERR, "cannot open %s: %m", login_access_opts->accessfile);
} }
return (match == 0 || (line[0] == '+')); return (match == 0 || (line[0] == '+'));
} }
Context not available.
static int static int
list_match(char *list, const char *item, list_match(char *list, const char *item,
int (*match_fn)(const char *, const char *)) int (*match_fn)(const char *, const char *, struct pam_login_access_options *),
struct pam_login_access_options *login_access_opts)
{ {
char *tok; char *tok;
int match = NO; int match = NO;
Context not available.
* the match is affected by any exceptions. * the match is affected by any exceptions.
*/ */
for (tok = strtok(list, sep); tok != NULL; tok = strtok((char *) 0, sep)) { for (tok = strtok(list, login_access_opts->listsep); tok != NULL; tok = strtok((char *) 0, login_access_opts->listsep)) {
if (strcasecmp(tok, "EXCEPT") == 0) /* EXCEPT: give up */ if (strcmp(tok, "EXCEPT") == 0) /* EXCEPT: give up */
break; break;
bjkUnsubmitted
Not Done
Inline Actions

Thinking about strtok_r() is clearly a matter for a different change.

bjk: Thinking about strtok_r() is clearly a matter for a different change.
cyAuthorUnsubmitted
Done
Inline Actions

strtok_r() isn't needed in this module. It's only used during account management. Only one will run. Ideally this should be replaced with strsep(), but that's for another project.

cy: strtok_r() isn't needed in this module. It's only used during account management. Only one will…
if ((match = (*match_fn)(tok, item)) != 0) /* YES */ if ((match = (*match_fn)(tok, item, login_access_opts)) != 0) /* YES */
break; break;
} }
/* Process exceptions to matches. */ /* Process exceptions to matches. */
if (match != NO) { if (match != NO) {
while ((tok = strtok((char *) 0, sep)) && strcasecmp(tok, "EXCEPT")) while ((tok = strtok((char *) 0, login_access_opts->listsep)) && strcmp(tok, "EXCEPT"))
/* VOID */ ; /* VOID */ ;
if (tok == NULL || list_match((char *) 0, item, match_fn) == NO) if (tok == NULL || list_match((char *) 0, item, match_fn,
login_access_opts) == NO)
bjkUnsubmitted
Done
Inline Actions

The indentation is off here.

bjk: The indentation is off here.
cyAuthorUnsubmitted
Done
Inline Actions

Thanks. Good catch.

cy: Thanks. Good catch.
return (match); return (match);
} }
return (NO); return (NO);
Context not available.
return (NO); return (NO);
} }
/* group_match - match a group against one token */
int
group_match(const char *tok, const char *string)
{
struct group *group;
struct passwd *passwd;
gid_t *grouplist;
int i, ngroups = NGROUPS;
if ((passwd = getpwnam(string)) == NULL) {
return (NO);
}
bjkUnsubmitted
Done
Inline Actions

It feels weird to call getpwnam() with a "groupname" (vs. getgrnam()), though IIUC the second parameter here is the username we're trying to determine access for so the code is okay.
I assume this made more sense to me the first time I looked but I confused myself by waiting too long to look at the updates.

bjk: It feels weird to call getpwnam() with a "groupname" (vs. getgrnam()), though IIUC the second…
cyAuthorUnsubmitted
Done
Inline Actions

This string because we don't know if this is a user or group. In this context we consider it user so renaming string to groupname was wrong. Username is correct.

Thank you for pointing this out.

cy: This string because we don't know if this is a user or group. In this context we consider it…
errno = 0;
if ((group = getgrnam(tok)) == NULL) {
if (errno != 0) {
syslog(LOG_ERR, "getgrnam() failed for %s: %s", string, strerror(errno));
} else {
syslog(LOG_NOTICE, "group not found: %s", string);
}
return (NO);
}
if ((grouplist = calloc(ngroups, sizeof(gid_t))) == NULL) {
syslog(LOG_ERR, "cannot allocate memory for grouplist: %s", string);
bjkUnsubmitted
Done
Inline Actions

I'm not sure I understand why "string" is the right thing after "grouplist:" here; "grouplist of" would make more sense, as would "grouplist for user" or "getgrouplist"

bjk: I'm not sure I understand why "string" is the right thing after "grouplist:" here; "grouplist…
cyAuthorUnsubmitted
Done
Inline Actions

"string" came from its sister function user_match() which doesn't know if "string" is a user or a group. group_match() does. I'll change this to "groupname".

cy: "string" came from its sister function user_match() which doesn't know if "string" is a user or…
return (NO);
}
if (getgrouplist(string, passwd->pw_gid, grouplist, &ngroups) != 0) {
syslog(LOG_ERR, "getgrouplist() failed for %s", string);
free(grouplist);
return (NO);
}
for (i = 0; i < ngroups; i++) {
if (grouplist[i] == group->gr_gid) {
free(grouplist);
return (YES);
}
}
free(grouplist);
bjkUnsubmitted
Done
Inline Actions

Three instances of free(grouplist) is perhaps on the boundary of moving to a cleanup handler.

bjk: Three instances of free(grouplist) is perhaps on the boundary of moving to a cleanup handler.
return (NO);
}
/* user_match - match a username against one token */ /* user_match - match a username against one token */
static int static int
user_match(const char *tok, const char *string) user_match(const char *tok, const char *string,
struct pam_login_access_options *login_access_opts)
{ {
struct group *group; size_t stringlen;
int i; char *grpstr;
int rc;
/* /*
* If a token has the magic value "ALL" the match always succeeds. * If a token has the magic value "ALL" the match always succeeds.
bjkUnsubmitted
Not Done
Inline Actions

How long is this line?

bjk: How long is this line?
cyAuthorUnsubmitted
Done
Inline Actions

76.

cy: 76.
Context not available.
if (tok[0] == '@') { /* netgroup */ if (tok[0] == '@') { /* netgroup */
return (netgroup_match(tok + 1, (char *) 0, string)); return (netgroup_match(tok + 1, (char *) 0, string));
} else if (tok[0] == '(' && tok[(stringlen = strlen(&tok[1]))] == ')') { /* group */
bjkUnsubmitted
Not Done
Inline Actions

I'm not super-keen on this construction but can proffer no alternative.

bjk: I'm not super-keen on this construction but can proffer no alternative.
cyAuthorUnsubmitted
Done
Inline Actions

It needs to be enclosed in parentheses. The only alternative would be nested if statements. I'm not enamored with this:

} else if (tok[0] == '(') {                 /* group */
    stringlen = strlen(&tok[1];
    if (tok[stringlen] == ')') {
        if ((grpstr = strndup(&tok[1], stringlen - 1)) == NULL) {
            syslog(LOG_ERR, "cannot allocate memory for %s", string);
            return (NO);
        }
    }
cy: It needs to be enclosed in parentheses. The only alternative would be nested if statements. I'm…
if ((grpstr = strndup(&tok[1], stringlen - 1)) == NULL) {
if (errno == ENOMEM) {
bjkUnsubmitted
Done
Inline Actions

Do we need to check errno to know that strndup failed to allocate memory?

bjk: Do we need to check errno to know that strndup failed to allocate memory?
cyAuthorUnsubmitted
Done
Inline Actions

Probably not.

cy: Probably not.
syslog(LOG_ERR, "cannot allocate memory for %s", string);
}
return (NO);
}
bjkUnsubmitted
Done
Inline Actions

Shouldn't we just return the result of group_match(), which is already YES or NO?

bjk: Shouldn't we just return the result of group_match(), which is already YES or NO?
rc = group_match(grpstr, string);
free(grpstr);
return (rc);
} else if (string_match(tok, string)) { /* ALL or exact match */ } else if (string_match(tok, string)) { /* ALL or exact match */
return (YES); return (YES);
} else if ((group = getgrnam(tok)) != NULL) {/* try group membership */ } else if (login_access_opts->defgroup == true) {/* try group membership */
for (i = 0; group->gr_mem[i]; i++) return (group_match(tok, string) == YES);
if (strcasecmp(string, group->gr_mem[i]) == 0)
return (YES);
} }
return (NO); return (NO);
} }
Context not available.
/* from_match - match a host or tty against a list of tokens */ /* from_match - match a host or tty against a list of tokens */
static int static int
from_match(const char *tok, const char *string) from_match(const char *tok, const char *string,
struct pam_login_access_options *login_access_opts __unused)
{ {
int tok_len; int tok_len;
int str_len; int str_len;
Context not available.
if ((str_len = strlen(string)) > (tok_len = strlen(tok)) if ((str_len = strlen(string)) > (tok_len = strlen(tok))
&& strcasecmp(tok, string + str_len - tok_len) == 0) && strcasecmp(tok, string + str_len - tok_len) == 0)
return (YES); return (YES);
} else if (strcasecmp(tok, "LOCAL") == 0) { /* local: no dots */ } else if (strcmp(tok, "LOCAL") == 0) { /* local: no dots */
if (strchr(string, '.') == 0) if (strchr(string, '.') == 0)
bjkUnsubmitted
Done
Inline Actions

Not quite already being touched, but strchr returns a pointer not an integer.

bjk: Not quite already being touched, but strchr returns a pointer not an integer.
cyAuthorUnsubmitted
Done
Inline Actions

This is original code. I'll fix this in another commit.

strchr() returns a pointer not an int.

Reported by: bjk
Approved by: des (blanket, implicit)
MFC after: 3 days

cy: This is original code. I'll fix this in another commit. strchr() returns a pointer not an int.
return (YES); return (YES);
} else if (tok[(tok_len = strlen(tok)) - 1] == '.' /* network */ } else if (tok[(tok_len = strlen(tok)) - 1] == '.' /* network */
Context not available.
* Otherwise, return YES if the token fully matches the string. * Otherwise, return YES if the token fully matches the string.
*/ */
if (strcasecmp(tok, "ALL") == 0) { /* all: always matches */ if (strcmp(tok, "ALL") == 0) { /* all: always matches */
return (YES); return (YES);
} else if (strcasecmp(tok, string) == 0) { /* try exact match */ } else if (strcasecmp(tok, string) == 0) { /* try exact match */
return (YES); return (YES);
Context not available.