Skip site navigation (1)Skip section navigation (2)


| raw e-mail | index | archive | help
>>> +	}
>>> +	va_start(ap, realm);

Also, this seems quite sketchy if realm was NULL.  You are starting the
va_list with the value of temp_realm which is not an on-stack argument?

>>> +	/*
>>> +	 * XXX Ideally we should be using krb5_build_principal_alloc_va()
>>> +	 * XXX here because krb5_build_principal_va() is deprecated. But,
>>> +	 * XXX this would require changes elsewhere in the calling code
>>> +	 * XXX to call krb5_free_principal() elsewhere to free the
>>> +	 * XXX principal. We can do that after Heimdal is removed from
>>> +	 * XXX our tree.
>>> +	 */
>>> +	rc = krb5_build_principal_va(context, principal, strlen(realm), realm,
>> ap);
>>> +	va_end(ap);

You need to wait to free temp_realm until here?

>>> +	return (rc);
>>> +}
>>> +#endif
>>> +
>>>    PAM_EXTERN int
>>>    pam_sm_authenticate(pam_handle_t *pamh, int flags __unused,
>>>        int argc __unused, const char *argv[] __unused)
>>> @@ -217,7 +272,13 @@ get_su_principal(krb5_context context, const char *tar
>> get_user, const char *curr
>>>    	if (rv != 0)
>>>    		return (errno);
>>>    	if (default_principal == NULL) {
>>> +#ifdef MK_MITKRB5
>>> +		/* For MIT KRB5. */
>>> +		rv = krb5_make_principal(context, default_principal, NULL, curr
>> ent_user, NULL);
>>> +#else
>>> +		/* For Heimdal. */
>>>    		rv = krb5_make_principal(context, &default_principal, NULL, cur
>> rent_user, NULL);
>>> +#endif
>>
>> At this point default_principal is always NULL, so you pass in a NULL pointer
>>   to
>> krb5_build_princpal_va.  That will surely crash.
> 
> In Heimdal the principal argument is defined as
> 
> 	krb5_principal *principal
> 
> Whereas in MIT the principal argument is
> 
> 	krb5_principal princ
> 
> In Heimdal krb5_principal is a structure. In MIT it is a pointer to the
> same structure.
> 
> build_principal() is defined as a struct in Heimdal and a pointer to a
> struct in MIT. Therefore the function prototypes are different.

This doesn't make sense.  The code assumes it is a pointer:

	if (default_principal == NULL) {
#ifdef MK_MITKRB5
		/* For MIT KRB5. */
		rv = krb5_make_principal(context, default_principal, NULL, current_user, NULL);
#else

How are we comparing a structure to NULL?

>>
>> Also, you then pass that NULL pointer to the following code which would also
>> surely crash:
>>
>> 	/* Now that we have some principal, if the target account is
>> 	 * `root', then transform it into a `root' instance, e.g.
>> 	 * `user@REA.LM' -> `user/root@REA.LM'.
>> 	 */
>> 	rv = krb5_unparse_name(context, default_principal, &principal_name);
>> 	krb5_free_principal(context, default_principal);
>>
>> This is why I said your comment seems wrong.  The Heimdal version is clearly
>> allocating
>> a principal, so the MIT version should also be doing that, and you should alr
>> eady be
>> using krb5_build_prinpcial_alloc_va() _now_.  Your comment claims that the ca
>> lling code
>> isn't using krb5_free_principal(), but the calling code quoted above in get_s
>> u_principal()
>> does call krb5_free_principal().
>>
>> Have you tested this at runtime?
> 
> Yes. It is running here.
> 
> slippy$ which ksu
> /usr/bin/ksu
> slippy$ /usr/bin/ksu
> Authenticated cy@CWSENT.COM
> Account root: authorization for cy@CWSENT.COM successful
> Changing uid to root (0)
> slippy$ id
> uid=0(root) gid=0(wheel) groups=0(wheel),5(operator),920(vboxusers)
> slippy$

Did you test without a valid credential cache to exercise the default_principal == NULL
path?

-- 
John Baldwin




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?>