patch for busybox-1.4.2 PAM support

[Anselmo Lacerda S. de Melo]

Hi!

Thanks for you comments! As I've said, this was based in another patch,
so there are some parts that I didn't change anything. I agree that I
didn't take care about some of them and them really need to be changed
for a newer version.

Thanks again,
Anselmo


Denis Vlasenko escreveu:
> 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
> 
>