Merge ~ahasenack/ubuntu/+source/nfs-utils:bionic-thread-safety-gssd-1927745 into ubuntu/+source/nfs-utils:ubuntu/bionic-devel

[Andreas Hasenack]

The linked bug has the SRU template filled in.

PPA with test packages: https://launchpad.net/~ahasenack/+archive/ubuntu/gssd-upcall-1927745

Note: it's a race condition and the supplied test doesn't always catch it. Sometimes I get it right away, other times I leave it running for an hour and nothing happens.

I had more success with reproducing the problem once I increased the number of cores available to the VM. I used 4.

This involves NFSv4 with kerberos, but I think I narrowed the setup down enough for an SRU test case. I may update the test case a bit still if I find a way to make the error happen more quickly, or more realiably.

I took the same patches as debian, which are in our focal+ packages, and just updated the DEP3 header with a link to this bug, and updated the "Last-Updated" field.

[Andreas Hasenack (ahasenack)]

DEP8 passed:
autopkgtest [15:19:11]: test local-server-client: [-----------------------
bash: line 1: 29878 Killed /tmp/autopkgtest.REnxBo/build.2Ib/real-tree/debian/tests/local-server-client 2> >(tee -a /tmp/autopkgtest.REnxBo/local-server-client-stderr >&2) > >(tee -a /tmp/autopkgtest.REnxBo/local-server-client-stdout)
autopkgtest [15:19:11]: test process requested reboot with marker boot1
autopkgtest-virt-ssh: WARNING: ssh connection failed. Retrying in 3 seconds...
localhost:/home on /mnt/nfs_home type nfs4 (rw,relatime,vers=4.2,rsize=262144,wsize=262144,namlen=255,hard,proto=tcp,timeo=600,retrans=2,sec=sys,clientaddr=127.0.0.1,local_lock=none,addr=127.0.0.1)
localhost:/var/log on /mnt/nfs_log type nfs4 (rw,relatime,vers=4.2,rsize=262144,wsize=262144,namlen=255,hard,proto=tcp,timeo=600,retrans=2,sec=sys,clientaddr=127.0.0.1,local_lock=none,addr=127.0.0.1)
autopkgtest [15:19:28]: test local-server-client: -----------------------]
autopkgtest [15:19:28]: test local-server-client: - - - - - - - - - - results - - - - - - - - - -
local-server-client PASS
autopkgtest [15:19:28]: @@@@@@@@@@@@@@@@@@@@ summary
local-server-client PASS
autopkgtest [15:19:28]: Binaries: resetting testbed apt configuration
Hit:1 http://br.archive.ubuntu.com/ubuntu bionic InRelease
Hit:2 http://br.archive.ubuntu.com/ubuntu bionic-updates InRelease
Hit:3 http://br.archive.ubuntu.com/ubuntu bionic-backports InRelease
Hit:4 http://br.archive.ubuntu.com/ubuntu bionic-security InRelease
Reading package lists...

I thought that "Killed" message odd, but it's also present in autopkgtest.ubuntu.com runs: https://autopkgtest.ubuntu.com/results/autopkgtest-bionic/bionic/amd64/n/nfs-utils/20201210_022246_2218d@/log.gz

I think it's from the reboot.

[Sergio Durigan Junior (sergiodj)]

I'll review this one.

[Sergio Durigan Junior (sergiodj)]

Thanks for the MP, Andreas.

I was able to reproduce the bug locally by following the SRU template. I have a few comments about it:

- I had to resort to "hostnamectl set-hostname foo.example.com" in order to get the FQDN properly working. Maybe it's worth mentioning this there.

- Something I also did while trying to reproduce the bug was to invoke the "./bz1419280_test_threads" in two terminals simultaneously. I don't know if this helps or not, but it seemed to trigger the problem faster.

- LXD will create VMs using only one core by default, it seems. I think it's also worth mentioning that doing "lxc config set vm-name limits.cpu=4" is a good idea before running anything.

Either way, it is obvious that using strtok(3) in a threaded application is a bad idea. I reviewed the patches and they look fine to me.

I double-checked that the package builds fine and the autopkgtest run is still succeeding.

I'm leaving two minor comments in the code, but other than that the MP LGTM. Feel free to proceed after you address them.

review: Approve

[Andreas Hasenack (ahasenack)]

I eventually remembered we have bileto, and started https://bileto.ubuntu.com/#/ticket/4551, but I don't know where the test run is. It's not in /running, nor are there any results yet.

[Andreas Hasenack (ahasenack)]

> - Something I also did while trying to reproduce the bug was to invoke the "./bz1419280_test_threads" in two terminals simultaneously. I don't know if this helps or not, but it seemed to trigger the problem faster.

This will be a false positive, unless you checked the ps output and confirmed that stat_as was in D state.

[Andreas Hasenack (ahasenack)]

> - I had to resort to "hostnamectl set-hostname foo.example.com" in order to get the FQDN properly working. Maybe it's worth mentioning this there.

Yeah, will do. This is particularly true if you miss the resolv.conf change, or if dhcp renewed the lease while you are testing and it resets the domain search. It's messy to automate this well enough for a test case ;)

[Sergio Durigan Junior (sergiodj)]

> > - Something I also did while trying to reproduce the bug was to invoke the
> "./bz1419280_test_threads" in two terminals simultaneously. I don't know if
> this helps or not, but it seemed to trigger the problem faster.
>
> This will be a false positive, unless you checked the ps output and confirmed
> that stat_as was in D state.

Ah, fair enough. Yeah, stat_as was in D state, but you've got a point here, so it's better to stick to the original reproduction steps.

[Andreas Hasenack (ahasenack)]

Ok, review points addressed, and I also updated the bug description

[Sergio Durigan Junior (sergiodj)]

LGTM, thanks.

review: Approve

[Andreas Hasenack (ahasenack)]

I'm force-pushing the upload tag, turns out there was another upload in unapproved due to https://bugs.launchpad.net/ubuntu/+source/nfs-utils/+bug/1918141. I talked with Robie and he rejected it, since there will be more discussions about that bug in particular.

$ git push pkg upload/1%1.3.4-2.1ubuntu5.4 --force
Enumerating objects: 23, done.
Counting objects: 100% (23/23), done.
Delta compression using up to 4 threads
Compressing objects: 100% (18/18), done.
Writing objects: 100% (18/18), 3.27 KiB | 197.00 KiB/s, done.
Total 18 (delta 13), reused 0 (delta 0), pack-reused 0
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/nfs-utils
 + 6bf167f6...f57b97a7 upload/1%1.3.4-2.1ubuntu5.4 -> upload/1%1.3.4-2.1ubuntu5.4 (forced update)

Uploaded:
$ dput ubuntu nfs-utils_1.3.4-2.1ubuntu5.4_source.changes
Checking signature on .changes
gpg: /home/andreas/git/packages/nfs-utils/nfs-utils_1.3.4-2.1ubuntu5.4_source.changes: Valid signature from AC983EB5BF6BCBA9
Checking signature on .dsc
gpg: /home/andreas/git/packages/nfs-utils/nfs-utils_1.3.4-2.1ubuntu5.4.dsc: Valid signature from AC983EB5BF6BCBA9
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading nfs-utils_1.3.4-2.1ubuntu5.4.dsc: done.
  Uploading nfs-utils_1.3.4-2.1ubuntu5.4.debian.tar.bz2: done.
  Uploading nfs-utils_1.3.4-2.1ubuntu5.4_source.buildinfo: done.
  Uploading nfs-utils_1.3.4-2.1ubuntu5.4_source.changes: done.
Successfully uploaded packages.

diff --git a/debian/changelog b/debian/changelog
index b235caf..7fe02e0 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,12 @@
+nfs-utils (1:1.3.4-2.1ubuntu5.4) bionic; urgency=medium
+
+  * Don't use non-thread-safe strtok() in handle_gssd_upcall()
+    (LP: #1927745):
+    - d/p/0010-gssd-replace-non-thread-safe-strtok-with-strsep.patch
+    - d/p/0011-gssd-Duplicate-the-upcall-string-for-error-messages.patch
+
+ -- Andreas Hasenack <[e-mail]>  Fri, 07 May 2021 11:42:14 -0300
+
 nfs-utils (1:1.3.4-2.1ubuntu5.3) bionic-security; urgency=medium
 
   * SECURITY UPDATE: privilege escalation via directory permissions
diff --git a/debian/patches/0010-gssd-replace-non-thread-safe-strtok-with-strsep.patch b/debian/patches/0010-gssd-replace-non-thread-safe-strtok-with-strsep.patch
new file mode 100644
index 0000000..a7da3da
--- /dev/null
+++ b/debian/patches/0010-gssd-replace-non-thread-safe-strtok-with-strsep.patch
@@ -0,0 +1,42 @@
+From: Frank Sorenson <[e-mail]>
+Date: Wed, 15 Feb 2017 10:36:47 -0500
+Subject: gssd: replace non-thread-safe strtok with strsep
+
+gssd uses the non-thread-safe strtok() function, which
+can lead to incorrect program behavior.
+
+Replace strtok() with the thread-safe strsep().
+
+Signed-off-by: Frank Sorenson <[e-mail]>
+Signed-off-by: Steve Dickson <[e-mail]>
+
+Origin: upstream, http://git.linux-nfs.org/?p=steved/nfs-utils.git;a=commitdiff;h=5ae8be8
+Bug-Redhat: https://bugzilla.redhat.com/show_bug.cgi?id=1419280
+Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=895381
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1927745
+Last-Update: 2020-05-07
+
+---
+ utils/gssd/gssd_proc.c | 3 ++-
+ 1 file changed, 2 insertions(+), 1 deletion(-)
+
+diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
+index d74d3724..30c6aceb 100644
+--- a/utils/gssd/gssd_proc.c
++++ b/utils/gssd/gssd_proc.c
+@@ -729,10 +729,11 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
+ 	char			*target = NULL;
+ 	char			*service = NULL;
+ 	char			*enctypes = NULL;
++	char			*pbuf = info->lbuf;
+ 
+ 	printerr(2, "\n%s: '%s' (%s)\n", __func__, info->lbuf, clp->relpath);
+ 
+-	for (p = strtok(info->lbuf, " "); p; p = strtok(NULL, " ")) {
++	while ((p = strsep(&pbuf, " "))) {
+ 		if (!strncmp(p, "mech=", strlen("mech=")))
+ 			mech = p + strlen("mech=");
+ 		else if (!strncmp(p, "uid=", strlen("uid=")))
+-- 
+2.20.1
+
diff --git a/debian/patches/0011-gssd-Duplicate-the-upcall-string-for-error-messages.patch b/debian/patches/0011-gssd-Duplicate-the-upcall-string-for-error-messages.patch
new file mode 100644
index 0000000..1ce7320
--- /dev/null
+++ b/debian/patches/0011-gssd-Duplicate-the-upcall-string-for-error-messages.patch
@@ -0,0 +1,93 @@
+From: Frank Sorenson <[e-mail]>
+Date: Wed, 15 Feb 2017 10:38:53 -0500
+Subject: gssd:  Duplicate the upcall string for error messages
+
+strsep() modifies the input string, so error messages
+may output only part of the upcall string.
+
+Make a copy of the upcall string, and use that in any
+error messages.
+
+Signed-off-by: Frank Sorenson <[e-mail]>
+Signed-off-by: Steve Dickson <[e-mail]>
+
+Origin: upstream, http://git.linux-nfs.org/?p=steved/nfs-utils.git;a=commitdiff;h=0a4f5e4
+Bug-Redhat: https://bugzilla.redhat.com/show_bug.cgi?id=1419280
+Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=895381
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1927745
+Last-Update: 2021-05-07
+
+---
+ utils/gssd/gssd_proc.c | 17 +++++++++++++----
+ 1 file changed, 13 insertions(+), 4 deletions(-)
+
+diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
+index 30c6aceb..4fc81c30 100644
+--- a/utils/gssd/gssd_proc.c
++++ b/utils/gssd/gssd_proc.c
+@@ -729,10 +729,17 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
+ 	char			*target = NULL;
+ 	char			*service = NULL;
+ 	char			*enctypes = NULL;
++	char			*upcall_str;
+ 	char			*pbuf = info->lbuf;
+ 
+ 	printerr(2, "\n%s: '%s' (%s)\n", __func__, info->lbuf, clp->relpath);
+ 
++	upcall_str = strdup(info->lbuf);
++	if (upcall_str == NULL) {
++		printerr(0, "ERROR: malloc failure\n");
++		goto out_nomem;
++	}
++
+ 	while ((p = strsep(&pbuf, " "))) {
+ 		if (!strncmp(p, "mech=", strlen("mech=")))
+ 			mech = p + strlen("mech=");
+@@ -749,7 +756,7 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
+ 	if (!mech || strlen(mech) < 1) {
+ 		printerr(0, "WARNING: handle_gssd_upcall: "
+ 			    "failed to find gss mechanism name "
+-			    "in upcall string '%s'\n", info->lbuf);
++			    "in upcall string '%s'\n", upcall_str);
+ 		goto out;
+ 	}
+ 
+@@ -762,7 +769,7 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
+ 	if (!uidstr) {
+ 		printerr(0, "WARNING: handle_gssd_upcall: "
+ 			    "failed to find uid "
+-			    "in upcall string '%s'\n", info->lbuf);
++			    "in upcall string '%s'\n", upcall_str);
+ 		goto out;
+ 	}
+ 
+@@ -775,7 +782,7 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
+ 	if (target && strlen(target) < 1) {
+ 		printerr(0, "WARNING: handle_gssd_upcall: "
+ 			 "failed to parse target name "
+-			 "in upcall string '%s'\n", info->lbuf);
++			 "in upcall string '%s'\n", upcall_str);
+ 		goto out;
+ 	}
+ 
+@@ -790,7 +797,7 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
+ 	if (service && strlen(service) < 1) {
+ 		printerr(0, "WARNING: handle_gssd_upcall: "
+ 			 "failed to parse service type "
+-			 "in upcall string '%s'\n", info->lbuf);
++			 "in upcall string '%s'\n", upcall_str);
+ 		goto out;
+ 	}
+ 
+@@ -803,6 +810,8 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
+ 		do_error_downcall(clp->gssd_fd, uid, -EACCES);
+ 	}
+ out:
++	free(upcall_str);
++out_nomem:
+ 	free(info);
+ 	return;
+ }
+-- 
+2.20.1
+
diff --git a/debian/patches/series b/debian/patches/series
index 4d44f03..0ff8b7f 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -18,3 +18,5 @@ uint16-max.diff
 fix-whitespaces-in-nfs-server.service.patch
 nfsiostat-replace-list-reserved-word.patch
 CVE-2019-3689.patch
+0010-gssd-replace-non-thread-safe-strtok-with-strsep.patch
+0011-gssd-Duplicate-the-upcall-string-for-error-messages.patch