[PATCH] winbindd: error handling in rpc_try_lookup_sids3()

Ralph Böhme slow at samba.org
Sat Apr 1 20:53:24 UTC 2017


On Sat, Apr 01, 2017 at 02:21:14PM +0200, Ralph Böhme wrote:
> On Fri, Mar 31, 2017 at 04:08:36PM -0700, Jeremy Allison wrote:
> > Also, the function below:
> > 
> > rpc_lookup_sids() has exactly the same logic as that
> > in rpc_try_lookup_sids3(). Should we do the same
> > change there ? If not, why not ?
> 
> Yes, there are more places where it's done wrong: winbindd_lookup_sids() and
> dcerpc_lsa_lookup_sids_noalloc().
> 
> Let's see what happens when I try to fix those as well... :)

ok, so how about this one? Fixes all lookupsids and lookupsids3 callers, adding
a new macro NT_STATUS_LOOKUP_ERR. It just passed a private autobuild.

I don't really think the macro name is a good choice, but I couldn't come up
with something else? Suggestions? It could be used for LSA and SAMR SID and name
lookups.

Cheerio!
-slow
-------------- next part --------------
From 7aa3fd15e18e90840d032e77cad7875c1a00cdb7 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 1 Apr 2017 16:44:45 +0200
Subject: [PATCH 1/4] s3/include: add NT_STATUS_LOOKUP_ERR

Useful helper macro to check the return value of LSA and SAMR
translations.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=12728

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/include/lsa.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/source3/include/lsa.h b/source3/include/lsa.h
index 7681aed..c23e942 100644
--- a/source3/include/lsa.h
+++ b/source3/include/lsa.h
@@ -22,4 +22,8 @@ int init_lsa_ref_domain_list(TALLOC_CTX *mem_ctx,
 			     const char *dom_name,
 			     struct dom_sid *dom_sid);
 
+#define NT_STATUS_LOOKUP_ERR(status) \
+	(!NT_STATUS_IS_OK(status) && \
+	 !NT_STATUS_EQUAL(status, STATUS_SOME_UNMAPPED) && \
+	 !NT_STATUS_EQUAL(status, NT_STATUS_NONE_MAPPED))
 #endif
-- 
2.9.3


From eb2901dac4cb206548ef5a57c70b4c3b95c7863b Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 1 Apr 2017 16:56:39 +0200
Subject: [PATCH 2/4] s3/rpc_client: use NT_STATUS_LOOKUP_ERR

No change in behaviour.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=12728

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/rpc_client/cli_lsarpc.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/source3/rpc_client/cli_lsarpc.c b/source3/rpc_client/cli_lsarpc.c
index 518489a..70952f6 100644
--- a/source3/rpc_client/cli_lsarpc.c
+++ b/source3/rpc_client/cli_lsarpc.c
@@ -256,10 +256,7 @@ static NTSTATUS dcerpc_lsa_lookup_sids_noalloc(struct dcerpc_binding_handle *h,
 		return status;
 	}
 
-	if (!NT_STATUS_IS_OK(result) &&
-	    !NT_STATUS_EQUAL(result, NT_STATUS_NONE_MAPPED) &&
-	    !NT_STATUS_EQUAL(result, STATUS_SOME_UNMAPPED))
-	{
+	if (NT_STATUS_LOOKUP_ERR(result)) {
 		*presult = result;
 		return status;
 	}
-- 
2.9.3


From 4269f6347554c09a63458adf08a1e7c62f7a57c1 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sun, 26 Mar 2017 08:22:13 +0200
Subject: [PATCH 3/4] winbindd: error handling in rpc_lookup_sids()

NT_STATUS_NONE_MAPPED and NT_STATUS_SOME_NOT_MAPPED should not be
treated as fatal error. We should continue processing the results and
not bail out.

In case we got NT_STATUS_NONE_MAPPED we must have to ensure all
lsa_TranslatedName are of type SID_NAME_UNKNOWN.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=12728

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/winbindd/winbindd_rpc.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/source3/winbindd/winbindd_rpc.c b/source3/winbindd/winbindd_rpc.c
index 3dd4f77..c607aa6 100644
--- a/source3/winbindd/winbindd_rpc.c
+++ b/source3/winbindd/winbindd_rpc.c
@@ -32,6 +32,7 @@
 #include "rpc_client/cli_samr.h"
 #include "rpc_client/cli_lsarpc.h"
 #include "../libcli/security/security.h"
+#include "lsa.h"
 
 /* Query display info for a domain */
 NTSTATUS rpc_query_user_list(TALLOC_CTX *mem_ctx,
@@ -981,9 +982,11 @@ static NTSTATUS rpc_try_lookup_sids3(TALLOC_CTX *mem_ctx,
 	if (!NT_STATUS_IS_OK(status)) {
 		return status;
 	}
-	if (NT_STATUS_IS_ERR(result)) {
+
+	if (NT_STATUS_LOOKUP_ERR(result)) {
 		return result;
 	}
+
 	if (sids->num_sids != lsa_names2.count) {
 		return NT_STATUS_INVALID_NETWORK_RESPONSE;
 	}
@@ -1010,7 +1013,7 @@ static NTSTATUS rpc_try_lookup_sids3(TALLOC_CTX *mem_ctx,
 			return NT_STATUS_INVALID_NETWORK_RESPONSE;
 		}
 	}
-	return result;
+	return NT_STATUS_OK;
 }
 
 NTSTATUS rpc_lookup_sids(TALLOC_CTX *mem_ctx,
@@ -1043,7 +1046,7 @@ NTSTATUS rpc_lookup_sids(TALLOC_CTX *mem_ctx,
 	if (!NT_STATUS_IS_OK(status)) {
 		return status;
 	}
-	if (NT_STATUS_IS_ERR(result)) {
+	if (NT_STATUS_LOOKUP_ERR(result)) {
 		return result;
 	}
 
@@ -1063,5 +1066,5 @@ NTSTATUS rpc_lookup_sids(TALLOC_CTX *mem_ctx,
 		}
 	}
 
-	return result;
+	return NT_STATUS_OK;
 }
-- 
2.9.3


From 1a3d65b7f25e0d39b2a3629916185a59fadf11b8 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 1 Apr 2017 16:51:07 +0200
Subject: [PATCH 4/4] s3/rpc_client: lookupsids error handling of
 NT_STATUS_NONE_MAPPED

NT_STATUS_NONE_MAPPED is not a fatal error, it just means we must return
all lsa_TranslatedName's as type SID_NAME_UNKNOWN.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=12728

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/rpc_client/cli_lsarpc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/source3/rpc_client/cli_lsarpc.c b/source3/rpc_client/cli_lsarpc.c
index 70952f6..41c1ef4 100644
--- a/source3/rpc_client/cli_lsarpc.c
+++ b/source3/rpc_client/cli_lsarpc.c
@@ -28,6 +28,7 @@
 #include "rpc_client/cli_lsarpc.h"
 #include "rpc_client/init_lsa.h"
 #include "../libcli/security/security.h"
+#include "lsa.h"
 
 /** @defgroup lsa LSA - Local Security Architecture
  *  @ingroup rpc_client
@@ -221,7 +222,7 @@ static NTSTATUS dcerpc_lsa_lookup_sids_noalloc(struct dcerpc_binding_handle *h,
 			return status;
 		}
 
-		if(!NT_STATUS_IS_ERR(result)) {
+		if (!NT_STATUS_LOOKUP_ERR(result)) {
 			lsa_names.count = lsa_names2.count;
 			lsa_names.names = talloc_array(mem_ctx,
 						       struct lsa_TranslatedName,
-- 
2.9.3



More information about the samba-technical mailing list