Interesting issue with NDR_NOALIGN and nbt encoding.
Andrew Bartlett
abartlet at samba.org
Wed May 23 23:23:36 MDT 2012
On Wed, 2012-05-23 at 21:25 -0700, Jeremy Allison wrote:
> So I'm working on the list of 3.6.x "blocker" bugs, and started
> work on:
>
> https://bugzilla.samba.org/show_bug.cgi?id=8373
>
> Can't join XP Pro workstations to 3.6.1 DC.
>
> It looks like a bug in parsing the NBT netlogon
> packet, inside the function: ndr_pull_nbt_netlogon_packet().
>
> I looked closely, and I found an interesting thing.
>
> These functions are auto-generated in 3.6.x via pidl,
> but have been removed from auto-generation in master
> with a note that:
>
> /* These responses are all handled manually, as they cannot be encoded in IDL fully
> See push_nbt_netlogon_response()
> */
>
> Which was commit b782b5ed by Andrew Bartlett..
> Curiouser and curiouser :-). This fix wasn't
> back-ported into v3-6-test btw. (Actually this
> fix was a simple comment addition, the actual
> fix was 2f5a1d2b1cfdbfc3d4c7c1e96d1ed061e7970f88,
>
> Manually handle the NETLOGON_SAM_LOGON_REQUEST too.
>
> With the sid structure being both optional and aligned, it was too
> hard to do this in just IDL.
>
> also not back-ported into 3.6.x.
Jeremy,
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.
> Looking at bug #8373 *really* closely what it looks
> like is that when parsing nbt_netlogon_query_for_pdc,
> whose idl looks like this:
>
> /* query for pdc request */
> typedef struct {
> astring computer_name;
> astring mailslot_name;
> [flag(NDR_ALIGN2)] DATA_BLOB _pad;
> nstring unicode_name;
> netlogon_nt_version_flags nt_version;
> uint16 lmnt_token;
> uint16 lm20_token;
> } nbt_netlogon_query_for_pdc;
>
> we mess up on parsing out the :
>
> nstring unicode_name;
>
> field, as in the associated capture file from the
> bug it shows that the mailslot_name ends on an odd
> boundary and is then aligned with a zero byte (to
> match the [flag(NDR_ALIGN2)] DATA_BLOB _pad; field).
>
> We get a iconv error which I believe is due to the
> offset being stuck at the padding zero.
>
> So, why isn't the :
>
> [flag(NDR_ALIGN2)] DATA_BLOB _pad;
>
> having the desired effect ? The chain of IDL looks
> like:
>
> nbt_netlogon_query_for_pdc is enclosed by :
>
> typedef [nodiscriminant] union {
> [case(LOGON_REQUEST)] NETLOGON_LOGON_REQUEST logon0;
> [case(LOGON_SAM_LOGON_REQUEST)] NETLOGON_SAM_LOGON_REQUEST logon;
> [case(LOGON_PRIMARY_QUERY)] nbt_netlogon_query_for_pdc pdc;
> [case(NETLOGON_ANNOUNCE_UAS)] NETLOGON_DB_CHANGE uas;
> } nbt_netlogon_request;
>
> which itself is enclosed by :
>
> typedef [flag(NDR_NOALIGN),public] struct {
> netlogon_command command;
> [switch_is(command)] nbt_netlogon_request req;
> } nbt_netlogon_packet;
>
> Note the flag(NDR_NOALIGN) assignment to the
> nbt_netlogon_packet struct. It turns out that
> setting flag(NDR_NOALIGN) on a structure affects
> *all* enclosed sub-marshalling/unmarshalling calls
> when called from code marshalling/unmarshalling
> this struct.
Yes, that's how it works.
> Looking carefully into our NBT functions I found code
> to hand-marshall similar structures such as :
>
> ndr_push_NETLOGON_SAM_LOGON_REQUEST()
>
> where we have :
>
> uint32_t _flags_save_DATA_BLOB = ndr->flags;
> ndr->flags &= ~LIBNDR_FLAG_NOALIGN;
> ndr_set_flags(&ndr->flags, LIBNDR_FLAG_ALIGN4);
> NDR_CHECK(ndr_push_DATA_BLOB(ndr, NDR_SCALARS, r->_pad));
> ndr->flags = _flags_save_DATA_BLOB;
>
> We're hand-unsetting LIBNDR_FLAG_NOALIGN here, as it
> turns out that ndr_set_flags() only ever OR's given
> flags into the ndr->flags field (with some complex
> rules).
>
> So why do we have to do this ? Right now it turns out
> that when we set flag(NDR_NOALIGN) in the definition
> of the nbt_netlogon_packet struct, this is recursive
> and means that we don't align all the way down when
> marshalling or unmarshalling - which is what we want.
>
> Until, that is, we hit the flag(NDR_ALIGN2) on the
> DATA_BLOB _pad definition. The NDR_ALIGN2 bit is
> set in the generated code via ndr_set_flags(), but
> as the LIBNDR_FLAG_NOALIGN is already set from the
> calling code, and setting this bit does not reset
> the LIBNDR_FLAG_NOALIGN it means it is completely
> ignored when evaluating the alignment. Thus the
> _pad blob alignment generation has no effect, and
> we end up being stuck on the offset of the padding
> zero.
>
> Sorry for this being so long, but the upshot of all
> this is I think that the flags
>
> LIBNDR_FLAG_ALIGN2|LIBNDR_FLAG_ALIGN4|LIBNDR_FLAG_ALIGN8
>
> which are collectively defined as LIBNDR_ALIGN_FLAGS,
> should be mutually exclusive with LIBNDR_FLAG_NOALIGN,
> in that when you set LIBNDR_FLAG_NOALIGN, the bits
> of LIBNDR_ALIGN_FLAGS should be removed, and when you
> set any of the LIBNDR_ALIGN_FLAGS bits, LIBNDR_FLAG_NOALIGN
> should be removed.
>
> If we do this correctly I think it then allows the
> nbt_netlogon_query_for_pdc struct to be correctly
> marshalled/unmarshalled by the gen_ndr generated
> code. I'm wondering if it also may remove the
> need for some of the hand-generation of this code
> that got put into master ?
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.
> 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?
> This really doesn't seem right to me, and if it IS
> explicitly part of the design, I'd like to know what
> that design is.
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.
Andrew Bartlett
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
More information about the samba-technical
mailing list