[PATCH]: NetSessEnum - Removing a loop around locking tdb traversal

[Jeremy Allison]

On Wed, Jan 08, 2014 at 10:00:58AM -0700, Christof Schmitt wrote:
> On Wed, Jan 08, 2014 at 12:38:15PM +0530, Shekhar Amlekar wrote:
> > Hi,
> > 
> > Please find patches that removes a loop around locking tdb traversal in 
> > NetSessEnum() call. I've also removed the getpwnam() call and used the uid 
> > inside the sessionid structure. Request your review.
> 
> Reviewed-by: Christof Schmitt <cs at samba.org>
> 
> Can we get a second review?

NAK on this one. I'm going to explain exactly
why, hopefully not to make Shekhar feel bad,
but to try and explain why this stuff can be
very tricky :-).

I started looking closely when I noticed in
the introduced function count_sess_files_fn()
the following code:

static void count_sess_files_fn(const struct share_mode_entry *e,
                                const char *sharepath, const char *fname,
                                void *data)
{
        struct sess_file_info *info = data;
        uint32_t rh = info->resume_handle;
        int i;

        for (i=0; i < info->num_entries; i++) {

'i' is being declared as an int, but info->num_entries
is a uint32_t. Harmless enough but should be tidied up
so I fixed it an moved on. But it raised a red flag
enough that I started to look very carefully at how
'i' was being used in that function.

Then we have:

struct sessionid *sess = &info->session_list[rh + i];

Hmmm. i is being added to 'rh' = info->resume_handler.
Where does that come from ?

Tracking back we find..

static void net_count_files_for_all_sess(struct srvsvc_NetSessCtr1 *ctr1,
                                         struct sessionid *session_list,
                                         uint32_t resume_handle,
                                         uint32_t num_entries)
{
        struct sess_file_info s_file_info;

        s_file_info.ctr = ctr1;
        s_file_info.session_list = session_list;
        s_file_info.resume_handle = resume_handle;

and the caller:

        /* count open files on all sessions in single tdb traversal */
        net_count_files_for_all_sess(ctr1, session_list,
                                     resume_handle_p ? *resume_handle_p : 0,
                                     num_entries);

But *resume_handle_p is a *client* passed value.
And info->session_list[] is a *server* allocated
array (via *total_entries = list_sessions(p->mem_ctx, &session_list);).

Oops. There are no range checks on the passed in
value..

So the fix up for this is that at the very least
*resume_handle_p+i must be checked for range between
0 and < *total_entries before being safe to use.

I will fix this patchset up and re-post a corrected
version for your review.

But it does show how tricky dealing with RPC values
from the client can be in a server :-).

Cheers,

	Jeremy.