patch for busybox-1.4.2 PAM support

[Denis Vlasenko]

On Monday 28 May 2007 18:28, Anselmo Lacerda S. de Melo wrote:
> Hi folks!
> 
> As I need PAM suport on busybox, I made a patch for busybox-1.4.2. It
> was based on  patch for busybox-1.2.1 that I found searching in the
> archives of this list.
> 
> All comments appreciated.

Patch against current svn is preferred.

+static struct pam_conv conv = {
+        misc_conv,
+        NULL
+};
+#endif

This struct can/should be const.


+#if ENABLE_PAM
+               pamret = PAM_SUCCESS;
+#endif
...
+#if ENABLE_PAM
+        pamret = pam_start( "login", username, &conv, &pamh );

What's the point of the first assignment?


+        else {
+            // continuing with pam authentication
+            // set TTY (so things like securetty work)
+            if((pamret = pam_set_item(pamh, PAM_TTY, short_tty)) != PAM_SUCCESS) {
+                bb_error_msg("Failed to pam_set_item TTY: %s", pam_strerror(pamh, pamret));
+                goto auth_failed;
+            }

Please use tabs for indent; please do not place assignments in if()s.


+        // Everything from here to auth_ok: is skipped when running
+        // PAM.  This is all PAM's responsibility anyway.
+#else
+               PWLOOKUP;
+#endif /* ENABLE_PAM */

If you will properly enclose non-PAM code in bigger #else block,
you will make life easier for people to read and for gcc to optimize
(it will notice that static check_securetty() is not needed).


+                        strcpy(username, pamuser);

Unsafe versus buffer overflow!


+ifeq ($(CONFIG_PAM),y)
+    LDFLAGS += -lpam -lpam_misc
+endif

Does not compile for me, need LDLIBS instead.


See attached updated patch against current svn.

It still has some grey areas:

+               if (pamret != PAM_SUCCESS) {
+                       bb_error_msg("pam_set_item(TTY) failed: %s", pam_strerror(pamh, pamret));
+                       goto auth_failed;

ok here.

+               }
+               pamret = pam_authenticate(pamh, 0);
+               if (pamret == PAM_SUCCESS) {
+                       // Then check that the account is healthy.
+                       pamret = pam_acct_mgmt(pamh, 0);
+                       if (pamret != PAM_SUCCESS) // No, it isn't
+                               bb_error_msg("user not allowed access: %s", pam_strerror(pamh, pamret));

shouldn't you goto auth_failed here?

+                       else {
+                               // read user back
+                               char *pamuser;
+                               /* gcc: "dereferencing type-punned pointer breaks aliasing rules..."
+                                * thus we use double cast */
+                               if (pam_get_item(pamh, PAM_USER, (const void **) (void*) &pamuser) != PAM_SUCCESS)
+                                       bb_error_msg("pam_get_item(USER) failed: %s", pam_strerror(pamh, pamret));

and here?

+                               else
+                                       safe_strncpy(username, pamuser, sizeof(username));
+                       }
+               }


+               if (pam_end(pamh, pamret) != PAM_SUCCESS)
+                       bb_error_msg("pam_end failed");

You do it only on success.
Do you need this on pam failure too? (If not, say so in comment).
Conversely, is it needed at all?
--
vda

-------------- next part --------------
A non-text attachment was scrubbed...
Name: login_pam.patch
Type: text/x-diff
Size: 5478 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20070529/73919b62/attachment-0002.bin