[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