Bug 15782 - winbindd shows memleak in kerberos_decode_pac
Summary: winbindd shows memleak in kerberos_decode_pac
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Winbind (show other bugs)
Version: 4.21.3
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-01-15 07:28 UTC by Subba Ramanna Bodda
Modified: 2025-02-05 08:34 UTC (History)
2 users (show)

See Also:


Attachments
patch from master for v4-21-test (9.32 KB, patch)
2025-01-17 15:05 UTC, Guenther Deschner
gd: review? (jra)
ambi: review+
gd: ci-passed+
Details
patch from master for v4-20-test (9.32 KB, patch)
2025-01-17 15:07 UTC, Guenther Deschner
gd: review? (jra)
ambi: review+
gd: ci-passed+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Subba Ramanna Bodda 2025-01-15 07:28:35 UTC
We have seen repeated messages in logs on one of the clusters as below.

[2025/01/13 11:21:04.083100,  5, pid=1481584, effective(0, 0), real(0, 0), class=auth] ../../auth/kerberos/kerberos_pac.c:352(kerberos_decode_pac)
  PAC Decode: Failed to verify the service signature: Invalid argument

And the nodes show winbindd memory increasing every few minutes in top.

We have seen this for 4.17.10, below code seems to possibly cause memleak. The code is same in master and other branches as well.

Everywhere else, status is returned, we see talloc_free(tmp_ctx), but after printing "PAC Decode: Failed to verify the service signature", it does not have that.

auth/kerberos/kerberos_pac.c
=============================

NTSTATUS kerberos_decode_pac(TALLOC_CTX *mem_ctx,
...
...
        if (service_keyblock) {
                /* verify by service_key */
                ret = check_pac_checksum(modified_pac_blob, srv_sig_ptr,
                                         context,
                                         service_keyblock);
                if (ret) {
                        DEBUG(5, ("PAC Decode: Failed to verify the service "
                                  "signature: %s\n", error_message(ret)));
                        return NT_STATUS_ACCESS_DENIED;
                }

                if (krbtgt_keyblock) {
                        /* verify the service key checksum by krbtgt_key */
                        ret = check_pac_checksum(srv_sig_ptr->signature, kdc_sig_ptr,
                                                 context, krbtgt_keyblock);
                        if (ret) {
                                DEBUG(1, ("PAC Decode: Failed to verify the KDC signature: %s\n",
                                          smb_get_krb5_error_message(context, ret, tmp_ctx)));
                                talloc_free(tmp_ctx);
                                return NT_STATUS_ACCESS_DENIED;
                        }
                }
        }
...
...
Comment 1 Samba QA Contact 2025-01-16 14:33:03 UTC
This bug was referenced in samba master:

c514ce8dcadcbbf0d86f3038d2be0f9253a76b75
Comment 2 Christian Ambach 2025-01-16 14:58:28 UTC
Good observation about the missing free of the temporary context, however as tmp_ctx is a child of men_ctx given to the function, it should be freed once the context given to the function is freed.

As it seems to only happen in the error path, the error path of the callers needs to be examined as well to see if there is a missing free there.
Comment 3 Samba QA Contact 2025-01-17 12:02:04 UTC
This bug was referenced in samba master:

f9eb0b248da0689c82656f3e482161c45749afb6
Comment 4 Guenther Deschner 2025-01-17 15:05:47 UTC
Created attachment 18531 [details]
patch from master for v4-21-test
Comment 5 Guenther Deschner 2025-01-17 15:07:03 UTC
Created attachment 18532 [details]
patch from master for v4-20-test
Comment 6 Subba Ramanna Bodda 2025-01-17 18:42:13 UTC
Thanks for the quick help. Sorry for my oversight and also for not completing my testing and updating more early here. Like mentioned here, the memleak might not be due to this missing talloc_free, and did not go away until I added one more fix "48493735e2d2091740fe784cf07a4258dfc0b512 - s3: winbindd: winbindd_pam: fix leak in extract_pac_vrfy_sigs". I was testing 4.17.10 and 4.17.12 and this additional fix is only in master and 4.21. No other version has it.

This ticket and version is OK code change wise, but we might have to ignore the memleak part of it.
Comment 7 Guenther Deschner 2025-01-30 13:36:38 UTC
Jule, please add to the next v4-20 and v4-21 releases, thanks!
Comment 8 Jule Anger 2025-02-03 14:46:52 UTC
Pushed to autobuild-v4-{21,20}-test.
Comment 9 Samba QA Contact 2025-02-03 15:54:03 UTC
This bug was referenced in samba v4-20-test:

79ca540b0a6cf70b0b04f76d5a2865406e89ad63
a0978446adbc3ea3b96b87180938312e8ab63dfe
Comment 10 Samba QA Contact 2025-02-03 20:02:12 UTC
This bug was referenced in samba v4-21-test:

0387515d687568a8d36e30d9e10d03d2b497e012
1c9a1d5fd11219f3e593596b3231aab51f349c7e
Comment 11 Jule Anger 2025-02-05 08:34:22 UTC
Closing out bug report.

Thanks!