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

Proposed by Paride Legovini
Status: Merged
Merged at revision: 3b9b57b5871a5f81136446c10d9b1a518cb29b10
Proposed branch: ~paride/ubuntu/+source/samba:lp1892145-focal
Merge into: ubuntu/+source/samba:ubuntu/focal-devel
Diff against target: 113 lines (+91/-0)
3 files modified
debian/changelog (+7/-0)
debian/patches/fix-double-free-with-unresolved-credentia-cache.patch (+83/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Christian Ehrhardt  (community) Approve
Utkarsh Gupta (community) Needs Fixing
Canonical Server Team Pending
Review via email: mp+406751@code.launchpad.net

Commit message

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.

To post a comment you must log in.
Revision history for this message
Utkarsh Gupta (utkarsh) wrote :

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
Revision history for this message
Paride Legovini (paride) wrote :

Thanks! Replied inline, updating my branch.

Revision history for this message
Paride Legovini (paride) wrote :

@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.

Revision history for this message
Paride Legovini (paride) wrote :

I verified that the following is empty:

  git diff ubuntu/focal-devel..67bb0b330583869795d54c71b6052bfb26778929

Revision history for this message
Robie Basak (racb) wrote :

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

Revision history for this message
Paride Legovini (paride) wrote :

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

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

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

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

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
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

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.

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 96d164d..95fd810 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,10 @@
6+samba (2:4.11.6+dfsg-0ubuntu1.10) focal; urgency=medium
7+
8+ * d/p/fix-double-free-with-unresolved-credentia-cache.patch: Fix
9+ double free with unresolved credential cache. (LP: #1892145)
10+
11+ -- Paride Legovini <paride@ubuntu.com> Fri, 06 Aug 2021 14:17:29 +0200
12+
13 samba (2:4.11.6+dfsg-0ubuntu1.9) focal; urgency=medium
14
15 * Fix samba-common-bin postinst errors (LP: #1905387)
16diff --git a/debian/patches/fix-double-free-with-unresolved-credentia-cache.patch b/debian/patches/fix-double-free-with-unresolved-credentia-cache.patch
17new file mode 100644
18index 0000000..f7370f2
19--- /dev/null
20+++ b/debian/patches/fix-double-free-with-unresolved-credentia-cache.patch
21@@ -0,0 +1,83 @@
22+Bug: https://bugzilla.samba.org/show_bug.cgi?id=14344
23+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/focal/+source/samba/+bug/1892145
24+Origin: upstream, https://git.samba.org/samba.git/?p=samba.git;a=patch;h=34f8ab774d1484b0e60
25+Applied-Upstream: 4.11.9, 4.12.3, 4.13.0
26+From: Noel Power <noel.power@suse.com>
27+Date: Tue, 14 Apr 2020 11:21:22 +0100
28+Subject: [PATCH] s3/librpc/crypto: Fix double free with unresolved credential
29+ cache
30+
31+We free gse_ctx->k5ctx but then free it again in the
32+talloc dtor. This patch just lets the talloc dtor handle
33+things and removes the extra krb5_free_context
34+
35+Failed to resolve credential cache 'DIR:/run/user/1000/krb5cc'! (No credentials cache found)
36+==30762== Invalid read of size 8
37+==30762== at 0x108100F4: k5_os_free_context (in /usr/lib64/libkrb5.so.3.3)
38+==30762== by 0x107EA661: krb5_free_context (in /usr/lib64/libkrb5.so.3.3)
39+==30762== by 0x7945D2E: gse_context_destructor (gse.c:84)
40+==30762== by 0x645FB49: _tc_free_internal (talloc.c:1157)
41+==30762== by 0x645FEC5: _talloc_free_internal (talloc.c:1247)
42+==30762== by 0x646118D: _talloc_free (talloc.c:1789)
43+==30762== by 0x79462E4: gse_context_init (gse.c:241)
44+==30762== by 0x794636E: gse_init_client (gse.c:268)
45+==30762== by 0x7947602: gensec_gse_client_start (gse.c:786)
46+==30762== by 0xBC87A3A: gensec_start_mech (gensec_start.c:743)
47+==30762== by 0xBC87BC6: gensec_start_mech_by_ops (gensec_start.c:774)
48+==30762== by 0xBC8167F: gensec_spnego_client_negTokenInit_step (spnego.c:633)
49+==30762== Address 0x17259928 is 40 bytes inside a block of size 496 free'd
50+==30762== at 0x4C2F50B: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
51+==30762== by 0x79462CA: gse_context_init (gse.c:238)
52+==30762== by 0x794636E: gse_init_client (gse.c:268)
53+==30762== by 0x7947602: gensec_gse_client_start (gse.c:786)
54+==30762== by 0xBC87A3A: gensec_start_mech (gensec_start.c:743)
55+==30762== by 0xBC87BC6: gensec_start_mech_by_ops (gensec_start.c:774)
56+==30762== by 0xBC8167F: gensec_spnego_client_negTokenInit_step (spnego.c:633)
57+==30762== by 0xBC813E2: gensec_spnego_client_negTokenInit_start (spnego.c:537)
58+==30762== by 0xBC84084: gensec_spnego_update_pre (spnego.c:1943)
59+==30762== by 0xBC83AE5: gensec_spnego_update_send (spnego.c:1741)
60+==30762== by 0xBC85622: gensec_update_send (gensec.c:449)
61+==30762== by 0x551BFD0: cli_session_setup_gensec_local_next (cliconnect.c:997)
62+==30762== Block was alloc'd at
63+==30762== at 0x4C306B5: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
64+==30762== by 0x107EA7AE: krb5_init_context_profile (in /usr/lib64/libkrb5.so.3.3)
65+==30762== by 0xB853215: smb_krb5_init_context_common (krb5_samba.c:3597)
66+==30762== by 0x794615B: gse_context_init (gse.c:209)
67+==30762== by 0x794636E: gse_init_client (gse.c:268)
68+==30762== by 0x7947602: gensec_gse_client_start (gse.c:786)
69+==30762== by 0xBC87A3A: gensec_start_mech (gensec_start.c:743)
70+==30762== by 0xBC87BC6: gensec_start_mech_by_ops (gensec_start.c:774)
71+==30762== by 0xBC8167F: gensec_spnego_client_negTokenInit_step (spnego.c:633)
72+==30762== by 0xBC813E2: gensec_spnego_client_negTokenInit_start (spnego.c:537)
73+==30762== by 0xBC84084: gensec_spnego_update_pre (spnego.c:1943)
74+==30762== by 0xBC83AE5: gensec_spnego_update_send (spnego.c:1741)
75+==30762==
76+
77+BUG: https://bugzilla.samba.org/show_bug.cgi?id=14344
78+Signed-off-by: Noel Power <noel.power@suse.com>
79+Reviewed-by: Volker Lendecke <vl@samba.org>
80+
81+Autobuild-User(master): Noel Power <npower@samba.org>
82+Autobuild-Date(master): Tue Apr 14 22:55:51 UTC 2020 on sn-devel-184
83+---
84+ source3/librpc/crypto/gse.c | 4 ----
85+ 1 file changed, 4 deletions(-)
86+
87+diff --git a/source3/librpc/crypto/gse.c b/source3/librpc/crypto/gse.c
88+index 6675f4dc597..1cf111bd974 100644
89+--- a/source3/librpc/crypto/gse.c
90++++ b/source3/librpc/crypto/gse.c
91+@@ -244,10 +244,6 @@ static NTSTATUS gse_context_init(TALLOC_CTX *mem_ctx,
92+ return NT_STATUS_OK;
93+
94+ err_out:
95+- if (gse_ctx->k5ctx) {
96+- krb5_free_context(gse_ctx->k5ctx);
97+- }
98+-
99+ TALLOC_FREE(gse_ctx);
100+ return status;
101+ }
102+--
103+2.25.1
104+
105diff --git a/debian/patches/series b/debian/patches/series
106index 2724df2..8bf4353 100644
107--- a/debian/patches/series
108+++ b/debian/patches/series
109@@ -66,3 +66,4 @@ CVE-2020-14323-2.patch
110 CVE-2020-14383-1.patch
111 CVE-2020-14383-2.patch
112 CVE-2021-20254.patch
113+fix-double-free-with-unresolved-credentia-cache.patch

Subscribers

People subscribed via source and target branches