Date: Thu, 9 Nov 2006 14:36:19 +0300 From: Ruslan Ermilov <ru@FreeBSD.org> To: freebsd-hackers@FreeBSD.org Subject: Re: RFC: pam_krb5: minimum_[ug]id options Message-ID: <20061109113619.GB53554@rambler-co.ru> In-Reply-To: <20061109011843.GA4880@charon.picobyte.net> References: <20061108212829.GA2738@charon.picobyte.net> <20061108221018.GB55351@rambler-co.ru> <20061109011843.GA4880@charon.picobyte.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--5G06lTa6Jq83wMTw
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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.
>=20
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
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> 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/2=
4 23:41:32 dd Exp $
> -.Dd January 15, 1999
> +.Dd Thursday 09, 2006
>=20
It should be ".Dd November 9, 2006".
> Index: pam_krb5.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> 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"
> =20
> @@ -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 =3D 0;
> + gid_t mingid =3D 0;
Initializations can be done later, please see below.
> =20
> retval =3D pam_get_user(pamh, &user, USER_PROMPT);
> if (retval !=3D PAM_SUCCESS)
> @@ -222,6 +227,39 @@
> =20
> PAM_LOG("Done getpwnam()");
> =20
> + retstr =3D openpam_get_option(pamh, PAM_OPT_MINIMUM_UID);
> + if (retstr !=3D NULL) {
> + if ((minuid =3D (uid_t)strtoul(retstr, NULL, 10)) =3D=3D 0) {
> + if (errno =3D=3D ERANGE || errno =3D=3D EINVAL) {
>=20
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) <=3D sizeof(long) ;-):
: unsigned long val;
:=20
: val =3D strtoul(retstr, NULL, 10);
: if ((val =3D=3D ULONG_MAX && errno =3D=3D ERANGE) ||
: (val =3D=3D 0 && errno =3D=3D EINVAL))
: /* error1 */
: else if (val > UID_MAX)
: /* error2 */
: else
: uid =3D (uid_t)val;
> + PAM_LOG("Error in minimum_uid: invalid UID");
> + return (PAM_SERVICE_ERR);
> + }
> + }
It probably makes sense to initialize "minuid =3D 0" only here (in
the "else" clause), rather than doing it in the declaration part.
> +
> + retstr =3D openpam_get_option(pamh, PAM_OPT_MINIMUM_GID);
> + if (retstr !=3D NULL) {
> + if ((mingid =3D (gid_t)strtoul(retstr, NULL, 10)) =3D=3D 0) {
> + if (errno =3D=3D ERANGE || errno =3D=3D 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,
--=20
Ruslan Ermilov
ru@FreeBSD.org
FreeBSD committer
--5G06lTa6Jq83wMTw
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (FreeBSD)
iD8DBQFFUxKzqRfpzJluFF4RAjbnAJsHXOGInGsksXJaKi/fa/QWRiHrugCgh9O2
R3yF3Cw0nn3A32kulpgpT8E=
=/ZjC
-----END PGP SIGNATURE-----
--5G06lTa6Jq83wMTw--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061109113619.GB53554>