Bug 11555 - lookup_names() looks up qualified names as unqualified.
lookup_names() looks up qualified names as unqualified.
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: DCE-RPCs and pipes
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-10-14 17:42 UTC by Jeremy Allison
Modified: 2015-10-21 09:55 UTC (History)
6 users (show)

See Also:


Attachments
git-am fix for master. (2.42 KB, patch)
2015-10-14 18:32 UTC, Jeremy Allison
no flags Details
git-am fix for master. (3.81 KB, patch)
2015-10-14 20:49 UTC, Jeremy Allison
asn: review+
Details
git-am fix for master. (5.87 KB, patch)
2015-10-15 17:12 UTC, Jeremy Allison
no flags Details
git-am fix for 4.3.next, 4.2.next (4.67 KB, patch)
2015-10-15 21:48 UTC, Jeremy Allison
jra: review? (uri)
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2015-10-14 17:42:35 UTC
Inside lookup_names() we have logic that should exit the name lookup if a name is passed in with a qualifying DOMAIN\ component (e.g. DOMAIN\name rather than just name).

Name lookup should only continue if DOMAIN == "" and the flag LOOKUP_NAME_ISOLATED is passed in.

This logic is incorrect in current master.

Patch to follow.
Comment 1 Jeremy Allison 2015-10-14 17:48:30 UTC
Logic from Microsoft we should be following for reference.

https://msdn.microsoft.com/en-us/library/windows/desktop/ms721797(v=vs.85).aspx
Comment 2 Jeremy Allison 2015-10-14 18:32:09 UTC
Created attachment 11490 [details]
git-am fix for master.
Comment 3 Jeremy Allison 2015-10-14 19:06:48 UTC
From: Justin Maggard <justin.maggard@netgear.com>
To: Jeremy Allison <jra@samba.org>
Subject: Re: Windows SID lookup

On 10/14/2015 10:25 AM, Jeremy Allison wrote:

> Can you try this patch. I'm pretty sure this should fix it !
>

It works!  Well done!  Had just enough time to run it through before my
meeting. :-)

-Justin
Comment 4 Jeremy Allison 2015-10-14 20:03:53 UTC
Comment on attachment 11490 [details]
git-am fix for master.

Hmmm. This causes the samba3.rpc.samba3.getusername test to fail with unable to look up 'NT Authority\Anonymous Logon'.

Investigating...
Comment 5 Jeremy Allison 2015-10-14 20:41:01 UTC
Comment on attachment 11490 [details]
git-am fix for master.

Found the problem. "NT Authority\xxx" as a wellknown name with a non-blank domain component should be looked up before we bail if given a blank domain.

Updated patch to follow.
Comment 6 Jeremy Allison 2015-10-14 20:49:10 UTC
Created attachment 11493 [details]
git-am fix for master.
Comment 7 Jeremy Allison 2015-10-14 23:53:24 UTC
From: Justin Maggard <justin.maggard@netgear.com>
To: Jeremy Allison <jra@samba.org>
Subject: Re: Windows SID lookup

On 10/14/2015 01:50 PM, Jeremy Allison wrote:

> It works but is incomplete :-). This version survives more
> of make test :-).
>

Cool, still working. :-)  Thanks for the update.
Comment 8 Andreas Schneider 2015-10-15 07:58:10 UTC
Comment on attachment 11493 [details]
git-am fix for master.

RB+ feel free to push to master!
Comment 9 Jeremy Allison 2015-10-15 17:12:51 UTC
Created attachment 11497 [details]
git-am fix for master.

Uri spotted a problem with the patch. From samba-technical:

From: Uri Simchoni <uri@samba.org>
To: samba-technical@lists.samba.org
Subject: Re: [PATCH] Fix bug #11555 - lookup_names() looks up qualified names as unqualified.

The use of the "NT Authority" literal string got me suspicious :)

I think this code would accept "NT Authority\Creator Group" and
translate it into "\Creator Group" with a SID that's not "NT
Authority".

Here is a modified version including Ralph's fix for that, and a reworking of Ralph's WIP to explicitly check for "NT Authority" in the "domain[0] != '\0' part of the code.
Comment 10 Jeremy Allison 2015-10-15 21:48:08 UTC
Created attachment 11498 [details]
git-am fix for 4.3.next, 4.2.next

Cherry-picked from master. Note the last patch (fixing the valid-users test) isn't there as that torture test wasn't back-ported to the release branches and only exists in master.
Comment 11 Jeremy Allison 2015-10-16 16:23:06 UTC
Re-assigning to Karolin for inclusion in 4.3.next, 4.2.next.
Comment 12 Karolin Seeger 2015-10-19 09:48:28 UTC
(In reply to Jeremy Allison from comment #11)
Pushed to autobuild-v4-[2|3]-test.
Comment 13 Karolin Seeger 2015-10-21 09:55:27 UTC
(In reply to Karolin Seeger from comment #12)
Pushed to both branches.
Closing out bug report.

Thanks!