Merge ~paride/ubuntu/+source/samba:lp1892145-focal into ubuntu/+source/samba:ubuntu/focal-devel

[Paride Legovini]

LP: #1892145 SRU: cherry-picked upstream fix https://git.samba.org/samba.git/?p=samba.git;a=patch;h=34f8ab774d1484b0e60 in pkg/ubuntu/focal-devel:

  * Fix double free with unresolved credential cache (LP: #1892145)
    Via: d/p/fix-double-free-with-unresolved-credentia-cache.patch

PPA: https://launchpad.net/~paride/+archive/ubuntu/samba-lp1892145

See https://bugs.launchpad.net/samba/+bug/1892145 for more context and details.

[Utkarsh Gupta (utkarsh)]

Two trivial comments. And probably run `quilt refresh` after applying the patch to clean some stuff, I guess. Otherwise looks alright. Though I haven't done a thorough review yet.

Once this is fixed, I'll do so and sponsor the upload if need be.

review: Needs Fixing

[Paride Legovini (paride)]

Thanks! Replied inline, updating my branch.

[Paride Legovini (paride)]

@utkarsh: branch updated with your suggestions, plus a merge from lp-1905387-testparm-focal to include the rich history that the importer didn't pickup, per MM discussion (thanks Robie). The merge commit (67bb0b3) has some explanation.

[Paride Legovini (paride)]

I verified that the following is empty:

  git diff ubuntu/focal-devel..67bb0b330583869795d54c71b6052bfb26778929

[Robie Basak (racb)]

Utkarsh got here before me, so I won't stop for a full review. However, +1 on the commit graph.

[Paride Legovini (paride)]

Per post-standup discussion I'm dropping the merge commit, leaving only my changes in the MP.

[Christian Ehrhardt  (paelzer)]

Re-adding canonical-server slot which was consumed by accident

[Christian Ehrhardt  (paelzer)]

It is somewhat puzzling to read the explanation that the destructor will make this a double free as you'd expect the destructor to also check the pointers validity right?
Like the removed
- if (gse_ctx->k5ctx) {
- krb5_free_context(gse_ctx->k5ctx);

Naively I'd expect the same check to prevent a double free in the dtor in TALLOC_FREE(gse_ctx);

Checking the destructor (same file gse_context_destructor) shows that it indeed checks for gse_ctx->k5ctx, but other than the pure free which the patch removes it does check and close potentially existing caches and keytabs.
And it does an explicit set to NULL after the free.
If the set to NULL is needed then the dtor will have run into the crash while trying to clean the rest - and the extra checks and cleanups on caches and keytabs seem right.

So TL;DR: this looks good but not to be a "fix double free" instead it seems to be more like "clean sub-elements before free and mark NULL afterwards". But at this point it becomes bike-shedding, my initial concerns are resolved and I think this is ok.

Changelog issues raised by Utkarsh are resolved as well by now.
The SRU test plan is slightly weak missing an explicit testcase, but that is for the SRU Team to discuss and the fix seems fine.

+1 and sponsoring

review: Approve

[Christian Ehrhardt  (paelzer)]

To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/samba
 * [new tag] upload/2%4.11.6+dfsg-0ubuntu1.10 -> upload/2%4.11.6+dfsg-0ubuntu1.10

Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading samba_4.11.6+dfsg-0ubuntu1.10.dsc: done.
  Uploading samba_4.11.6+dfsg-0ubuntu1.10.debian.tar.xz: done.
  Uploading samba_4.11.6+dfsg-0ubuntu1.10_source.buildinfo: done.
  Uploading samba_4.11.6+dfsg-0ubuntu1.10_source.changes: done.
Successfully uploaded packages.

diff --git a/debian/changelog b/debian/changelog
index 96d164d..95fd810 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+samba (2:4.11.6+dfsg-0ubuntu1.10) focal; urgency=medium
+
+  * d/p/fix-double-free-with-unresolved-credentia-cache.patch: Fix
+    double free with unresolved credential cache. (LP: #1892145)
+
+ -- Paride Legovini <[e-mail]>  Fri, 06 Aug 2021 14:17:29 +0200
+
 samba (2:4.11.6+dfsg-0ubuntu1.9) focal; urgency=medium
 
   * Fix samba-common-bin postinst errors (LP: #1905387)
diff --git a/debian/patches/fix-double-free-with-unresolved-credentia-cache.patch b/debian/patches/fix-double-free-with-unresolved-credentia-cache.patch
new file mode 100644
index 0000000..f7370f2
--- /dev/null
+++ b/debian/patches/fix-double-free-with-unresolved-credentia-cache.patch
@@ -0,0 +1,83 @@
+Bug: https://bugzilla.samba.org/show_bug.cgi?id=14344
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/focal/+source/samba/+bug/1892145
+Origin: upstream, https://git.samba.org/samba.git/?p=samba.git;a=patch;h=34f8ab774d1484b0e60
+Applied-Upstream: 4.11.9, 4.12.3, 4.13.0
+From: Noel Power <[e-mail]>
+Date: Tue, 14 Apr 2020 11:21:22 +0100
+Subject: [PATCH] s3/librpc/crypto: Fix double free with unresolved credential
+ cache
+
+We free gse_ctx->k5ctx but then free it again in the
+talloc dtor. This patch just lets the talloc dtor handle
+things and removes the extra krb5_free_context
+
+Failed to resolve credential cache 'DIR:/run/user/1000/krb5cc'! (No credentials cache found)
+==30762== Invalid read of size 8
+==30762==    at 0x108100F4: k5_os_free_context (in /usr/lib64/libkrb5.so.3.3)
+==30762==    by 0x107EA661: krb5_free_context (in /usr/lib64/libkrb5.so.3.3)
+==30762==    by 0x7945D2E: gse_context_destructor (gse.c:84)
+==30762==    by 0x645FB49: _tc_free_internal (talloc.c:1157)
+==30762==    by 0x645FEC5: _talloc_free_internal (talloc.c:1247)
+==30762==    by 0x646118D: _talloc_free (talloc.c:1789)
+==30762==    by 0x79462E4: gse_context_init (gse.c:241)
+==30762==    by 0x794636E: gse_init_client (gse.c:268)
+==30762==    by 0x7947602: gensec_gse_client_start (gse.c:786)
+==30762==    by 0xBC87A3A: gensec_start_mech (gensec_start.c:743)
+==30762==    by 0xBC87BC6: gensec_start_mech_by_ops (gensec_start.c:774)
+==30762==    by 0xBC8167F: gensec_spnego_client_negTokenInit_step (spnego.c:633)
+==30762==  Address 0x17259928 is 40 bytes inside a block of size 496 free'd
+==30762==    at 0x4C2F50B: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
+==30762==    by 0x79462CA: gse_context_init (gse.c:238)
+==30762==    by 0x794636E: gse_init_client (gse.c:268)
+==30762==    by 0x7947602: gensec_gse_client_start (gse.c:786)
+==30762==    by 0xBC87A3A: gensec_start_mech (gensec_start.c:743)
+==30762==    by 0xBC87BC6: gensec_start_mech_by_ops (gensec_start.c:774)
+==30762==    by 0xBC8167F: gensec_spnego_client_negTokenInit_step (spnego.c:633)
+==30762==    by 0xBC813E2: gensec_spnego_client_negTokenInit_start (spnego.c:537)
+==30762==    by 0xBC84084: gensec_spnego_update_pre (spnego.c:1943)
+==30762==    by 0xBC83AE5: gensec_spnego_update_send (spnego.c:1741)
+==30762==    by 0xBC85622: gensec_update_send (gensec.c:449)
+==30762==    by 0x551BFD0: cli_session_setup_gensec_local_next (cliconnect.c:997)
+==30762==  Block was alloc'd at
+==30762==    at 0x4C306B5: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
+==30762==    by 0x107EA7AE: krb5_init_context_profile (in /usr/lib64/libkrb5.so.3.3)
+==30762==    by 0xB853215: smb_krb5_init_context_common (krb5_samba.c:3597)
+==30762==    by 0x794615B: gse_context_init (gse.c:209)
+==30762==    by 0x794636E: gse_init_client (gse.c:268)
+==30762==    by 0x7947602: gensec_gse_client_start (gse.c:786)
+==30762==    by 0xBC87A3A: gensec_start_mech (gensec_start.c:743)
+==30762==    by 0xBC87BC6: gensec_start_mech_by_ops (gensec_start.c:774)
+==30762==    by 0xBC8167F: gensec_spnego_client_negTokenInit_step (spnego.c:633)
+==30762==    by 0xBC813E2: gensec_spnego_client_negTokenInit_start (spnego.c:537)
+==30762==    by 0xBC84084: gensec_spnego_update_pre (spnego.c:1943)
+==30762==    by 0xBC83AE5: gensec_spnego_update_send (spnego.c:1741)
+==30762==
+
+BUG: https://bugzilla.samba.org/show_bug.cgi?id=14344
+Signed-off-by: Noel Power <[e-mail]>
+Reviewed-by: Volker Lendecke <[e-mail]>
+
+Autobuild-User(master): Noel Power <[e-mail]>
+Autobuild-Date(master): Tue Apr 14 22:55:51 UTC 2020 on sn-devel-184
+---
+ source3/librpc/crypto/gse.c | 4 ----
+ 1 file changed, 4 deletions(-)
+
+diff --git a/source3/librpc/crypto/gse.c b/source3/librpc/crypto/gse.c
+index 6675f4dc597..1cf111bd974 100644
+--- a/source3/librpc/crypto/gse.c
++++ b/source3/librpc/crypto/gse.c
+@@ -244,10 +244,6 @@ static NTSTATUS gse_context_init(TALLOC_CTX *mem_ctx,
+ 	return NT_STATUS_OK;
+ 
+ err_out:
+-	if (gse_ctx->k5ctx) {
+-		krb5_free_context(gse_ctx->k5ctx);
+-	}
+-
+ 	TALLOC_FREE(gse_ctx);
+ 	return status;
+ }
+-- 
+2.25.1
+
diff --git a/debian/patches/series b/debian/patches/series
index 2724df2..8bf4353 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -66,3 +66,4 @@ CVE-2020-14323-2.patch
 CVE-2020-14383-1.patch
 CVE-2020-14383-2.patch
 CVE-2021-20254.patch
+fix-double-free-with-unresolved-credentia-cache.patch