On Mon, 02 Dec 2013 12:36:38 -0700
Orion Poplawski <orion@xxxxxxxxxxxxx> wrote:
> On 11/30/2013 03:32 AM, Jeff Layton wrote:
> > On Wed, 13 Nov 2013 13:59:33 -0700
> > Orion Poplawski <orion@xxxxxxxxxxxxx> wrote:
> >
> >> From e2e6e73f05d912377656675292994858b99cabbb Mon Sep 17 00:00:00 2001
> >> From: Orion Poplawski <orion@xxxxxxxx>
> >> Date: Wed, 13 Nov 2013 13:53:30 -0700
> >> Subject: [PATCH] Create cifscreds PAM module to insert credentials at login
> >>
> >
> > Sorry for taking so long to review this. This looks like a good start.
> > FWIW, I'd like to see this merged for the next release, which should
> > happen in the next month or two.
> >
>
> No problem, thanks for the review.
>
> > When you think this is ready for merge, please resend with a real
> > '[PATCH]' tag in the subject, so I don't miss it...
>
> Hopefully this is okay.
>
> >> @@ -231,6 +236,7 @@ AM_CONDITIONAL(CONFIG_CIFSUPCALL, [test "$enable_cifsupcall" != "no"])
> >> AM_CONDITIONAL(CONFIG_CIFSCREDS, [test "$enable_cifscreds" != "no"])
> >> AM_CONDITIONAL(CONFIG_CIFSIDMAP, [test "$enable_cifsidmap" != "no"])
> >> AM_CONDITIONAL(CONFIG_CIFSACL, [test "$enable_cifsacl" != "no"])
> >> +AM_CONDITIONAL(CONFIG_PAM, [test "$enable_pam" != "no" -o "$enable_pam" != "no"])
> >
> > Erm...you're testing the same thing twice here?
>
> Oops, copy/paste error.
>
> >
> > I think the autoconf stuff needs some work.
> >
> > If the box doesn't have what's needed to build it, the build is going
> > to fail unless someone explicitly disables CONFIG_PAM.
> >
> > Ideally, we'd build this by default and have autoconf test for what's
> > required to build the PAM module. If the build requires aren't present,
> > then the building of the PAM module should be disabled with a warning
> > message that states that.
> >
> > There are some other examples of that sort of behavior in configure.ac.
> > It's a little more complicated, but it makes life easier for people
> > building the package.
>
> Okay, I've added a section for checking for security/pam_appl.h. Should we
> also check for the other headers/libraries as well?
>
> I've also added to the keyutils.h section so as to disable the pam module
> there too.
>
>
> >> +static void
> >> +free_password (char *password)
> >> +{
> >> + volatile char *vp;
> >> + size_t len;
> >> +
> >> + if (!password) {
> >> + return;
> >> + }
> >> +
> >> + /* Defeats some optimizations */
> >> + len = strlen (password);
> >> + memset (password, 0xAA, len);
> >> + memset (password, 0xBB, len);
> >> +
> >> + /* Defeats others */
> >> + vp = (volatile char*)password;
> >> + while (*vp) {
> >> + *(vp++) = 0xAA;
> >> + }
> >> +
> >
> > I'm all for scrubbing the pw memory but that seems a bit like overkill.
> > Got any references to cite about why you need to write over it 3 times?
> >
> > If that's really necessary, then we should move the password handling
> > in cifscreds and mount.cifs binary to use something like this too.
> > Maybe consider putting this in an a.out lib and linking it into all 3?
>
> This is copied directly from the gnome-keyring pam module. I don't have any
> references for why it is done. My guess is because it is part of a PAM chain,
> so perhaps to prevent later modules from accessing it? Or perhaps just to
> scrub memory? I suspect that the multiple times is to defeat compiler
> optimization making it a no-op?
>
> >> +
> >> +/**
> >> + * This is called when the PAM session is closed.
> >> + *
> >> + * Currently it does nothing. The session closing should remove the passwords
> >> + *
> >> + * @param ph PAM handle
> >> + * @param flags currently unused, TODO: check for silent flag
> >> + * @param argc number of arguments for this PAM module
> >> + * @param argv array of arguments for this PAM module
> >> + * @return PAM_SUCCESS
> >> + */
> >> +PAM_EXTERN int pam_sm_close_session(pam_handle_t *ph, int flags, int argc, const char **argv)
> >> +{
> >> + return PAM_SUCCESS;
> >> +}
> >> +
> >
> > Cleaning up after you're done seems like a good thing to do. The thing
> > we need to be careful about here is that we might still need these
> > creds to write out dirty data. This may require some consideration...
>
> Yeah, I'm not sure what there is to do here really. Anything would be more
> than what is done currently running cifscreds directly.
>
> >> +/**
> >> + * This is called when PAM is invoked to change the user's authentication token.
> >> + * TODO - update the credetials
> >> + *
> >> + * @param ph PAM handle
> >> + * @param flags currently unused, TODO: check for silent flag
> >> + * @param argc number of arguments for this PAM module
> >> + * @param argv array of arguments for this PAM module
> >> + * @return PAM_SUCCESS
> >> + */
> >> + PAM_EXTERN int pam_sm_setcred(pam_handle_t *ph, int flags, int argc, const char **argv)
> >> +{
> >> + return PAM_SUCCESS;
> >> +}
> >
> > The rest looks reasonably straightforward, but I don't PAM that well,
> > so it's hard for me to comment.
> >
>
> Actually, I'm not sure what should be done here. gnome-keyring doesn't do
> anything. http://pubs.opengroup.org/onlinepubs/008329799/pam_sm_setcred.htm
> describes the call.
>
> Looks like the pam_sm_chauthtok function is used to update the passwords.
> I've made attempts to implement that.
>
Ok, I've got it merged into my staging repo, and will plan to do some
testing with it in the next few days. At this point, I think the main
thing the patch is missing is a manpage. Do you mind writing one up for
it?
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
