Bug 10933 - net ads join -k can segfault with existing keytab entries
Summary: net ads join -k can segfault with existing keytab entries
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Tools (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-12 16:25 UTC by Guenther Deschner
Modified: 2014-11-20 12:38 UTC (History)
1 user (show)

See Also:


Attachments
use talloc_zero_array for keytab array (1.10 KB, patch)
2014-11-12 16:28 UTC, Guenther Deschner
asn: review+
gd: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Guenther Deschner 2014-11-12 16:25:30 UTC
It's possible for ads_keytab_create_default() to pass an invalid pointer to ads_keytab_add_entry() at the following spot:

697             for (i = 0; oldEntries[i]; i++) {
698                     ret |= ads_keytab_add_entry(ads, oldEntries[i]);
699                     TALLOC_FREE(oldEntries[i]);
700             }

The reason this happens is due to the oldEntries array being allocated (and zeroed) only enough space for the amount of existing entries found, so for example with one existing entry, oldEntries[1] would not be guaranteed to be NULL

		oldEntries = talloc_array(tmpctx, char *, found);

		memset(oldEntries, '\0', found * sizeof(char *));
		
(gdb) x/32 oldEntries
0x7fdcaca42630: 0xaca41f00      0x00007fdc      0x00000021      0x00000000

(gdb) p oldEntries
$29 = (char **) 0x7fdcaca42630
(gdb) p oldEntries[0]
$30 = 0x7fdcaca41f00 "example-princ"
(gdb) p oldEntries[1]
$31 = 0x21 <Address 0x21 out of bounds>
Comment 1 Guenther Deschner 2014-11-12 16:26:47 UTC
See also: https://bugzilla.redhat.com/show_bug.cgi?id=1163383
Comment 2 Guenther Deschner 2014-11-12 16:28:11 UTC
Created attachment 10430 [details]
use talloc_zero_array for keytab array
Comment 3 Jeremy Allison 2014-11-12 17:51:52 UTC
Comment on attachment 10430 [details]
use talloc_zero_array for keytab array

(Not that I was asked) - but LGTM (and pushed to master :-).

Jeremy.
Comment 4 Guenther Deschner 2014-11-14 11:01:19 UTC
Karolin, patch is in master with 2 reviews 0de6799996955fbf8e19ace8c4b7b61f5a262cb5.

Can you please push to 4.1 and 4.2 ?

Thanks.
Comment 5 Karolin Seeger 2014-11-17 19:54:48 UTC
(In reply to Guenther Deschner from comment #4)
Cherry-picked and pushed to autobuild-v4-[1|2]-test.

Please note that the usual procedure is to add the patchsets including the bug number in the commit message to the bug report and add separate review flags, because the review is only valid for one branch. It might (and did) happen that a patch is valid for master, but wrong for e.g. v4-1-test although applying cleanly. Thanks!
Comment 6 Karolin Seeger 2014-11-18 20:16:26 UTC
Pushed to v4-2-test and v4-1-test.
Closing out bug report.

Thanks!