Interesting issue with NDR_NOALIGN and nbt encoding.

[Andrew Bartlett]

On Wed, 2012-05-23 at 22:32 -0700, Jeremy Allison wrote:
> On Thu, May 24, 2012 at 03:23:36PM +1000, Andrew Bartlett wrote:
> > 
> > That commit was in 2008, when we first got the WSPP docs, and I started
> > to properly handle these structures.  I may not have filed a
> > release-critical bug to move the comment, but I assure you, the rest of
> > this code is very old indeed. 
> > 
> > What is much more relevant is the work I did (including testing) on
> > WinXP failing to join 3.6 RC:
> > https://bugzilla.samba.org/show_bug.cgi?id=8326
> > 
> > Also for background see the change Volker made in commit
> > 2642f385887fbd3aecd4286a4d6223a21d981714
> > Author: Volker Lendecke <vl at samba.org>
> > Date:   Thu Nov 17 22:36:22 2011 +0100
> > 
> >     s3: Fix bug 8371
> >     
> >     ndr_set_flag or's in the given flag (ALIGN4). At this point,
> > ndr->flags
> >     contains NOALIGN, which will persist. In ndr_push_DATA_BLOB NOALIGN
> > overrides
> >     everything else, so that the ALIGN4 is not respected.
> >     
> >     Autobuild-User: Volker Lendecke <vlendec at samba.org>
> >     Autobuild-Date: Fri Nov 18 09:33:37 CET 2011 on sn-devel-104
> > 
> > Metze: 
> > The patch from metze on the bug
> > https://bugzilla.samba.org/show_bug.cgi?id=8371
> > https://attachments.samba.org/attachment.cgi?id=7118 does not seem to
> > have made it into master, but also seems relevant. 
> 
> No worries - I wasn't blaming you, just trying
> to understand the history of the code here. It's
> really interesting stuff and you seemed to be the
> last one touching it - and you know how that looks :-).

I do appreciate you pinging me about this, as with my work in 2008 and
the WinXP domain join bug I found just before the 3.6 release, I have
had a good deal to do with this code. 

> > I doubt it will allow us not to hand-generate to code you originally
> > mentioned.  Certainly for the response 'netlogon'
> >  packets, the issue is that the only way to parse them is to first read
> > the last bytes, and then work forward. 
> 
> Can you explain more there ? Why is this the case ?

In netlogon_samlogon_response the union is switched on the value 8 bytes
in from the end of the packet!  In nbt_netlogon_response it is similar,
the last 4 bytes. 

> > > So attached is a patch to do just that. I'm testing
> > > it currently in master, but I'd really like to get
> > > feedback if I've missed something as to why it is
> > > allowed to set any of LIBNDR_FLAG_ALIGN2|LIBNDR_FLAG_ALIGN4|LIBNDR_FLAG_ALIGN8
> > > concurrently with LIBNDR_FLAG_NOALIGN.
> > 
> > What is the point of LIBNDR_FLAG_NOALIGN then if all the alignment flags
> > override it?
> 
> Ah - they're *SUPPOSED* to override it - that's the point.
> But they do so temporarily.
> 
> Whenever you set a [flag(NDR_ALIGNXXX)] on a field
> you're saying that field (and everything it contains)
> need to be aligned on that boundary. But it's only
> for that field.
> 
> So the code generated looks like :
> 
> 	uint32_t _flags_save_xxx = ndr->flags;
> 	ndr_set_flags(&ndr->flags, LIBNDR_FLAG_ALIGNY);
> 	NDR_CHECK(ndr_push_XXX(ndr, NDR_SCALARS, r->xxx));
> 	ndr->flags = _flags_save_xxx;
> 
> Note we save off the containing ndr->flags and restore
> after dealing with the sub-field.
> 
> So I think the correct fix here is to treat NDR_NOALIGN
> in *exactly* the same way as the other NDR_ALIGNX types,
> and when any of them is set it overrides any of the others
> previously set.

I guess that seems reasonable.  It does seem to prevent noalign from
overriding those however.  Effectively it means 'no default alignment',
rather than 'no alignment'.  We use it in a lot of places - fixing this
for NBT could expose an assumption somewhere else. 

> I think the small patch I just posted (and am attaching
> again to this email) does just that.
> 
> > I'm not totally sure here either, except to say that we need more
> > known-correct blobs and tests for them, and that a good number of us
> > have been stumbling though different parts of this puzzle over the past
> > year, and at least we should connect the dots.  It may be that some of
> > the top level NOALIGN flags are simply wrong. 
> 
> More testing gratefully received :-). I'm going to see
> what feedback my change gets from the testers of that
> 3.6.x bug.

Have we worked out what would go wrong if we simply dropped the NOALIGN
totally?  Either that, or make a new flags NDR_NO_DEFAULT_ALIGN to have
it align these strings (marked with their alignment), but not a uint32?

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org