PAM kbd-int with privsep

[Niels Provos]

On Tue, Jun 25, 2002 at 11:52:55AM +1000, Damien Miller wrote:
> The following is a patch (based on FreeBSD code) which gets kbd-int
> working with privsep. It moves the kbd-int PAM conversation to a child
> process and communicates with it over a socket.
Some comments.  I am pretty tried so I might have missed something.

> +	va_start(ap, fmt);
> +	len = vsnprintf(buf, sizeof(buf), fmt, ap);
> +	va_end(ap);
> +	if (len == -1 || len > sizeof(buf))
> +		fatal("sshpam_send: message too long");
> +	mstr = xstrdup(buf);
> +	if (ctxt->pid != 0)
> +		debug2("to child: %d bytes", len);
> +	r = send(ctxt->sock, mstr, len + 1, MSG_EOR);
> +	free(mstr);
> +	return (r);
> +}
The check on the vsnprintf length is off by one.  It should be
len >= sizeof(buf):

    These functions return the number of characters printed (not
    including the trailing `\0' used to end output to strings), except for
    snprintf() and vsnprintf(), which return the number of characters that
    would have been printed if the size were unlimited (again, not
    including the final `\0').

> +	ctxt = data;
> +	if (n <= 0 || n > PAM_MAX_NUM_MSG)
> +		return (PAM_CONV_ERR);
> +	if ((*resp = calloc(n, sizeof **resp)) == NULL)
> +		return (PAM_BUF_ERR);
This code would be better if the sizeof would be on struct pam_msg or
whatever it is that resp points to.


> +
> +	debug2(__func__);
> +	switch (ctxt->done) {
> +	case 1:
> +		return (0);	
> +	case 0:
> +		break;
> +	default:
> +		return (-1);
> +	}
> +	if (num != 1) {
> +		error("expected one response, got %u", num);
> +		return (-1);
> +	}
> +	sshpam_send(ctxt, "%s", *resp);
> +	switch (sshpam_peek(ctxt)) {
> +	case 'P':
> +	case 'p':
> +		return (1);
> +	case '=':
> +		msg = sshpam_receive(ctxt);
> +		xfree(msg);
> +		ctxt->done = 1;
> +		return (0);
> +	default:
> +		msg = sshpam_receive(ctxt);
> +		if (*msg == '!')
> +			error("%s", msg + 1);
> +		xfree(msg);
> +		ctxt->done = -1;
> +		return (-1);
>  	}
This part could use some comments about what the magic values do.

> +#ifdef USE_PAM
> +    {MONITOR_REQ_PAM_INIT_CTX, 0, mm_answer_sshpam_init_ctx},
> +    {MONITOR_REQ_PAM_START, MON_ONCE, mm_answer_pam_start},
> +    {MONITOR_REQ_PAMQUERY, MON_ISAUTH, mm_answer_sshpamquery},
> +    {MONITOR_REQ_PAMRESPOND, MON_AUTH, mm_answer_sshpamrespond},
> +    {MONITOR_REQ_PAM_FREE_CTX, 0, mm_answer_sshpam_free_ctx},
> +#endif
>      {0, 0, NULL}
>  };
Th INIT and FREE_CTX could you MON_ONCE.   Right? (I do not rally no,
but it looks like it).

>  
> +void
> +mm_sshpam_free_ctx(void *ctxtp)
> +{
> +}
> +
If the child never calls free_ctx, then the monitor code for this
case is not needed.  Or this function needs to be filled in to
free the context correctly.

More eyes should look at the monitor code.

Niels.