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

Proposed by Andreas Hasenack on 2018-08-03
Status: Merged
Approved by: Christian Ehrhardt  on 2018-08-07
Approved revision: 2407df3c3e905208bda8dc155b4e7ad520bdd668
Merge reported by: Andreas Hasenack
Merged at revision: cb29ce25ffd64f56120c1d819b3a35b0b2a83a54
Proposed branch: ~ahasenack/ubuntu/+source/samba:xenial-samba-include-1583324
Merge into: ubuntu/+source/samba:ubuntu/xenial-devel
Diff against target: 95 lines (+73/-0)
3 files modified
debian/changelog (+7/-0)
debian/patches/bug_1583324_include_with_macro.patch (+65/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Christian Ehrhardt  2018-08-03 Approve on 2018-08-07
Canonical Server Team 2018-08-03 Pending
Review via email: mp+352301@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Andreas Hasenack (ahasenack) wrote :

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

Christian Ehrhardt  (paelzer) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

I agree, thanks for the extra check on this.

review: Approve
Christian Ehrhardt  (paelzer) wrote :

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) wrote :

Please set to merged when accepted by the SRU Team

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/changelog b/debian/changelog
index 41aaaec..27e36bf 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
1samba (2:4.3.11+dfsg-0ubuntu0.16.04.16) xenial; urgency=medium
2
3 * d/p/bug_1583324_include_with_macro.patch: don't fail parsing the
4 config file if it has macros in include directives (LP: #1583324)
5
6 -- Andreas Hasenack <andreas@canonical.com> Thu, 02 Aug 2018 18:30:26 -0300
7
1samba (2:4.3.11+dfsg-0ubuntu0.16.04.15) xenial-security; urgency=medium8samba (2:4.3.11+dfsg-0ubuntu0.16.04.15) xenial-security; urgency=medium
29
3 * SECURITY UPDATE: Insufficient input validation on client directory10 * 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
4new file mode 10064411new file mode 100644
index 0000000..600070e
--- /dev/null
+++ b/debian/patches/bug_1583324_include_with_macro.patch
@@ -0,0 +1,65 @@
1From 3c6ea3293c6aac67bc442f47185fd494714e4806 Mon Sep 17 00:00:00 2001
2From: Quentin Gibeaux <qgibeaux@iris-tech.fr>
3Date: Thu, 29 Oct 2015 13:48:27 +0100
4Subject: [PATCH] lib/param: handle (ignore) substitution variable in smb.conf
5
6BUG: https://bugzilla.samba.org/show_bug.cgi?id=10722
7
8The function handle_include returns false when trying to include
9files that have a substitution variable in filename (like %U),
10this patch makes handle_include to ignore this case, to make
11samba-tool work when there is such include in samba's configuration.
12
13Error was :
14 root@ubuntu:/usr/local/samba# grep 'include.*%U' etc/smb.conf
15 include = %U.conf
16 root@ubuntu:/usr/local/samba# ./bin/samba-tool user list
17 Can't find include file %U.conf
18 ERROR(runtime): uncaught exception - Unable to load default file
19
20Signed-off-by: Quentin Gibeaux <qgibeaux@iris-tech.fr>
21Reviewed-by: Michael Adam <obnox@samba.org>
22Reviewed-by: Jeremy Allison <jra@samba.org>
23
24Autobuild-User(master): Jeremy Allison <jra@samba.org>
25Autobuild-Date(master): Wed Dec 9 02:05:30 CET 2015 on sn-devel-104
26Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/samba/+bug/1583324
27Last-Update: 2018-10-02
28---
29 lib/param/loadparm.c | 18 ++++++++++++++++++
30 1 file changed, 18 insertions(+)
31
32--- a/lib/param/loadparm.c
33+++ b/lib/param/loadparm.c
34@@ -1109,6 +1109,8 @@
35 const char *pszParmValue, char **ptr)
36 {
37 char *fname;
38+ const char *substitution_variable_substring;
39+ char next_char;
40
41 if (lp_ctx->s3_fns) {
42 return lp_ctx->s3_fns->lp_include(lp_ctx, service, pszParmValue, ptr);
43@@ -1123,6 +1125,22 @@
44 if (file_exist(fname))
45 return pm_process(fname, do_section, lpcfg_do_parameter, lp_ctx);
46
47+ /*
48+ * If the file doesn't exist, we check that it isn't due to variable
49+ * substitution
50+ */
51+ substitution_variable_substring = strchr(fname, '%');
52+
53+ if (substitution_variable_substring != NULL) {
54+ next_char = substitution_variable_substring[1];
55+ if ((next_char >= 'a' && next_char <= 'z')
56+ || (next_char >= 'A' && next_char <= 'Z')) {
57+ DEBUG(2, ("Tried to load %s but variable substitution in "
58+ "filename, ignoring file.\n", fname));
59+ return true;
60+ }
61+ }
62+
63 DEBUG(2, ("Can't find include file %s\n", fname));
64
65 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
102CVE-2018-10919-8.patch102CVE-2018-10919-8.patch
103CVE-2018-10919-9.patch103CVE-2018-10919-9.patch
104CVE-2018-10919-10.patch104CVE-2018-10919-10.patch
105bug_1583324_include_with_macro.patch

Subscribers

People subscribed via source and target branches