Merge ~ahasenack/ubuntu/+source/samba:xenial-samba-include-1583324 into ubuntu/+source/samba:ubuntu/xenial-devel

[Andreas Hasenack]

Fix for https://bugs.launchpad.net/ubuntu/+source/samba/+bug/1583324. Bug has the SRU template and testing instructions.

PPA with test packages: https://launchpad.net/~ahasenack/+archive/ubuntu/samba-include-1583324 (ppa:ahasenack/samba-include-1583324).

TL;DR "samba-tool testparm" chokes when smb.conf has an include file directive that uses %variable substitutions: it tries to load the filename without expanding the variable. The patch makes it ignore such cases.

[Andreas Hasenack (ahasenack)]

If approving this, then also please push the upload tag and sponsor it, as I can't upload samba myself. Thanks.

[Christian Ehrhardt  (paelzer)]

Formalisms (changelog, version, headers, ...) all LGTM.
Code change itself reads ok from a samba code POV.
Changes are upstream and in newer Ubuntu releases already for a while.

next is testing ...

[Christian Ehrhardt  (paelzer)]

First of all upgrade and the fix as described worked and you have my +1 on what you fixed.

One thing that I have wondered about when testing/fuzzing the config.
I had a config where I extended the bad include to eventually be like:
 include = /etc/samba/smb.conf.%U
 include = /etc/samba/smb.conf.%a
 include = /etc/samba/smb.conf.%b
 include = /etc/samba/smb.conf.%c

When I only have the %U and press enter to dump the config then I get a config without the broken includes.
But with the set of the above (in fact any number of such includes >1) I get the LAST of the broken includes in the dumped config.

Will that also be what the server uses and will it break it?

I'll need your experience to rate if this is an issue or not.
If you say this is find I'm good and we can tag/sponsor - otherwise it is up to the discussion.

review: Needs Information

[Christian Ehrhardt  (paelzer)]

I did a few more experiments, it always keeps the last entry in the dumped output.
Had valid and invalid includes in between - it just seems to apply to the last.

[Andreas Hasenack (ahasenack)]

It looks like it is somewhat expected that it always includes only the last include statement, and this is not a change introduced with this fix.

I found this old bug: https://bugzilla.samba.org/show_bug.cgi?id=4271

Where comment 7 (https://bugzilla.samba.org/show_bug.cgi?id=4271#c7) says:
"""
It is really that printing includes from testparm is deceiving
since it will for one section (share ir global) only ever print
the last include file encountered in that section.
"""

I was under the impression that the fix for that bug, which got merged, would completely remove the include lines from the output of samba-tool testparm, but as you found out, the last include directive is included.

So what happens is that:
- the content of all include files is read and included in the output
- the last include statement is also included, as is, as well as its content. This sounds like a bug, and one that was there before.

The same happens with 4.8.2, a much newer version (latest one at this time is 4.8.3).

This is unrelated to the presence of %macros in the include statement as far as I can see, therefore I believe it's ok to tag and upload.

[Christian Ehrhardt  (paelzer)]

I agree, thanks for the extra check on this.

review: Approve

[Christian Ehrhardt  (paelzer)]

upload/2%4.3.11+dfsg-0ubuntu0.16.04.14 pushed

$ dput ubuntu ../samba_4.3.11+dfsg-0ubuntu0.16.04.14_source.changes
Checking signature on .changes
Checking signature on .dsc
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading samba_4.3.11+dfsg-0ubuntu0.16.04.14.dsc: done.
  Uploading samba_4.3.11+dfsg-0ubuntu0.16.04.14.debian.tar.xz: done.
  Uploading samba_4.3.11+dfsg-0ubuntu0.16.04.14_source.buildinfo: done.
  Uploading samba_4.3.11+dfsg-0ubuntu0.16.04.14_source.changes: done.
Successfully uploaded packages.

[Christian Ehrhardt  (paelzer)]

Please set to merged when accepted by the SRU Team

diff --git a/debian/changelog b/debian/changelog
index 41aaaec..27e36bf 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+samba (2:4.3.11+dfsg-0ubuntu0.16.04.16) xenial; urgency=medium
+
+  * d/p/bug_1583324_include_with_macro.patch: don't fail parsing the
+    config file if it has macros in include directives (LP: #1583324)
+
+ -- Andreas Hasenack <[e-mail]>  Thu, 02 Aug 2018 18:30:26 -0300
+
 samba (2:4.3.11+dfsg-0ubuntu0.16.04.15) xenial-security; urgency=medium
 
   * SECURITY UPDATE: Insufficient input validation on client directory
diff --git a/debian/patches/bug_1583324_include_with_macro.patch b/debian/patches/bug_1583324_include_with_macro.patch
new file mode 100644
index 0000000..600070e
--- /dev/null
+++ b/debian/patches/bug_1583324_include_with_macro.patch
@@ -0,0 +1,65 @@
+From 3c6ea3293c6aac67bc442f47185fd494714e4806 Mon Sep 17 00:00:00 2001
+From: Quentin Gibeaux <[e-mail]>
+Date: Thu, 29 Oct 2015 13:48:27 +0100
+Subject: [PATCH] lib/param: handle (ignore) substitution variable in smb.conf
+
+BUG: https://bugzilla.samba.org/show_bug.cgi?id=10722
+
+The function handle_include returns false when trying to include
+files that have a substitution variable in filename (like %U),
+this patch makes handle_include to ignore this case, to make
+samba-tool work when there is such include in samba's configuration.
+
+Error was :
+	root@ubuntu:/usr/local/samba# grep 'include.*%U' etc/smb.conf
+	include = %U.conf
+	root@ubuntu:/usr/local/samba# ./bin/samba-tool user list
+	Can't find include file %U.conf
+	ERROR(runtime): uncaught exception - Unable to load default file
+
+Signed-off-by: Quentin Gibeaux <[e-mail]>
+Reviewed-by: Michael Adam <[e-mail]>
+Reviewed-by: Jeremy Allison <[e-mail]>
+
+Autobuild-User(master): Jeremy Allison <[e-mail]>
+Autobuild-Date(master): Wed Dec  9 02:05:30 CET 2015 on sn-devel-104
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/samba/+bug/1583324
+Last-Update: 2018-10-02
+---
+ lib/param/loadparm.c | 18 ++++++++++++++++++
+ 1 file changed, 18 insertions(+)
+
+--- a/lib/param/loadparm.c
++++ b/lib/param/loadparm.c
+@@ -1109,6 +1109,8 @@
+ 			   const char *pszParmValue, char **ptr)
+ {
+ 	char *fname;
++	const char *substitution_variable_substring;
++	char next_char;
+ 
+ 	if (lp_ctx->s3_fns) {
+ 		return lp_ctx->s3_fns->lp_include(lp_ctx, service, pszParmValue, ptr);
+@@ -1123,6 +1125,22 @@
+ 	if (file_exist(fname))
+ 		return pm_process(fname, do_section, lpcfg_do_parameter, lp_ctx);
+ 
++       /*
++        * If the file doesn't exist, we check that it isn't due to variable
++        * substitution
++        */
++	substitution_variable_substring = strchr(fname, '%');
++
++	if (substitution_variable_substring != NULL) {
++		next_char = substitution_variable_substring[1];
++		if ((next_char >= 'a' && next_char <= 'z')
++		    || (next_char >= 'A' && next_char <= 'Z')) {
++			DEBUG(2, ("Tried to load %s but variable substitution in "
++				 "filename, ignoring file.\n", fname));
++			return true;
++		}
++	}
++
+ 	DEBUG(2, ("Can't find include file %s\n", fname));
+ 
+ 	return false;
diff --git a/debian/patches/series b/debian/patches/series
index c3957a7..4b191c3 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -102,3 +102,4 @@ CVE-2018-10919-7.patch
 CVE-2018-10919-8.patch
 CVE-2018-10919-9.patch
 CVE-2018-10919-10.patch
+bug_1583324_include_with_macro.patch