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

[Christof Schmitt]

On Thu, Jan 09, 2014 at 01:39:26PM -0800, Jeremy Allison wrote:
> 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 :-).

Thanks for finding that and explaining the problem, i missed it in my
review. I will wait for the corrected patchset.

Christof