The changes for error injection in Samba AD-DC MSRPC requests

[Joseph Sutton]

On 30/08/23 4:48 am, Richard Sharpe via samba-technical wrote:
> Hi folks,
> 
> Attached are two patches to provide early access and solicit feedback
> on the error injection changes I have made so far. I suspect there are
> mistakes I have made and there may be better ways to do this, so I
> would appreciate feedback.
> 
> Since the code is under the GPL these patches must also be regarded as being so.
> 
> The changes allow you to add things like the following to the smb.conf:
> 
> "lsarpc error inject = lsa_LookupSids error NT_STATUS_RPC_CALL_FAILED
> 5; lsa_LookupSids delay 3000 5"
> 
> The meaning of this is inject the specified error into LookupSids
> responses very five requests and also delay them for 2 seconds.
> 
> The changes modify Pidl to generate code to:
> 
> 1. Parse the error injection parameter. This has to handle multiple
> RPC requests etc and may be an issue in that there may be a limit to
> the amount of text you can include. This is handled when the RPC
> interface is initialized.
> 2. Check and inject the errors when needed.
> 
> The potential problems I see:
> 
> 1. If I have not defined an smb.conf parameter for an MSRPC interface
> you cannot inject errors for it. I see that during domain join LSA and
> SAMR are used, but I have only enabled error injection for LSA. This
> is easy to fix but requires a rebuild.
> 2. I have done really ugly things in generate_param.py
> 3. The code changes in librpc/rpc/dcesrv_core.c needs to be looked at carefully.
> 
> The good news is that it seems to work and is now async. The second
> change was to ensure that if more than one client was calling MSRPC
> requests and the first one required a delay the others would also not
> hit a delay. In that respect you need to apply both changes.
> 
> The changes are based on 4.19.0rc1 but probably will apply cleanly to
> other builds. I started with mainline but then dropped back to 4.19.0.
> Also, there is an RPM SPEC file in the patch that you can possibly
> ignore.
> 
> Any feedback at all is welcome.
> 

Looks interesting! I just have a few small pieces of feedback.

diff --git a/docs-xml/smbdotconf/error_inject/lsa_inject.xml 
b/docs-xml/smbdotconf/error_inject/lsa_inject.xml
index 8c478cb8bb7..ec8758f30d1 100644
--- a/docs-xml/smbdotconf/error_inject/lsa_inject.xml
+++ b/docs-xml/smbdotconf/error_inject/lsa_inject.xml
@@ -11,7 +11,7 @@
  	</para>

  	<para>
-		This paremeter is meant for error injection.
+		This parameter is meant for error injection.
  	</para>
  </description>
  <value type="default">empty string</value>

A small typo in the documentation (paremeter → parameter).



diff --git a/pidl/lib/Parse/Pidl/Samba4/NDR/Server.pm 
b/pidl/lib/Parse/Pidl/Samba4/NDR/Server.pm
index e4474400a76..f32317d4ccd 100644
--- a/pidl/lib/Parse/Pidl/Samba4/NDR/Server.pm
+++ b/pidl/lib/Parse/Pidl/Samba4/NDR/Server.pm
@@ -147,7 +147,7 @@ static void $name\__op_unbind(struct 
dcesrv_connection_context *context, const s
  #endif
  }

-struct rpc_err_inject err_inj_table_$name\[$num_calls] = {{0}};
+static struct rpc_err_inject err_inj_table_$name\[$num_calls] = {{0}};

  static NTSTATUS $name\__op_ndr_pull(struct dcesrv_call_state 
*dce_call, TALLOC_CTX *mem_ctx, struct ndr_pull *pull, void **r)
  {

I think the err_inj_table_$name array can be made static.



@@ -268,6 +268,7 @@ sub Boilerplate_Ep_Server($)


  	pidl "
+#ifdef LIB_PARAM_$uname\_ERROR_INJECT
  static int find_$name\_function(const struct ndr_interface_table *table,
  			     char *name)
  {	int i;
@@ -280,7 +281,6 @@ static int find_$name\_function(const struct 
ndr_interface_table *table,
  	return -1;
  }

-#ifdef LIB_PARAM_$uname\_ERROR_INJECT
  /* Process one error injection entry */
  static NTSTATUS $name\_process_one_err_inj(const char *err_inj_param)
  {

The LIB_PARAM_$uname\_ERROR_INJECT #ifdef could be moved up to encompass 
find_$name\_function() and avoid a build error.

Regards,
Joseph