Revisiting krb5_gic_opt_ext

[Jeffrey Hutzelman]

--On Wednesday, November 25, 2009 02:40:56 PM -0500 ghudson at mit.edu wrote:

> Warning: grotty stuff ahead.
>
> The background: the krb5_get_init_creds system was originally
> specified using a caller-allocated options structure
> (krb5_get_init_creds_opt), which meant options fields couldn't be
> added without breaking the ABI.
>
> To address this, krb5_gic_opt_ext was created.  This is a
> library-allocated structure which is ABI-compatible with
> krb5_get_init_creds_opt (sort of; see below).  If you're an API which
> takes an options argument, you call krb5int_gic_opt_to_opte on it,
> with the "force" flag set.
>
> opt_to_opte has an "interesting" contract: if the input is already an
> extended structure, it creates an alias; otherwise, it creates a copy
> and sets a "shadowed" flag.  The caller is supposed to check the
> shadowed flag and free the structure when it is done.
>
> The problem: this contract is not nestable.  If the input of
> opt_to_opte is a copy created by a previous call to opt_to_opte, then
> the output will be an alias which looks like a copy, and that will
> result in a double-free.
>
> We haven't run into this problem yet because we've never had a reason
> to pass an opt_to_opte'd options structure to an external API.  But
> I'm preparing to integrate some IAKERB support code which does;
> specifically, the synchronous wrapper function krb5_get_init_creds
> needs to supply options to krb5_init_creds_init.
>
> Possible solutions:
>
> 1. Make opt_to_opte copy the structure if the input is shadowed and
> the force flag is set.  This solves the immediate problem and creates
> no extra work if the original caller used the new API.
>
> 2. Make opt_to_opte copy the structure all the time.  This is extra
> work in the common case, but makes the code easier to analyze.
> ("Maybe this is allocated and maybe this is an alias" is not a very
> safe construction.)
>
> 3. Eliminate the copies.  Instead, when peering at new options fields,
> use accessors which can deal with either an extended or non-extended
> options structure.  This would touch the most code, but would have the
> same benefits of (2) without the extra allocation-and-copy work.  This
> would probably entail changing internal interfaces like
> krb5_get_init_creds to use pointers to krb5_get_init_creds_opt instead
> of krb5_gic_opt_ext.


4. Make opt_to_opte copy the structure only when the input is non-extended, 
and add a refcount to the extended version of the structure.  Creating an 
alias increments the refcount, and callers that currently free when the 
"shadowed" flag is set should instead call a put operation that decs the 
refcount and frees when appropriate.