Re: svn commit: samba r19520 - in branches/SAMBA_4_0/source/lib/ldb/samba: .

On Wed, 2006-11-01 at 07:56 -0500, simo wrote:
> On Wed, 2006-11-01 at 03:17 +0000, abartlet <at> samba.org wrote:
> > Author: abartlet
> > Date: 2006-11-01 03:17:23 +0000 (Wed, 01 Nov 2006)
> > New Revision: 19520
> > 
> > WebSVN: http://websvn.samba.org/cgi-bin/viewcvs.cgi?view=rev&root=samba&rev=19520
> > 
> > Log:
> > Try not to read past the end of the ldb buffer.
> > 
> > Andrew Bartlett
> > 
> > Modified:
> >    branches/SAMBA_4_0/source/lib/ldb/samba/ldif_handlers.c
> > 
> > 
> > Changeset:
> > Modified: branches/SAMBA_4_0/source/lib/ldb/samba/ldif_handlers.c
> > ===================================================================
> > --- branches/SAMBA_4_0/source/lib/ldb/samba/ldif_handlers.c	2006-10-31 19:06:46 UTC (rev 19519)
> > +++ branches/SAMBA_4_0/source/lib/ldb/samba/ldif_handlers.c	2006-11-01 03:17:23 UTC (rev 19520)
> >  <at>  <at>  -80,10 +80,12  <at>  <at> 
> >  
> >  static BOOL ldb_comparision_objectSid_isString(const struct ldb_val *v)
> >  {
> > -	/* see if the input if null-terninated */
> > -	if (v->data[v->length] != '\0') return False;
> > -	
> > +	if (v->length < 3) {
> > +		return False;
> > +	}
> > +
> >  	if (strncmp("S-", (const char *)v->data, 2) != 0) return False;
> > +	
> >  	return True;
> >  }
> >  
> >  <at>  <at>  -179,9 +181,6  <at>  <at> 
> >  	struct GUID guid;
> >  	NTSTATUS status;
> >  
> > -	/* see if the input if null-terninated */
> > -	if (v->data[v->length] != '\0') return False;
> > -
> >  	if (v->length < 33) return False;
> >  
> >  	status = GUID_from_string((const char *)v->data, &guid);
> 
> Checking for the length and checking for termination are not the same
> thing. We can have binary blobs as data. I think that keeping the check
> on null termination is important as it may prevent segfaults deriving by
> a run past the buffer by functions that expect null terminated strings
> as input.

The fundamental problem comes from the fact that ldb presumes that all
buffers have a NULL terminator at v->data[v->length].  However, if you
create a data blob with data_blob(), or the ndr_push_data_blob
functions, this will not contain such a terminator.

Relying on any data to be present at v->data[v->length] is inconsistent
and unexpected.

I realise it works really nicely for strings, but currently it also
works by dumb luck as much as anything...

Andrew Bartlett

--

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Red Hat Inc.                  http://redhat.com