On Mon, Jun 11, 2012 at 10:00:47PM +0200, Mats Erik Andersson wrote:
> Package: linux-ftpd
> Version: 0.17-34
> Severity: normal
>
> Performing a round of code audit of the PAM code
> found in the present source, I believe to have
> located inaccurate and missing calls.
>
> I am not in a possession of a refined enough use case
> to verify that my suggestion is correctly reacting to
> accurate accounting setups, but the new code works
> as expected with respect to standard setups.
>
> The suggested ordering between pam_setcred() and
> pam_{open/close}_session() and WTMP actions is what
> the standard manuals pledge for. The present code
> does not follow these advices, so let us evaluate
> my suggestions and then decide on appropriate measures.
Hi Mats,
I'm no expert at PAM coding, so I don't know what the correct ordering
should be.
> --- linux-ftpd-0.17.debian/ftpd/ftpd.c
> +++ linux-ftpd-0.17/ftpd/ftpd.c
> @@ -930,9 +930,6 @@ static void end_login(void)
> if (logged_in) {
> #ifdef USE_PAM
> int error;
> - error = pam_close_session(pamh, 0);
> - pam_end(pamh, error);
> - pamh = 0;
> #endif
> ftpdlogwtmp(ttyline, "", "", NULL);
> if (doutmp)
> @@ -941,6 +938,12 @@ static void end_login(void)
> if (!notickets && krbtkfile_env)
> unlink(krbtkfile_env);
> #endif
> +#ifdef USE_PAM
> + error = pam_close_session(pamh, 0);
> + (void) pam_setcred(pamh, PAM_DELETE_CRED);
> + pam_end(pamh, error);
> + pamh = 0;
> +#endif
> }
> pw = NULL;
> logged_in = 0;
Seems OK.
Taking a look at proftpd's PAM module, when closing the session the
ordering is: delete credential, close session, pam_end. We were missing
the credential deletion, but I'm not sure if that should go before or
after pam_close_session. Probably not that important.
> @@ -961,7 +964,7 @@ static int PAM_conv (int num_msg, const
>
> #define GET_MEM \
> if (!(repl = realloc(repl, size))) \
> - return PAM_CONV_ERR; \
> + return PAM_BUF_ERR; \
> size += sizeof(struct pam_response)
> #define COPY_STRING(s) (s) ? strdup(s) : NULL
This seems OK.
> @@ -1055,10 +1058,10 @@ static int pam_doit(void)
> /* Alright, we got it */
> error = pam_acct_mgmt(pamh, 0);
> if (error == PAM_SUCCESS)
> - error = pam_open_session(pamh, 0);
> - if (error == PAM_SUCCESS)
> error = pam_setcred(pamh, PAM_ESTABLISH_CRED);
> if (error == PAM_SUCCESS)
> + error = pam_open_session(pamh, 0);
> + if (error == PAM_SUCCESS)
> error = pam_get_item(pamh, PAM_USER, &vp);
> if (error == PAM_SUCCESS) {
> const char *user = vp;
In this case proftpd's PAM code follows your proposal (pam_setcred,
pam_open_session). The example at [1] does too. So, that's OK to me too.
Regards,
Alberto
[1] http://www.freebsd.org/doc/en_US.ISO8859-1/articles/pam/pam-sample-appl.html
--
Alberto Gonzalez Iniesta | Formación, consultoría y soporte técnico
agi@(inittab.org|debian.org)| en GNU/Linux y software libre
Encrypted mail preferred | http://inittab.com
Key fingerprint = 9782 04E7 2B75 405C F5E9 0C81 C514 AF8E 4BA4 01C3
--
To UNSUBSCRIBE, email to [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]
