Bug 12505 - smbd will crash if keytab do not exist and SMB session is requested
Summary: smbd will crash if keytab do not exist and SMB session is requested
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: 4.4.8
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on: 10490
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-10 01:33 UTC by Jura Sasek
Modified: 2021-02-18 09:52 UTC (History)
3 users (show)

See Also:


Attachments
fix (425 bytes, patch)
2017-01-10 01:33 UTC, Jura Sasek
no flags Details
patch cherry-picked from master (1.12 KB, patch)
2021-02-10 00:57 UTC, Andrew Bartlett
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jura Sasek 2017-01-10 01:33:35 UTC
Created attachment 12808 [details]
fix

smbd dump core on krb5_storage_free(0)

core 'core' of 1856:    /usr/sbin/smbd -D
 f612e6fc __lwp_sigqueue (6, ffffffef, ffffffec, fc32a718, 5, 6) + 8
 f60aa26c abort    (802c, f6178270, 6, 1, f6179b98, 0) + 108
 f4d70254 smb_panic_s3 (822170, f5b562f4, fff933d0, f4dc8000, f4d5b510,
f5b562f4) + 264
 f5fb3ed8 sig_fault (1, f5fe0000, f4d6fff0, c000, 43000, c008) + 3f8
 f6129c1c __sighndlr (b, 0, fc32ac00, f5fb3ae0, 0, f6178270) + c
 f611c8f4 call_user_handler (0, 0, b, f617d000, f5932a40, fc32ac00) + 370
 f611cbfc sigacthandler (b, 0, fc32ac00, 0, 0, f6178270) + 58
 --- called from signal handler with signal 11 (SIGSEGV) ---
 f50f0df4 krb5_storage_free (0, 0, 1c, 1, f6182ae0, 8) + 4
 f50d4cc4 fkt_end_seq_get (823338, 820270, fc32b050, fc32b074, fc32b050,
f50d4cc0) + 4
 f519297c fill_mem_keytab_from_system_keytab (823338, fc32b07c, ffffffff,
fc32b028, fc32b07c, fc32b05c) + 8bc
 f5192f1c gse_krb5_get_server_keytab (823338, 822120, 0, fffca900, f51be000,
35400) + 30c
 f5193c04 gse_init_server (ffff, 12, 0, 0, fc32b43c, 8220f0) + 1e4
.
.
because of the null pointer redirection:
KRB5_LIB_FUNCTION krb5_error_code KRB5_LIB_CALL
krb5_storage_free(krb5_storage *sp)
{
    if(sp == NULL) return -1; <- this test should be added to avoid it off
    if(sp->free)
          ^^^here smbd crashes (in library private/libkrb5-samba4.so.26.0.0)
        (*sp->free)(sp);

...in file samba-4.4.8/source4/heimdal/lib/krb5/store.c
Comment 1 Jura Sasek 2017-08-12 21:45:47 UTC
Not yet fixed in trunk. What is the problem to add a check for null pointer to avoid of possible null pointer reference?
Comment 2 Andrew Bartlett 2017-08-14 09:16:56 UTC
(In reply to Jura Sasek from comment #1)
Can you get me the gdb 'bt full' backtrace?

In any case:

To fix it as proposed we first need to get such a fix upstream in Heimdal, or show it is already addressed there.  (Otherwise we risk loss of that fix if we ever manage an update). 

However, it might be that we just need to fix Samba not to free NULL.
Comment 3 Stefan Metzmacher 2017-08-14 09:34:29 UTC
(In reply to Jura Sasek from comment #1)

I'd guess that the fix for bug #10490 fixed this...
Can you try to verify that?

Thanks!
Comment 4 Paul Wise 2017-08-20 15:16:01 UTC
At my workplace we found a similar crash, but in winbindd when using AD authentication for SSH logins.

I note that #11824 is a similar bug report and probably a duplicate of this one.

I note that the NULL pointer dereference is already fixed in heimdal upstream, so the fix should be to sync samba's version of heimdal with the upstream heimdal, or enhance heimdal upstream so samba's version is no longer needed.

I note that the heimdal upstream fix returns 0 instead of -1 so it would probably be best to change heimdal upstream to return -1 instead.
Comment 5 Paul Wise 2017-08-20 15:50:09 UTC
I've verified that the heimdal upstream fix (similar to the one attached to this bug report, but returning 0 instead of -1) has fixed the crash that my workplace was experiencing.
Comment 6 Paul Wise 2017-08-20 15:54:26 UTC
I've filed a bug about removing or updating the samba copy of heimdal:

https://bugzilla.samba.org/show_bug.cgi?id=12976
Comment 7 Paul Wise 2021-01-29 05:26:58 UTC
For Debian bullseye, this is the only remaining patch we are carrying at work, so it would be nice to have it backported to 4.13.x and or any other earlier releases that are still supported by samba upstream.
Comment 8 Andrew Bartlett 2021-01-29 05:57:47 UTC
https://wiki.samba.org/index.php/Contribute#Submitting_Patches_via_GitLab

https://wiki.samba.org/index.php/Contribute#Bugzilla

Ideally find the change in upstream Heimdal and create a commit referencing that.
Comment 10 Samba QA Contact 2021-02-09 04:17:04 UTC
This bug was referenced in samba master:

f9ed4f7028a5ed29026ac8ef1b47b63755ba98f8
Comment 11 Andrew Bartlett 2021-02-10 00:57:52 UTC
Created attachment 16440 [details]
patch cherry-picked from master

Patch for 4.12, 4.13, 4.14 (and all other releases really, this hasn't changed in ages).
Comment 12 Jeremy Allison 2021-02-10 17:19:43 UTC
Comment on attachment 16440 [details]
patch cherry-picked from master

LGTM.
Comment 13 Jeremy Allison 2021-02-10 17:20:19 UTC
Re-assigning to Karolin for inclusion in all supported releases.
Comment 14 Karolin Seeger 2021-02-15 12:29:46 UTC
(In reply to Jeremy Allison from comment #13)
Pushed to autobuild-v4-{14,13,12}-test.
Comment 15 Samba QA Contact 2021-02-16 17:17:05 UTC
This bug was referenced in samba v4-13-test:

780fbc3004126175c66ec906910453aed866b163
Comment 16 Samba QA Contact 2021-02-16 18:28:13 UTC
This bug was referenced in samba v4-14-test:

a6f228f67549f2be71cbfb96e632f0d0aa495055
Comment 17 Samba QA Contact 2021-02-16 22:34:14 UTC
This bug was referenced in samba v4-12-test:

e80ef35f9356eadcaf6578a5b2c8d68acc45c172
Comment 18 Samba QA Contact 2021-02-18 09:52:56 UTC
This bug was referenced in samba v4-14-stable (Release samba-4.14.0rc3):

a6f228f67549f2be71cbfb96e632f0d0aa495055