Merge ~sergiodj/ubuntu/+source/samba:samba-v1-file-as-directory-bug1872476-v2 into ubuntu/+source/samba:ubuntu/focal-devel

[Sergio Durigan Junior]

The current samba package on focal suffers from a bug that manifests itselft when the SMB1 (NT1) protocol is being used. In this scenario, the user will see regular files being presented as directories when using GIO/gvfs to browse the share (e.g., nautilus, or via the "gio" command). Even though the SMB1 protocol is old, it is still used by some old NAS devices and therefore this issue can impact a non-trivial amount of users.

The fix, which had to be cherry-picked from upstream, is to revert a specific libsmb code that attempts to obtain some POSIX stat(2) information when running on UNIX-like environments. The upstream commit can be found here:

  https://gitlab.com/samba-team/devel/samba/-/commit/39c910fd9cba3caf7414274b678b9eee33d7e20b

In order to reproduce the bug, one can do:

$ lxc launch ubuntu-daily:focal samba-bug1872476
$ lxc shell samba-bug1872476
# apt install samba smbclient gvfs gvfs-backends dbus-x11
# sed -i 's/^\[global\]/[global]\n client min protocol = NT1\n client max protocol = NT1\n server min protocol = NT1\n server max protocol = NT1/g' /etc/samba/smb.conf
# cat >> /etc/samba/smb.conf << __EOF__
[testshare]
  path = /testshare
  readonly = no
  browsable = yes
  guest ok = yes
__EOF__
# mkdir /testshare
# cat > /testshare/123.txt << __EOF__
Hi, this is a test for samba bug 1872476.
This is a regular file, and not a directory.
__EOF__
# ufw allow samba
# eval $(dbus-launch --sh-syntax)
# gio mount -a smb://127.0.0.1/testshare
# gio info smb://127.0.0.1/testshare/123.txt | grep '^type:'
type: directory

As can be seen above, the regular file '123.txt' is mistakenly listed as a directory.

There's a PPA with a proposed fix here:

https://launchpad.net/~sergiodj/+archive/ubuntu/samba-bug1872476-v2

When trying it out, it is important to umount the share and remount it with the new package:

# gio mount -u smb://127.0.0.1/testshare
# add-apt-repository ppa:sergiodj/samba-bug1872476-v2
# apt update && apt upgrade
# gio mount -a smb://127.0.0.1/testshare
# gio info smb://127.0.0.1/testshare/123.txt | grep '^type:'
type: regular

autopkg is still happy:

autopkgtest [14:38:14]: test smbclient-share-access: -----------------------]
autopkgtest [14:38:14]: test smbclient-share-access: - - - - - - - - - - results - - - - - - - - - -
smbclient-share-access PASS
autopkgtest [14:38:15]: @@@@@@@@@@@@@@@@@@@@ summary
cifs-share-access PASS
python-smoke PASS
smbclient-anonymous-share-list PASS
smbclient-authenticated-share-list PASS
smbclient-share-access PASS

[Rafael David Tinoco (rafaeldtinoco)]

I'll review this one.

[Rafael David Tinoco (rafaeldtinoco)]

# checklist for fixes
----------------------------
 [x] changelog entry correct:
     - do not mention changes to d/p/series file in changelog
 [.] targeted to correct codename
 [.] version number is correct
 [.] update-maintainer has been run before
 ----
 [.] changes forwarded upstream/debian (if appropriate)
 [.] patches match what was proposed upstream
 ----
 [x] patches correctly included in debian/patches/series?
 [.] patches have correct DEP3 metadata
 ----
 [-] relying on PPA only for build check ?
 [-] if relying on PPA, did it install correctly ?
 ----
 [.] building it locally ?
 [.] if building locally, was source build good ?
 [.] if building locally, was binary build good ?
 ----
 [-] was autopkgtest tested ?
 ----
 [.] is this a SRU ?
 [x] if a SRU, does the public bug have a template ?
     - please add a SRU template to the public bug
       (https://wiki.ubuntu.com/StableReleaseUpdates)
 [-] is this a bundle of fixes ?
 [.] is this a single fix ?
 ----
 [.] if single fix, was testcase provided ?
 [-] if single fix, and testcase provided, could I reproduce it ?
 [-] if single fix, and testcase provided, did it work ?
----------------------------
 [.] = ok
 [x] = not ok
 [?] = question
 [!] = note
 [-] = n/a
----------------------------
# comments:

- Could you please fix the changelog and add the SRU template to the public
bug ? After you do that, I'll follow your testcase from the SRU (usually
we don't put test cases in the merge proposal, it all goes to the public bug
in the SRU template format).

This happens because first I'll verify your testcase, and, instead of seeking
for it I know where to find. After I have verified and sponsored your patches,
by uploading your source package, the SRU team - since this case is not a fix
to a -devel release - will verify it again AND allow the upload into the
-proposed archive.

It makes it all easier to have this format (thats why we enforce it).

It was a very nice work overall! Congrats!

review: Needs Fixing

[Rafael David Tinoco (rafaeldtinoco)]

I just did a small SRU template at:

https://bugs.launchpad.net/ubuntu/+source/monitoring-plugins/+bug/1875517

you can use, together with the SRU template wiki page, to make yours.

[Andreas Hasenack (ahasenack)]

Groovy is also affected and will need a fix before this SRU can be accepted. I'll bump the samba version in groovy: we will have to go ahead of debian there.

[Sergio Durigan Junior (sergiodj)]

On Tuesday, May 05 2020, Rafael David Tinoco wrote:

Thanks for the review!

> # checklist for fixes
> ----------------------------
> [x] changelog entry correct:
> - do not mention changes to d/p/series file in changelog

Done.

> [.] targeted to correct codename
> [.] version number is correct
> [.] update-maintainer has been run before
> ----
> [.] changes forwarded upstream/debian (if appropriate)
> [.] patches match what was proposed upstream
> ----
> [x] patches correctly included in debian/patches/series?

I don't understand why this was marked as not ok. Is it because of the
changelog entry?

> [.] patches have correct DEP3 metadata
> ----
> [-] relying on PPA only for build check ?
> [-] if relying on PPA, did it install correctly ?
> ----
> [.] building it locally ?
> [.] if building locally, was source build good ?
> [.] if building locally, was binary build good ?
> ----
> [-] was autopkgtest tested ?
> ----
> [.] is this a SRU ?
> [x] if a SRU, does the public bug have a template ?
> - please add a SRU template to the public bug
> (https://wiki.ubuntu.com/StableReleaseUpdates)

I'm creating the habit of filing the MP first, and then add the SRU
template just after it was reviewed. I will make sure to do both the MP
and the SRU template at the same time from now on.

> [-] is this a bundle of fixes ?
> [.] is this a single fix ?
> ----
> [.] if single fix, was testcase provided ?
> [-] if single fix, and testcase provided, could I reproduce it ?
> [-] if single fix, and testcase provided, did it work ?
> ----------------------------
> [.] = ok
> [x] = not ok
> [?] = question
> [!] = note
> [-] = n/a
> ----------------------------
> # comments:
>
> - Could you please fix the changelog and add the SRU template to the public
> bug ? After you do that, I'll follow your testcase from the SRU (usually
> we don't put test cases in the merge proposal, it all goes to the public bug
> in the SRU template format).

Yep!

> This happens because first I'll verify your testcase, and, instead of seeking
> for it I know where to find. After I have verified and sponsored your patches,
> by uploading your source package, the SRU team - since this case is not a fix
> to a -devel release - will verify it again AND allow the upload into the
> -proposed archive.
>
> It makes it all easier to have this format (thats why we enforce it).

Makes sense.

> It was a very nice work overall! Congrats!

Thanks!

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

[Sergio Durigan Junior (sergiodj)]

On Tuesday, May 05 2020, Andreas Hasenack wrote:

> Groovy is also affected and will need a fix before this SRU can be accepted. I'll bump the samba version in groovy: we will have to go ahead of debian there.

Should I work on backporting the patch to groovy as well?

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

[Andreas Hasenack (ahasenack)]

No, in groovy I'll prefer to go with the latest upstream 4.12.x. This will need that d/README.source dance to make sure us and debian get the same dfsg orig tarball.

[Sergio Durigan Junior (sergiodj)]

Rafael, I force-pushed my branch with the changelog changes, and also wrote the SRU template. Let me know what you think :-). Thanks!

[Rafael David Tinoco (rafaeldtinoco)]

>> [x] patches correctly included in debian/patches/series?
>
> I don't understand why this was marked as not ok. Is it because of the
changelog entry?
>

I should have changed this to [.] after moving the changelog entry to the top!

Thanks Sergio. Let me check test case and upload, will post it here.

[Sergio Durigan Junior (sergiodj)]

On Tuesday, May 05 2020, Rafael David Tinoco wrote:

>>> [x] patches correctly included in debian/patches/series?
>>
>> I don't understand why this was marked as not ok. Is it because of the
> changelog entry?
>>
>
> I should have changed this to [.] after moving the changelog entry to the top!

Aha! :-)

> Thanks Sergio. Let me check test case and upload, will post it here.

Thank you, Rafael!

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

[Rafael David Tinoco (rafaeldtinoco)]

 ----
 [.] if single fix, was testcase provided ?
 [.] if single fix, and testcase provided, could I reproduce it ?
 [.] if single fix, and testcase provided, did it work ?
----------------------------
 [.] = ok
 [x] = not ok
 [?] = question
 [!] = note
 [-] = n/a
----------------------------

All good! Thanks for the fixes. Uploading it now...

review: Approve

[Rafael David Tinoco (rafaeldtinoco)]

(c)rafaeldtinoco@focal:~/.../sources/ubuntu/samba$ git ubuntu tag --upload

(c)rafaeldtinoco@focal:~/.../sources/ubuntu/samba$ git describe --tags
upload/2%4.11.6+dfsg-0ubuntu1.2

(c)rafaeldtinoco@focal:~/.../sources/ubuntu/samba$ git push pkg upload/2%4.11.6+dfsg-0ubuntu1.2
Enumerating objects: 16, done.
Counting objects: 100% (16/16), done.
Delta compression using up to 8 threads
Compressing objects: 100% (11/11), done.
Writing objects: 100% (11/11), 2.92 KiB | 124.00 KiB/s, done.
Total 11 (delta 7), reused 0 (delta 0)
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/samba
 * [new tag] upload/2%4.11.6+dfsg-0ubuntu1.2 -> upload/2%4.11.6+dfsg-0ubuntu1.2

-----

(c)rafaeldtinoco@focal:~/.../sources/ubuntu/samba$ dput ubuntu ../samba_4.11.6+dfsg-0ubuntu1.2_source.changes
Checking signature on .changes
gpg: ../samba_4.11.6+dfsg-0ubuntu1.2_source.changes: Valid signature from A93E0E0AD83C0D0F
Checking signature on .dsc
gpg: ../samba_4.11.6+dfsg-0ubuntu1.2.dsc: Valid signature from A93E0E0AD83C0D0F
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading samba_4.11.6+dfsg-0ubuntu1.2.dsc: done.
  Uploading samba_4.11.6+dfsg-0ubuntu1.2.debian.tar.xz: done.
  Uploading samba_4.11.6+dfsg-0ubuntu1.2_source.buildinfo: done.
  Uploading samba_4.11.6+dfsg-0ubuntu1.2_source.changes: done.
Successfully uploaded packages.

diff --git a/debian/changelog b/debian/changelog
index 1b68ade..1c12f80 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,12 @@
+samba (2:4.11.6+dfsg-0ubuntu1.2) focal; urgency=medium
+
+  * Fix "Shared files are shown as folders" (LP: #1872476)
+    - d/p/0023-libsmb-Don-t-try-to-find-posix-stat-info-in-SMBC_get.patch:
+      Don't obtain stat(2) information if dealing with older protocols on
+      UNIX-like systems.
+
+ -- Sergio Durigan Junior <[e-mail]>  Thu, 30 Apr 2020 15:17:24 -0400
+
 samba (2:4.11.6+dfsg-0ubuntu1.1) focal-security; urgency=medium
 
   * SECURITY UPDATE: Use-after-free in AD DC LDAP server
diff --git a/debian/patches/0023-libsmb-Don-t-try-to-find-posix-stat-info-in-SMBC_get.patch b/debian/patches/0023-libsmb-Don-t-try-to-find-posix-stat-info-in-SMBC_get.patch
new file mode 100644
index 0000000..3e0ec24
--- /dev/null
+++ b/debian/patches/0023-libsmb-Don-t-try-to-find-posix-stat-info-in-SMBC_get.patch
@@ -0,0 +1,88 @@
+From: Volker Lendecke <[e-mail]>
+Date: Thu, 19 Mar 2020 11:01:41 +0100
+Subject: libsmb: Don't try to find posix stat info in SMBC_getatr()
+
+This wrongly used "frame" instead of "fname", which can never have
+worked. A first attempt to fix in 51551e0d53fa6 caused a few followup
+patches in an attempt to clean up the test failures 51551e0d53fa6
+introduced. They were reverted after a few discussions. So rather than
+changing behaviour, just remove the code that introduced the valgrind
+error again.
+
+Signed-off-by: Volker Lendecke <[e-mail]>
+Reviewed-by: Andreas Schneider <[e-mail]>
+
+Author: Volker Lendecke <[e-mail]>
+Origin: upstream, https://gitlab.com/samba-team/devel/samba/-/commit/39c910fd9cb
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/samba/+bug/1872476
+Reviewed-By: Sergio Durigan Junior <[e-mail]>
+Last-Update: 2020-04-30
+---
+ source3/include/libsmb_internal.h |  1 -
+ source3/libsmb/libsmb_file.c      | 20 --------------------
+ source3/libsmb/libsmb_server.c    |  9 ---------
+ 3 files changed, 30 deletions(-)
+
+diff --git a/source3/include/libsmb_internal.h b/source3/include/libsmb_internal.h
+index af56df5..edd19c7 100644
+--- a/source3/include/libsmb_internal.h
++++ b/source3/include/libsmb_internal.h
+@@ -76,7 +76,6 @@ typedef struct DOS_ATTR_DESC {
+ struct _SMBCSRV {
+ 	struct cli_state *cli;
+ 	dev_t dev;
+-	bool try_posixinfo;
+ 	bool no_pathinfo;
+ 	bool no_pathinfo2;
+ 	bool no_pathinfo3;
+diff --git a/source3/libsmb/libsmb_file.c b/source3/libsmb/libsmb_file.c
+index 1577010..f0a16c6 100644
+--- a/source3/libsmb/libsmb_file.c
++++ b/source3/libsmb/libsmb_file.c
+@@ -504,26 +504,6 @@ SMBC_getatr(SMBCCTX * context,
+ 		return False;
+ 	}
+ 
+-	if (srv->try_posixinfo) {
+-		SMB_STRUCT_STAT sbuf;
+-
+-		status = cli_posix_stat(targetcli, frame, &sbuf);
+-		if (NT_STATUS_IS_OK(status)) {
+-			setup_stat_from_stat_ex(&sbuf, path, sb);
+-
+-			TALLOC_FREE(frame);
+-			return true;
+-		}
+-		if (NT_STATUS_EQUAL(status, NT_STATUS_NOT_IMPLEMENTED) ||
+-		    NT_STATUS_EQUAL(status, NT_STATUS_INVALID_LEVEL)) {
+-			/*
+-			 * Turn this off if the server doesn't
+-			 * support it.
+-			 */
+-			srv->try_posixinfo = false;
+-		}
+-	}
+-
+ 	if (!srv->no_pathinfo2) {
+ 		status = cli_qpathinfo2(targetcli,
+ 					targetpath,
+diff --git a/source3/libsmb/libsmb_server.c b/source3/libsmb/libsmb_server.c
+index d4f71d8..0067df4 100644
+--- a/source3/libsmb/libsmb_server.c
++++ b/source3/libsmb/libsmb_server.c
+@@ -657,15 +657,6 @@ SMBC_server_internal(TALLOC_CTX *ctx,
+ 	ZERO_STRUCTP(srv);
+ 	DLIST_ADD(srv->cli, c);
+ 	srv->dev = (dev_t)(str_checksum(server) ^ str_checksum(share));
+-	srv->try_posixinfo = false;
+-	/*
+-	 * Until SMB2 POSIX is done, only
+-	 * try POSIX stat on SMB1 with POSIX capabilities.
+-	 */
+-	if ((smbXcli_conn_protocol(c->conn) < PROTOCOL_SMB2_02) &&
+-	    (smb1cli_conn_capabilities(c->conn) & CAP_UNIX)) {
+-		srv->try_posixinfo = true;
+-	}
+         srv->no_pathinfo = False;
+         srv->no_pathinfo2 = False;
+ 	srv->no_pathinfo3 = False;
diff --git a/debian/patches/series b/debian/patches/series
index 6086f87..c1557bd 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -20,3 +20,4 @@ CVE-2020-10704-5.patch
 CVE-2020-10704-6.patch
 CVE-2020-10704-7.patch
 CVE-2020-10704-8.patch
+0023-libsmb-Don-t-try-to-find-posix-stat-info-in-SMBC_get.patch