RFC: pam_krb5: minimum_[ug]id options

[Ruslan Ermilov]

On Thu, Nov 09, 2006 at 01:18:44AM +0000, Shaun Amott wrote:
> Thanks for reviewing the patch. Here's an updated version with your
> suggestions incorporated.
> 
Please don't remove me from Cc:.  I prefer to receive directed
replies, and I didn't ask for non-directed reply via setting the
Mail-Followup-To: header like you seem to prefer.  Thanks.

Below are some more comments; it's still not being perfect...

> Index: pam_krb5.8
> ===================================================================
> RCS file: /home/ncvs/src/lib/libpam/modules/pam_krb5/pam_krb5.8,v
> retrieving revision 1.6
> diff -u -r1.6 pam_krb5.8
> --- pam_krb5.8	24 Nov 2001 23:41:32 -0000	1.6
> +++ pam_krb5.8	9 Nov 2006 01:14:18 -0000
> @@ -1,7 +1,7 @@
>  .\"
>  .\" $Id: pam_krb5.5,v 1.5 2000/01/05 00:59:56 fcusack Exp $
>  .\" $FreeBSD: src/lib/libpam/modules/pam_krb5/pam_krb5.8,v 1.6 2001/11/24 23:41:32 dd Exp $
> -.Dd January 15, 1999
> +.Dd Thursday 09, 2006
> 
It should be ".Dd November 9, 2006".

> Index: pam_krb5.c
> ===================================================================
> RCS file: /home/ncvs/src/lib/libpam/modules/pam_krb5/pam_krb5.c,v
> retrieving revision 1.23
> diff -u -r1.23 pam_krb5.c
> --- pam_krb5.c	7 Jul 2005 14:16:38 -0000	1.23
> +++ pam_krb5.c	9 Nov 2006 01:14:19 -0000
> @@ -88,6 +88,8 @@
>  #define PAM_OPT_CCACHE		"ccache"
>  #define PAM_OPT_DEBUG		"debug"
>  #define PAM_OPT_FORWARDABLE	"forwardable"
> +#define PAM_OPT_MINIMUM_GID	"minimum_gid"
> +#define PAM_OPT_MINIMUM_UID	"minimum_uid"
>  #define PAM_OPT_NO_CCACHE	"no_ccache"
>  #define PAM_OPT_REUSE_CCACHE	"reuse_ccache"
>  
> @@ -110,6 +112,9 @@
>  	const char *user, *pass;
>  	const void *sourceuser, *service;
>  	char *principal, *princ_name, *ccache_name, luser[32], *srvdup;
> +	const char *retstr;
> +	uid_t minuid = 0;
> +	gid_t mingid = 0;

Initializations can be done later, please see below.
>  
>  	retval = pam_get_user(pamh, &user, USER_PROMPT);
>  	if (retval != PAM_SUCCESS)
> @@ -222,6 +227,39 @@
>  
>  	PAM_LOG("Done getpwnam()");
>  
> +	retstr = openpam_get_option(pamh, PAM_OPT_MINIMUM_UID);
> +	if (retstr != NULL) {
> +		if ((minuid = (uid_t)strtoul(retstr, NULL, 10)) == 0) {
> +			if (errno == ERANGE || errno == EINVAL) {
> 
Checking for ERANGE here is pointless, as when it's set, the
return value will be ULONG_MAX and not zero.

> +				PAM_LOG("Error in minimum_uid: %s",
> +				    strerror(errno));
> +				return (PAM_SERVICE_ERR);
> +			}
> +		} else if (minuid > UID_MAX) {

Err, you should be range checking an uncasted "unsigned long"
value against UID_MAX because by casting it to (uid_t) this
condition is always false.  On 32-bit platforms where "long"
is 4 bytes it's moot anyway, but on 64-bit platforms with
8-byte longs it will make a difference.  I think a correct
code would look something like this (assuming it's guaranteed
that sizeof(uid_t) <= sizeof(long) ;-):

: 	unsigned long val;
: 
: 	val = strtoul(retstr, NULL, 10);
: 	if ((val == ULONG_MAX && errno == ERANGE) ||
: 	    (val == 0 && errno == EINVAL))
: 		/* error1 */
: 	else if (val > UID_MAX)
: 		/* error2 */
: 	else
: 		uid = (uid_t)val;

> +			PAM_LOG("Error in minimum_uid: invalid UID");
> +			return (PAM_SERVICE_ERR);
> +		}
> +	}

It probably makes sense to initialize "minuid = 0" only here (in
the "else" clause), rather than doing it in the declaration part.

> +
> +	retstr = openpam_get_option(pamh, PAM_OPT_MINIMUM_GID);
> +	if (retstr != NULL) {
> +		if ((mingid = (gid_t)strtoul(retstr, NULL, 10)) == 0) {
> +			if (errno == ERANGE || errno == EINVAL) {
> +				PAM_LOG("Error in minimum_gid: %s",
> +				    strerror(errno));
> +				return (PAM_SERVICE_ERR);
> +			}
> +		} else if (mingid > GID_MAX) {
> +			PAM_LOG("Error in minimum_gid: invalid GID");
> +			return (PAM_SERVICE_ERR);
> +		}
> +	}
> +
> +	if (pwd->pw_uid < minuid || pwd->pw_gid < mingid)
> +		return (PAM_IGNORE);

Ditto for the GID code.


Cheers,
-- 
Ruslan Ermilov
ru at FreeBSD.org
FreeBSD committer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20061109/9c9f668d/attachment.pgp