Bug 12748 - use of wrong buffer in cleanupdb_store_child() leads to stack overread
Summary: use of wrong buffer in cleanupdb_store_child() leads to stack overread
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: 4.6.2
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
Depends on:
Reported: 2017-04-19 09:00 UTC by Hanno Böck
Modified: 2020-12-11 13:27 UTC (History)
2 users (show)

See Also:

patch with fix (613 bytes, patch)
2017-04-19 09:00 UTC, Hanno Böck
no flags Details
address sanitizer stack trace (3.43 KB, text/plain)
2017-04-19 09:02 UTC, Hanno Böck
no flags Details
Patch in git-am format (1.11 KB, patch)
2017-04-19 12:04 UTC, Volker Lendecke
no flags Details
git-am fix for 4.6.next, 4.5.next. (1.29 KB, patch)
2017-04-20 23:25 UTC, Jeremy Allison
slow: review+

Note You need to log in before you can comment on or make changes to this bug.
Description Hanno Böck 2017-04-19 09:00:40 UTC
Created attachment 13161 [details]
patch with fix

I discovered a buffer overread detectable with address sanitizer that shows up when one tries to start a samba server and run smbclient -L against it.

The error is in these lines of cleanupdb_store_child (or at least... I'm 90% sure):

	TDB_DATA tdbkey = { .dptr = (uint8_t *)&key, .dsize = sizeof(key) };
	TDB_DATA tdbdata = { .dptr = (uint8_t *)&key, .dsize = sizeof(rec) };

So the idea here is probably to have a pointer to a buffer and then the size of the buffer in each TDB_DATA struct.

However as you can see both lines point to the key variable, yet the second one has sizeof(rec). Thus this doesn't match. The second line needs to point to &rec instead of &key.

Patch attached.

This bug will subsequently lead to an 8 byte buffer overread in tdb_write (part of tdb, not samba itself). I'll attach a stack trace as well.
Comment 1 Hanno Böck 2017-04-19 09:02:40 UTC
Created attachment 13162 [details]
address sanitizer stack trace
Comment 2 Volker Lendecke 2017-04-19 12:04:24 UTC
Created attachment 13163 [details]
Patch in git-am format

100% correct. Attached find a patch with the formalisms we use. I've added your Signed-off-by: line, hoping that is okay.
Comment 3 Volker Lendecke 2017-04-19 12:05:23 UTC
Ralph, git blame points at you :-)

Can you take a quick look, this is a pretty obvious cut&paste error
Comment 4 Ralph Böhme 2017-04-19 12:12:24 UTC
D'oh! Looking...
Comment 5 Ralph Böhme 2017-04-19 12:15:53 UTC
Pushed. Thanks for finding this (Hanno) and for providing the patch (Volker). :)
Comment 6 Volker Lendecke 2017-04-19 12:38:01 UTC
Just to be precise: The patch was from Hanno, I just did the "git format-patch" icing.
Comment 7 Jeremy Allison 2017-04-20 23:25:36 UTC
Created attachment 13164 [details]
git-am fix for 4.6.next, 4.5.next.

Cherry-picked from master.
Comment 8 Ralph Böhme 2017-04-21 04:20:18 UTC
Reassigning to Karolin for inclusion in 4.5 and 4.6.
Comment 9 Karolin Seeger 2017-04-21 07:10:41 UTC
(In reply to Ralph Böhme from comment #8)
Pushed to autobuild-v4-{6,5}-test.
Comment 10 Karolin Seeger 2017-04-28 07:07:54 UTC
(In reply to Karolin Seeger from comment #9)
Pushed to both branches.
Closing out bug report.