Interesting issue with NDR_NOALIGN and nbt encoding.
Jeremy Allison
jra at samba.org
Wed May 23 22:25:08 MDT 2012
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.
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.
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 ?
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.
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.
Cheers,
Jeremy.
-------------- next part --------------
diff --git a/librpc/ndr/ndr.c b/librpc/ndr/ndr.c
index 2279d1c..ff5c280 100644
--- a/librpc/ndr/ndr.c
+++ b/librpc/ndr/ndr.c
@@ -383,6 +383,10 @@ _PUBLIC_ void ndr_set_flags(uint32_t *pflags, uint32_t new_flags)
}
if (new_flags & LIBNDR_ALIGN_FLAGS) {
(*pflags) &= ~LIBNDR_FLAG_REMAINING;
+ (*pflags) &= ~LIBNDR_FLAG_NOALIGN;
+ }
+ if (new_flags & LIBNDR_FLAG_NOALIGN) {
+ (*pflags) &= ~LIBNDR_ALIGN_FLAGS;
}
if (new_flags & LIBNDR_FLAG_NO_RELATIVE_REVERSE) {
(*pflags) &= ~LIBNDR_FLAG_RELATIVE_REVERSE;
diff --git a/librpc/ndr/ndr_basic.c b/librpc/ndr/ndr_basic.c
index 7a4e22a..3565ab6 100644
--- a/librpc/ndr/ndr_basic.c
+++ b/librpc/ndr/ndr_basic.c
@@ -1253,7 +1253,7 @@ _PUBLIC_ enum ndr_err_code ndr_push_DATA_BLOB(struct ndr_push *ndr, int ndr_flag
{
if (ndr->flags & LIBNDR_FLAG_REMAINING) {
/* nothing to do */
- } else if (ndr->flags & LIBNDR_ALIGN_FLAGS) {
+ } else if (ndr->flags & (LIBNDR_ALIGN_FLAGS|LIBNDR_FLAG_NOALIGN)) {
if (ndr->flags & LIBNDR_FLAG_NOALIGN) {
blob.length = 0;
} else if (ndr->flags & LIBNDR_FLAG_ALIGN2) {
@@ -1281,7 +1281,7 @@ _PUBLIC_ enum ndr_err_code ndr_pull_DATA_BLOB(struct ndr_pull *ndr, int ndr_flag
if (ndr->flags & LIBNDR_FLAG_REMAINING) {
length = ndr->data_size - ndr->offset;
- } else if (ndr->flags & LIBNDR_ALIGN_FLAGS) {
+ } else if (ndr->flags & (LIBNDR_ALIGN_FLAGS|LIBNDR_FLAG_NOALIGN)) {
if (ndr->flags & LIBNDR_FLAG_NOALIGN) {
length = 0;
} else if (ndr->flags & LIBNDR_FLAG_ALIGN2) {
More information about the samba-technical
mailing list