[PATCH] Messaging improvements and fixes needed for auth logging

[Andrew Bartlett]

On Mon, 2017-03-20 at 11:06 +0100, Volker Lendecke wrote:
> On Mon, Mar 20, 2017 at 08:21:13PM +1300, Andrew Bartlett via samba-
> technical wrote:
> > From eef04949817bab65aa3c3268cf8690e7753e174e Mon Sep 17 00:00:00
> > 2001
> > From: Andrew Bartlett <abartlet at samba.org>
> > Date: Tue, 14 Mar 2017 15:22:01 +1300
> > Subject: [PATCH 4/7] lib/util: Do not return an unterminated
> > pointer in
> >  tdb_fetch_talloc()
> > 
> > Otherwise, if a TDB entry is truncated to 0 length, this will
> > return
> > uninitialised memory!
> 
> Can you explain a bit more what is going on here?

Certainly.  Have you run the test I've added?  (make test
TESTS=messaging triggers it nicely). 

>  I would like to
> avoid DATA_BLOB and/or TDB_DATA where it makes sense. Here we always
> return a talloc'ed object that carries its own length. I think that a
> talloc objects is just as expressive as a DATA_BLOB, you can always
> query its length with talloc_get_size.
> 
> I would like to understand the bug that this fixes that is not
> fixable
> with keeping just the uint8_t* return from tdb_fetch_talloc().

The callers otherwise assumed it was a NULL terminated string, and
wandered off the end of the string.  

I understand your passion of talloc_get_size(), and it certainly can be
very useful in inline code and loops, but I don't think that idiom is
as reasonable for for return values.

My view is that in Samba, byte buffers should always be encoded as a
DATA_BLOB, struct ldb_val or TDB_DATA.  This makes it clear to the
caller that before passing to further downstream users (such as
strv_count() taking a const char *) that a check for or addition of
termination is required.

That is, we always look suspiciously at code like:
	ids = (char *)data.dptr;
while the original
        ids = (char *)data;
triggers much less suspicion.

I hope this makes my analysis clear.

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba