[PATCH] PAM Support for Busybox Login

Denis Vlasenko vda.linux at googlemail.com
Sun Oct 1 10:00:02 UTC 2006


On Friday 29 September 2006 23:26, Thaddeus Ternes wrote:
> Previously, a patch for Busybox login was shared on the list that
> added PAM support to 1.0.0.  That patch no longer applies to 1.2.1, so
> I did some work to fix it up, as well as make it a bit easier to build
> (thanks for the help on that, Jason).
> 
> Here's the original patch:
> http://www.busybox.net/lists/busybox/2004-December/013257.html
> 
> And attached is my swing at it (should patch cleanly against 1.2.1).
> 
> All comments appreciated.

+#define PWLOOKUP \
+               if (!( pw = getpwnam ( username ))) {  \
+                        pw_copy.pw_name   = "UNKNOWN"; \
+                        pw_copy.pw_passwd = "!";       \
+                        opt_fflag = 0;                 \
+                        failed = 1;                    \
+                } else                                 \
+                        pw_copy = *pw;                 \
+                pw = &pw_copy; \

Please do it this way:

#define PWLOOKUP do { statement; statement; } while(0)

It will behave exactly like single C statement then.

+#ifdef CONFIG_PAM

#ifdef does not detect typos in macro names. #if ENABLE_PAM is better.

+        // TODO: decide how this comment should be worded (since we have the MACRO above)
+                /* PAM authentication goes before (and instead of) all the
+                 * other stuff.  The reason we initialize the pw struct
+                 * before going on with PAM is that the pw struct is used
+                 * later on, and this way saved us some changing of that code.
+                 */
+                if ((pamret = pam_start( "login", username, &conv, &pamh )) != PAM_SUCCESS)

Generally avoid assignments inside if().
                 pamret = pam_start( "login", username, &conv, &pamh );
                 if (pamret != PAM_SUCCESS)
generates the same assembly code, but easier to read.
It is not an iron rule. In nested "else ifs", for example,
it is ok...

+                {
+                        /* pam_start() failed.  We failed to initialize the PAM
+                         * structures, and should probably carry on with login,
+                         * so we avoid locking people out.
+                         */
+                         fprintf(stderr, "PAM initialization failed: %s\n", pam_strerror(pamh, pamret));
+                                failed = 1;

1. bb_error_msg will do the same, but smaller (no stderr parameter, no \n)
2. Indentation is broken. (At least

+                         goto auth_ok;
+                }
+                else
+                {   // continuing with pam authentication

Regarding '{': usually we do
	if() {
		...
	} else {

+                    // set TTY (so things like securetty work)
+                               if((pamret = pam_set_item(pamh, PAM_TTY, tty)) != PAM_SUCCESS)
+                               {
+                                       fprintf(stderr, "Failed to pam_set_item TTY: %s\n", pam_strerror(pamh, pamret));
+                                       failed = 1;
+                                       goto auth_ok;
+                               }
+                    else if ( ( pamret = pam_authenticate( pamh, 0 ) ) == PAM_SUCCESS )
+                               {
+                               // Then check that the account is healthy.
+                               if ( ( pamret = pam_acct_mgmt( pamh, 0 ) ) != PAM_SUCCESS ) // No, it isn't
+                                       fprintf( stderr, "User not allowed access: %s\n",pam_strerror( pamh, pamret ) );
+                                       else
+                                       {       // read user back
+                                char *pamuser;
+                                               if(pam_get_item(pamh, PAM_USER, (const void **) &pamuser)!= PAM_SUCCESS)
+                                                       fprintf(stderr, "pam_get_item failed on username\n");
+                                               else
+                                    strcpy(username, pamuser);
+                                       }
+                    }
+                    // If we get here, the user was authenticated, and is
+                    // granted access.
+                    if ( pam_end( pamh, pamret ) != PAM_SUCCESS )

We typically format it like this: if (pam_end(pamh, pamret) != PAM_SUCCESS)

+                        fprintf( stderr, "PAM cleaning up failed\n" );
+
+                    if ( pamret == PAM_SUCCESS )
+                        failed = 0;
+                    else
+                        failed = 1;
+                       PWLOOKUP
+                               goto auth_ok;
+                }
+                // Everything from here to auth_ok: is skipped when running
+                // PAM.  This is all PAM's responsibility anyway.  The exception
+                // is if pam_start() failed, which would indicate a more serious
+                // error in the PAM library.  In that case, we carry on with
+                // standard procedure.
+#else
+               PWLOOKUP
+#endif /* CONFIG_PAM */

                if (( pw-> pw_passwd [0] == '!' ) || ( pw-> pw_passwd[0] == '*' ))
                        failed = 1;

Generally I like it.

Maybe will add CONFIG_DESKTOP to make such options
visible only for fat "desktop replacement"-class bbox.
So that embedded people will not feel like
bbox is going to be horribly bloated now.
--
vda



More information about the busybox mailing list