Questions about iter check in profile_iterator()

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Questions about iter check in profile_iterator()

Neng Xue
Hi MIT krb team,

Recently I am working on a kerberos related project in Solaris which
will upgrade our krb version to 1.13. We have a set of solaris specific
profile interfaces underneath using MIT kerberos profile interfaces.
They used to work fine. However, after the version upgrade I noticed the
interface wrapper encountered an issue. I narrowed down the issue to:

*prof_get.c:profile_iterator()*:

582 if (iter->magic != PROF_MAGIC_ITERATOR)
583        return PROF_MAGIC_ITERATOR;

When the iter is NULL our program will segmentation fault because of the
null pointer deference.

Here is piece of our code:

       code = *profile_iterator_create*(profile, hierarchy,
42        PROFILE_ITER_LIST_SECTION, *&state*);
43    while (code == 0) {
44        code = *profile_iterator*(&*state*, &name, &value);
45        if (code == 0 && name != NULL) {
46
47            if (key != NULL && value != NULL) {
48                boolean_t ex_match = strcmp(key, value) ?
49                    B_FALSE : B_TRUE;
50                boolean_t match = strcasecmp(key, value) ?
51                    B_FALSE : B_TRUE;
52
53                if (ex_match == B_FALSE && case_ins == B_TRUE &&
54                    match == B_TRUE) {
55                    code2 = add_to_list(&values, name);
56                    if (code2 != 0) {
57                        end_list(&values, 0);
58                        code = code2;
59                    } else
60                        end_list(&values, &ret_values);
61                    goto cleanup;
62                } else if (ex_match == B_FALSE ||
63                    case_ins == B_TRUE)
64                    goto not_found;
65            }
66            code2 = add_to_list(&values, name);
67            if (code2 != 0) {
68                end_list(&values, 0);
69                code = code2;
70                goto cleanup;
71            }
72            found = B_TRUE;
73        }
74
75not_found:
76        if (name != NULL) {
77            profile_release_string(name);
78            name = NULL;
79        }
80        if (value != NULL) {
81            profile_release_string(value);
82            value = NULL;
83        }
84    }

The while loop will segmentation fault during the *third* loop. During
the *second* loop, *profile_node_iterator()* underneath will free the
iter pointer, then the third loop will still use that iter pointer to
deference iter->magic value. So my question is that do we use the
profile_iterator() in the wrong way or there should be an iter check in
the profile_iterator()? Thanks.


Best

--
Neng Xue
Oracle Solaris Software Engineer
Santa Clara, CA, USA

_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about iter check in profile_iterator()

Will Fiveash-2
On Mon, Jan 12, 2015 at 04:06:40PM -0800, Neng Xue wrote:

> Hi MIT krb team,
>
> Recently I am working on a kerberos related project in Solaris which will
> upgrade our krb version to 1.13. We have a set of solaris specific profile
> interfaces underneath using MIT kerberos profile interfaces. They used to
> work fine. However, after the version upgrade I noticed the interface
> wrapper encountered an issue. I narrowed down the issue to:
>
> *prof_get.c:profile_iterator()*:
>
> 582 if (iter->magic != PROF_MAGIC_ITERATOR)
> 583        return PROF_MAGIC_ITERATOR;
>
> When the iter is NULL our program will segmentation fault because of the
> null pointer deference.
>
> Here is piece of our code:
>
>       code = *profile_iterator_create*(profile, hierarchy,
> 42        PROFILE_ITER_LIST_SECTION, *&state*);
> 43    while (code == 0) {
> 44        code = *profile_iterator*(&*state*, &name, &value);
> 45        if (code == 0 && name != NULL) {
> 46
> 47            if (key != NULL && value != NULL) {
> 48                boolean_t ex_match = strcmp(key, value) ?
> 49                    B_FALSE : B_TRUE;
> 50                boolean_t match = strcasecmp(key, value) ?
> 51                    B_FALSE : B_TRUE;
> 52
> 53                if (ex_match == B_FALSE && case_ins == B_TRUE &&
> 54                    match == B_TRUE) {
> 55                    code2 = add_to_list(&values, name);
> 56                    if (code2 != 0) {
> 57                        end_list(&values, 0);
> 58                        code = code2;
> 59                    } else
> 60                        end_list(&values, &ret_values);
> 61                    goto cleanup;
> 62                } else if (ex_match == B_FALSE ||
> 63                    case_ins == B_TRUE)
> 64                    goto not_found;
> 65            }
> 66            code2 = add_to_list(&values, name);
> 67            if (code2 != 0) {
> 68                end_list(&values, 0);
> 69                code = code2;
> 70                goto cleanup;
> 71            }
> 72            found = B_TRUE;
> 73        }
> 74
> 75not_found:
> 76        if (name != NULL) {
> 77            profile_release_string(name);
> 78            name = NULL;
> 79        }
> 80        if (value != NULL) {
> 81            profile_release_string(value);
> 82            value = NULL;
> 83        }
> 84    }
>
> The while loop will segmentation fault during the *third* loop. During the
> *second* loop, *profile_node_iterator()* underneath will free the iter
> pointer, then the third loop will still use that iter pointer to deference
> iter->magic value. So my question is that do we use the profile_iterator()
> in the wrong way or there should be an iter check in the profile_iterator()?
> Thanks.

Given what Neng has told me it looks like profile_iterator() is
returning 0 when iter->idata is NULL just after the call to
profile_node_iterator() which leads to (in profile_iterator):

    retval = profile_node_iterator(&iter->idata, 0, &name, &value);
    if (iter->idata == NULL) {  <----
        free(iter);  <----
        *iter_p = NULL;  <----
    }

Also there is the call to profile_node_iterator() and in the definition
of profile_node_iterator() I see:

errcode_t profile_node_iterator(void **iter_p, <--------
                                struct profile_node **ret_node,
                                char **ret_name, char **ret_value)

So in the profile_iterator call to profile_node_iterator() the
&iter->idata arg is iter_p for profile_node_iterator?  Seems odd to me.

--
Will Fiveash
Oracle Solaris Software Engineer
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about iter check in profile_iterator()

Greg Hudson
On 01/12/2015 07:06 PM, Neng Xue wrote:
> So my question is that do we use the
> profile_iterator() in the wrong way or there should be an iter check in
> the profile_iterator()? Thanks.

The loop condition should be "state != NULL" rather than "code == 0".
profile_iterator_create returns 0 and sets *iter_p to NULL on the last
iteration.

On 01/12/2015 08:13 PM, Will Fiveash wrote:
> So in the profile_iterator call to profile_node_iterator() the
> &iter->idata arg is iter_p for profile_node_iterator?  Seems odd to me.

I don't see what is odd about that.  iter->idata holds the node iterator
state.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about iter check in profile_iterator()

Shawn M Emery
On 01/12/15 08:40 PM, Greg Hudson wrote:
> On 01/12/2015 07:06 PM, Neng Xue wrote:
>> So my question is that do we use the
>> profile_iterator() in the wrong way or there should be an iter check in
>> the profile_iterator()? Thanks.
> The loop condition should be "state != NULL" rather than "code == 0".
> profile_iterator_create returns 0 and sets *iter_p to NULL on the last
> iteration.

We may want to change other functions like krb5_425_conv_principal if
this is the intended conditional.

Shawn.
--

> On 01/12/2015 08:13 PM, Will Fiveash wrote:
>> So in the profile_iterator call to profile_node_iterator() the
>> &iter->idata arg is iter_p for profile_node_iterator?  Seems odd to me.
> I don't see what is odd about that.  iter->idata holds the node iterator
> state.
> _______________________________________________
> krbdev mailing list             [hidden email]
> https://mailman.mit.edu/mailman/listinfo/krbdev
>
>

_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about iter check in profile_iterator()

Greg Hudson
On 01/13/2015 01:45 AM, Shawn M Emery wrote:
> We may want to change other functions like krb5_425_conv_principal
> if this is the intended conditional.

That does appear broken, and also appears to be our only use of
profile_iterator() outside of the profile itself.

Prior to 1.10 that code would get a PROF_MAGIC_ITERATOR error from the
invalid call after the final iteration, so it would function even
though it was incorrectly using the interface.  I think it would be
good to restore the 1.9 behavior by adding a null pointer check to the
magic check, but it would also be good to fix krb5_425_conv_principal
to correctly detect the final iteration.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about iter check in profile_iterator()

Neng Xue
In reply to this post by Greg Hudson
Hi Greg,

Thanks for the clarification!

Best,
Neng

On 01/12/15 07:40 PM, Greg Hudson wrote:

> On 01/12/2015 07:06 PM, Neng Xue wrote:
>> So my question is that do we use the
>> profile_iterator() in the wrong way or there should be an iter check in
>> the profile_iterator()? Thanks.
> The loop condition should be "state != NULL" rather than "code == 0".
> profile_iterator_create returns 0 and sets *iter_p to NULL on the last
> iteration.
>
> On 01/12/2015 08:13 PM, Will Fiveash wrote:
>> So in the profile_iterator call to profile_node_iterator() the
>> &iter->idata arg is iter_p for profile_node_iterator?  Seems odd to me.
> I don't see what is odd about that.  iter->idata holds the node iterator
> state.

--
Neng Xue
Oracle Solaris Software Engineer
Santa Clara, CA, USA

_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about iter check in profile_iterator()

Greg Hudson
In reply to this post by Shawn M Emery
On 01/13/2015 01:45 AM, Shawn M Emery wrote:
> We may want to change other functions like krb5_425_conv_principal if
> this is the intended conditional.

I need to amend my previous answer on this.

When the iteration has finished, profile_iterator() returns 0 with
*iter_p, *ret_name, and *ret_value all set to NULL.
krb5_425_conv_principal() detects this at line 290 and breaks out of the
loop.  The code quoted in this post's original thread doesn't do that,
instead continuing on to call profile_iterator() once again with a null
*iter_p.

I don't think a change to krb5_425_conv_principal() is warranted.  There
are several ways to correctly terminate a profile iteration loop and it
uses one of them.  If I were writing an iteration loop myself, I would
probably write:

    for (;;) {
        ret = profile_iterator(&state, &name, &value);
        if (ret)
            goto cleanup; /* ... or otherwise handle the error */
        if (name == NULL)
            break;
        /* Do stuff with name and value. */
        profile_release_string(name);
        profile_release_string(value);
    }

I will submit a pull request to restore the 1.9 behavior of
profile_iterator() on a null *iter_p, but correct calling code should
not be relying on that behavior.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about iter check in profile_iterator()

Will Fiveash-2
In reply to this post by Greg Hudson
On Mon, Jan 12, 2015 at 10:40:39PM -0500, Greg Hudson wrote:
>
> On 01/12/2015 08:13 PM, Will Fiveash wrote:
> > So in the profile_iterator call to profile_node_iterator() the
> > &iter->idata arg is iter_p for profile_node_iterator?  Seems odd to me.
>
> I don't see what is odd about that.  iter->idata holds the node iterator
> state.

Eh, looking at the code more I see what's going on so nevermind.
--
Will Fiveash
Oracle Solaris Software Engineer
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: Questions about iter check in profile_iterator()

Will Fiveash-2
On Wed, Jan 14, 2015 at 03:45:01PM -0600, Will Fiveash wrote:

> On Mon, Jan 12, 2015 at 10:40:39PM -0500, Greg Hudson wrote:
> >
> > On 01/12/2015 08:13 PM, Will Fiveash wrote:
> > > So in the profile_iterator call to profile_node_iterator() the
> > > &iter->idata arg is iter_p for profile_node_iterator?  Seems odd to me.
> >
> > I don't see what is odd about that.  iter->idata holds the node iterator
> > state.
>
> Eh, looking at the code more I see what's going on so nevermind.

And looking more I think that in the public header there should be:

struct prof_iter;
typedef struct prof_iter *prof_iter;

and the profile_iterator declaration:

profile_iterator(void **iter_p,...

should instead be:

profile_iterator(prof_iter *iter_p,...
{

  struct profile_iterator *iter = (struct profile_iterator *) *iter_p;

etc...  This would allow the compiler type checking to catch caller
errors like:

void*      iterator = NULL;
...
profile_iterator (iterator, &realm_name, &dummy_value);

which should be:

profile_iterator (&iterator, &realm_name, &dummy_value);
                  ^

Those type of calling errors can currently slip by the compiler.

--
Will Fiveash
Oracle Solaris Software Engineer
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev