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:

Description of the change

Fix for Bug has the SRU template and testing instructions.

PPA with test packages: (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:

Where comment 7 ( 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
  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
1diff --git a/debian/changelog b/debian/changelog
2index 41aaaec..27e36bf 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,10 @@
6+samba (2:4.3.11+dfsg-0ubuntu0.16.04.16) xenial; urgency=medium
8+ * d/p/bug_1583324_include_with_macro.patch: don't fail parsing the
9+ config file if it has macros in include directives (LP: #1583324)
11+ -- Andreas Hasenack <> Thu, 02 Aug 2018 18:30:26 -0300
13 samba (2:4.3.11+dfsg-0ubuntu0.16.04.15) xenial-security; urgency=medium
15 * SECURITY UPDATE: Insufficient input validation on client directory
16diff --git a/debian/patches/bug_1583324_include_with_macro.patch b/debian/patches/bug_1583324_include_with_macro.patch
17new file mode 100644
18index 0000000..600070e
19--- /dev/null
20+++ b/debian/patches/bug_1583324_include_with_macro.patch
21@@ -0,0 +1,65 @@
22+From 3c6ea3293c6aac67bc442f47185fd494714e4806 Mon Sep 17 00:00:00 2001
23+From: Quentin Gibeaux <>
24+Date: Thu, 29 Oct 2015 13:48:27 +0100
25+Subject: [PATCH] lib/param: handle (ignore) substitution variable in smb.conf
29+The function handle_include returns false when trying to include
30+files that have a substitution variable in filename (like %U),
31+this patch makes handle_include to ignore this case, to make
32+samba-tool work when there is such include in samba's configuration.
34+Error was :
35+ root@ubuntu:/usr/local/samba# grep 'include.*%U' etc/smb.conf
36+ include = %U.conf
37+ root@ubuntu:/usr/local/samba# ./bin/samba-tool user list
38+ Can't find include file %U.conf
39+ ERROR(runtime): uncaught exception - Unable to load default file
41+Signed-off-by: Quentin Gibeaux <>
42+Reviewed-by: Michael Adam <>
43+Reviewed-by: Jeremy Allison <>
45+Autobuild-User(master): Jeremy Allison <>
46+Autobuild-Date(master): Wed Dec 9 02:05:30 CET 2015 on sn-devel-104
48+Last-Update: 2018-10-02
50+ lib/param/loadparm.c | 18 ++++++++++++++++++
51+ 1 file changed, 18 insertions(+)
53+--- a/lib/param/loadparm.c
54++++ b/lib/param/loadparm.c
55+@@ -1109,6 +1109,8 @@
56+ const char *pszParmValue, char **ptr)
57+ {
58+ char *fname;
59++ const char *substitution_variable_substring;
60++ char next_char;
62+ if (lp_ctx->s3_fns) {
63+ return lp_ctx->s3_fns->lp_include(lp_ctx, service, pszParmValue, ptr);
64+@@ -1123,6 +1125,22 @@
65+ if (file_exist(fname))
66+ return pm_process(fname, do_section, lpcfg_do_parameter, lp_ctx);
68++ /*
69++ * If the file doesn't exist, we check that it isn't due to variable
70++ * substitution
71++ */
72++ substitution_variable_substring = strchr(fname, '%');
74++ if (substitution_variable_substring != NULL) {
75++ next_char = substitution_variable_substring[1];
76++ if ((next_char >= 'a' && next_char <= 'z')
77++ || (next_char >= 'A' && next_char <= 'Z')) {
78++ DEBUG(2, ("Tried to load %s but variable substitution in "
79++ "filename, ignoring file.\n", fname));
80++ return true;
81++ }
82++ }
84+ DEBUG(2, ("Can't find include file %s\n", fname));
86+ return false;
87diff --git a/debian/patches/series b/debian/patches/series
88index c3957a7..4b191c3 100644
89--- a/debian/patches/series
90+++ b/debian/patches/series
91@@ -102,3 +102,4 @@ CVE-2018-10919-7.patch
92 CVE-2018-10919-8.patch
93 CVE-2018-10919-9.patch
94 CVE-2018-10919-10.patch


People subscribed via source and target branches