On Sunday 01 February 2009 20:47, walter harms wrote:
> Denys Vlasenko schrieb:
> > On Saturday 31 January 2009 18:37, walter harms wrote:
> >> Hi Denis,
> >> i have broken down my patch to login.c into several pieces (4).
> >> Shall i send the diff or the .c ?
> >
> > Please send diffs
>
> attached all patches should apply on top of each other. The patches are based
> on the login.c i send to the list end of last year.
>
> v0-v1: basicly moves SELINUX and LOGIN_SCRIPTS out of the way
-#if ENABLE_SELINUX
- if (is_selinux_enabled()) {
- security_context_t old_tty_sid, new_tty_sid;
+ USE_SELINUX(initselinux(username, full_tty, &user_sid));
- if (get_default_context(username, NULL, &user_sid)) {
but then
+static void initselinux(char *username, char *full_tty,
+ security_context_t * user_sid)
+{
+ security_context_t old_tty_sid, new_tty_sid;
+
+ if (get_default_context(username, NULL, user_sid)) {
+ bb_error_msg_and_die("cannot get SID for %s", username);
+ }
You lost "if (is_selinux_enabled()) ..." part
> v1-v2: rework the get ttynam() part, move from array to pointer
+ full_tty = ttyname(STDIN_FILENO);
+ if (!full_tty || strncmp(full_tty, "/dev/", 5)) {
+ full_tty = (char *) "UNKNOWN";
+ short_tty = full_tty;
+ } else {
+ short_tty = full_tty + 5;
If ttyname does not start with "/dev/", you set it to "UNKNOWN".
This is wrong.
> v2-v3: move pswd check for PAM and !PAM into check_pswd()
> - fix getpwnam_r buffer size
+static
+struct passwd *bb_getpwnam_r(char *username)
+{
+ struct passwd *pwdstruct,*ptr;
+ struct passwd *pwd1;
+ size_t len = 1024;
+ long int initlen = sysconf(_SC_GETPW_R_SIZE_MAX);
+
+ if (initlen > 0)
+ len = (size_t) initlen;
what if sysconf() return a value which doesn't fit in size_t?
+
+ /*
+ pwdstruct is a list of pointers to
no, it isn't
+ data stored in pwdbuf
stored where?
+ */
+
+ pwdstruct= xmalloc(len+sizeof(*pwdstruct));
+ ptr=pwdstruct+len;
Bug. pwdstruct + n moves ptr n * sizeof(struct passwd) bytes!
+ /*
+ pwd1 will become NULL on not found
+ getpwnam_r will become 0 on eror
getpwnam does not "become", it "returns".
+/*
+ this is a special case
+*/
+ if (!pamuser || !pamuser[0])
+ goto auth_failed;
Does this comment actually explain something? If it says
'NULL or "" username is considered a failure', well,
that's obvious from the code.
if (option_mask32 & LOGIN_OPT_f)
...
...
...
opt &= ~LOGIN_OPT_f;
You forgot to copy updated flags to option_mask32.
I think check_pswd() can be better implemented as:
struct passwd *check_pswd(const char *username)
instead of
static int check_pswd(char *username, int userlen, struct passwd **pw2)
When these issues are addressed:
function old new delta
login_main 1552 1584 +32
:(
I applied patches 1 and 2, thanks! Please look at attached patch.
It's a cleaned up version of patch 3. I hesitate to apply it
to svn, there might be more unexploded ordnance there.
If it would be at least not bigger with the patch according
to "make bloatcheck", then maybe...
--
vda
diff -d -urpN busybox.4/loginutils/login.c busybox.5/loginutils/login.c
--- busybox.4/loginutils/login.c 2009-02-02 00:13:37.000000000 +0100
+++ busybox.5/loginutils/login.c 2009-02-02 01:03:16.000000000 +0100
@@ -31,7 +31,12 @@ enum {
TIMEOUT = 60,
EMPTY_USERNAME_COUNT = 10,
USERNAME_SIZE = 32,
- TTYNAME_SIZE = 32,
+};
+
+enum {
+ LOGIN_OPT_f = (1 << 0),
+ LOGIN_OPT_h = (1 << 1),
+ LOGIN_OPT_p = (1 << 2),
};
static char* short_tty;
@@ -267,14 +272,129 @@ static void alarm_handler(int sig UNUSED
_exit(EXIT_SUCCESS);
}
+#if ENABLE_PAM
+static struct passwd *bb_getpwnam_r(const char *username)
+{
+ struct passwd *pw, *result;
+ int buflen;
+ long sclen;
+
+ buflen = 1024;
+ sclen = sysconf(_SC_GETPW_R_SIZE_MAX);
+ if ((unsigned long)sclen < 999999)
+ buflen = sclen;
+
+ pw = xmalloc(sizeof(*pw) + buflen);
+ result = NULL;
+ getpwnam_r(username, pw, (char*)(pw + 1), buflen, &result);
+ if (result != NULL)
+ return result;
+ free(pw);
+ return NULL;
+}
+
+static struct passwd *check_pswd(const char *username)
+{
+ int pamret;
+ pam_handle_t *pamh;
+ const char *pamuser;
+ const char *failed_msg;
+ struct passwd *pw = NULL;
+
+ pamret = pam_start("login", username, &conv, &pamh);
+ if (pamret != PAM_SUCCESS) {
+ failed_msg = "start";
+ goto pam_auth_failed;
+ }
+ /* set TTY (so things like securetty work) */
+ pamret = pam_set_item(pamh, PAM_TTY, short_tty);
+ if (pamret != PAM_SUCCESS) {
+ failed_msg = "set_item(TTY)";
+ goto pam_auth_failed;
+ }
+ pamret = pam_authenticate(pamh, 0);
+ if (pamret != PAM_SUCCESS) {
+ failed_msg = "authenticate";
+ goto pam_auth_failed;
+ /* TODO: or just "goto auth_failed"
+ * since user seems to enter wrong password
+ * (in this case pamret == 7)
+ */
+ }
+ /* check that the account is healthy */
+ pamret = pam_acct_mgmt(pamh, 0);
+ if (pamret != PAM_SUCCESS) {
+ failed_msg = "acct_mgmt";
+ goto pam_auth_failed;
+ }
+ /* read user back */
+ pamuser = NULL;
+ /* gcc: "dereferencing type-punned pointer breaks aliasing rules..."
+ * thus we cast to (void*) */
+ if (pam_get_item(pamh, PAM_USER, (void *) &pamuser) != PAM_SUCCESS) {
+ failed_msg = "get_item(USER)";
+ goto pam_auth_failed;
+ }
+ if (!pamuser || !pamuser[0])
+ goto auth_failed;
+
+ /* Don't use "pw = getpwnam(username);",
+ * PAM is said to be capable of destroying static storage
+ * used by getpwnam(). We are using safe(r) function */
+ pw = bb_getpwnam_r(username);
+ if (!pw)
+ goto auth_failed;
+
+ pamret = pam_open_session(pamh, 0);
+ if (pamret != PAM_SUCCESS) {
+ failed_msg = "open_session";
+ goto pam_auth_failed;
+ }
+ pamret = pam_setcred(pamh, PAM_ESTABLISH_CRED);
+ if (pamret != PAM_SUCCESS) {
+ failed_msg = "setcred";
+ goto pam_auth_failed;
+ }
+ return pw; /* success, continue login process */
+
+ pam_auth_failed:
+ bb_error_msg("pam_%s call failed: %s (%d)", failed_msg,
+ pam_strerror(pamh, pamret), pamret);
+ auth_failed:
+ free(pw);
+ return NULL;
+}
+#else /* !ENABLE_PAM */
+static struct passwd *check_pswd(const char *username)
+{
+ struct passwd *pw;
+
+ pw = getpwnam(username); /* replace with _r ?? */
+ if (!pw)
+ goto fake_it;
+
+ if (pw->pw_passwd[0] == '!' || pw->pw_passwd[0] == '*')
+ goto auth_failed;
+ if (option_mask32 & LOGIN_OPT_f)
+ return pw; /* -f USER: success without asking passwd */
+ if (pw->pw_uid == 0 && !check_securetty())
+ goto auth_failed;
+ /* Don't check the password if password entry is empty (!) */
+ if (!pw->pw_passwd[0])
+ return pw; /* success */
+ fake_it:
+ /* Authorization takes place here */
+ if (correct_password(pw))
+ return pw;
+ auth_failed:
+// free(pw);
+ return NULL;
+}
+#endif
+
int login_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
int login_main(int argc UNUSED_PARAM, char **argv)
{
- enum {
- LOGIN_OPT_f = (1<<0),
- LOGIN_OPT_h = (1<<1),
- LOGIN_OPT_p = (1<<2),
- };
char *fromhost;
char username[USERNAME_SIZE];
const char *tmp;
@@ -287,14 +407,6 @@ int login_main(int argc UNUSED_PARAM, ch
char *full_tty;
USE_SELINUX(security_context_t user_sid = NULL;)
USE_FEATURE_UTMP(struct utmp utent;)
-#if ENABLE_PAM
- int pamret;
- pam_handle_t *pamh;
- const char *pamuser;
- const char *failed_msg;
- struct passwd pwdstruct;
- char pwdbuf[256];
-#endif
username[0] = '\0';
signal(SIGALRM, alarm_handler);
@@ -351,96 +463,17 @@ int login_main(int argc UNUSED_PARAM, ch
if (!username[0])
get_username_or_die(username, sizeof(username));
-#if ENABLE_PAM
- pamret = pam_start("login", username, &conv, &pamh);
- if (pamret != PAM_SUCCESS) {
- failed_msg = "start";
- goto pam_auth_failed;
- }
- /* set TTY (so things like securetty work) */
- pamret = pam_set_item(pamh, PAM_TTY, short_tty);
- if (pamret != PAM_SUCCESS) {
- failed_msg = "set_item(TTY)";
- goto pam_auth_failed;
- }
- pamret = pam_authenticate(pamh, 0);
- if (pamret != PAM_SUCCESS) {
- failed_msg = "authenticate";
- goto pam_auth_failed;
- /* TODO: or just "goto auth_failed"
- * since user seems to enter wrong password
- * (in this case pamret == 7)
- */
- }
- /* check that the account is healthy */
- pamret = pam_acct_mgmt(pamh, 0);
- if (pamret != PAM_SUCCESS) {
- failed_msg = "acct_mgmt";
- goto pam_auth_failed;
- }
- /* read user back */
- pamuser = NULL;
- /* gcc: "dereferencing type-punned pointer breaks aliasing rules..."
- * thus we cast to (void*) */
- if (pam_get_item(pamh, PAM_USER, (void*)&pamuser) != PAM_SUCCESS) {
- failed_msg = "get_item(USER)";
- goto pam_auth_failed;
- }
- if (!pamuser || !pamuser[0])
- goto auth_failed;
- safe_strncpy(username, pamuser, sizeof(username));
- /* Don't use "pw = getpwnam(username);",
- * PAM is said to be capable of destroying static storage
- * used by getpwnam(). We are using safe(r) function */
- pw = NULL;
- getpwnam_r(username, &pwdstruct, pwdbuf, sizeof(pwdbuf), &pw);
- if (!pw)
- goto auth_failed;
- pamret = pam_open_session(pamh, 0);
- if (pamret != PAM_SUCCESS) {
- failed_msg = "open_session";
- goto pam_auth_failed;
- }
- pamret = pam_setcred(pamh, PAM_ESTABLISH_CRED);
- if (pamret != PAM_SUCCESS) {
- failed_msg = "setcred";
- goto pam_auth_failed;
- }
- break; /* success, continue login process */
-
- pam_auth_failed:
- bb_error_msg("pam_%s call failed: %s (%d)", failed_msg,
- pam_strerror(pamh, pamret), pamret);
- safe_strncpy(username, "UNKNOWN", sizeof(username));
-#else /* not PAM */
- pw = getpwnam(username);
- if (!pw) {
- strcpy(username, "UNKNOWN");
- goto fake_it;
- }
-
- if (pw->pw_passwd[0] == '!' || pw->pw_passwd[0] == '*')
- goto auth_failed;
-
- if (opt & LOGIN_OPT_f)
- break; /* -f USER: success without asking passwd */
-
- if (pw->pw_uid == 0 && !check_securetty())
- goto auth_failed;
-
- /* Don't check the password if password entry is empty (!) */
- if (!pw->pw_passwd[0])
- break;
- fake_it:
- /* authorization takes place here */
- if (correct_password(pw))
+ pw = check_pswd(username);
+ if (pw)
break;
-#endif /* ENABLE_PAM */
- auth_failed:
+ safe_strncpy(username, "UNKNOWN", userlen);
+
opt &= ~LOGIN_OPT_f;
+ option_mask32 = opt;
bb_do_delay(FAIL_DELAY);
/* TODO: doesn't sound like correct English phrase to me */
puts("Login incorrect");
+//FIXME: PAM may have it own ideas how often we can try
if (++count == 3) {
syslog(LOG_WARNING, "invalid password for '%s'%s",
username, fromhost);
@@ -450,6 +483,8 @@ int login_main(int argc UNUSED_PARAM, ch
} /* while (1) */
alarm(0);
+ safe_strncpy(username, pw->pw_name, sizeof(username));
+
/* We can ignore /etc/nologin if we are logging in as root,
* it doesn't matter whether we are run by root or not */
if (pw->pw_uid != 0)
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
