Bug 13420 - Use after free in AD DC LSA server (inter-forest trust changes)
Use after free in AD DC LSA server (inter-forest trust changes)
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB
unspecified
All All
: P5 regression
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks: 13286
  Show dependency treegraph
 
Reported: 2018-05-03 04:21 UTC by Andrew Bartlett
Modified: 2018-05-17 11:33 UTC (History)
6 users (show)

See Also:


Attachments
patch for master (1.48 KB, patch)
2018-05-03 04:23 UTC, Andrew Bartlett
no flags Details
backtrace from addressanitiser (25.15 KB, text/plain)
2018-05-03 08:13 UTC, Andrew Bartlett
no flags Details
backported patch for 4.8 (1.66 KB, patch)
2018-05-10 02:24 UTC, Andrew Bartlett
metze: review-
Details
Additional patch for master (6.67 KB, patch)
2018-05-11 04:58 UTC, Stefan Metzmacher
no flags Details
Additional patch for master (3.16 KB, text/plain)
2018-05-11 08:44 UTC, Stefan Metzmacher
abartlet: review+
metze: review? (garming)
Details
patch for 4.8 cherry-picked from master (5.08 KB, patch)
2018-05-13 08:55 UTC, Andrew Bartlett
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2018-05-03 04:21:19 UTC
commit ab7988aa2fd1a43f576a4b73a6893c61c7ef1957
Author: Stefan Metzmacher <metze@samba.org>
Date:   Fri Jan 19 13:42:40 2018 +0100

    s4:rpc_server/lsa: prepare dcesrv_lsa_LookupSids* for async processing
    
    Bug: https://bugzilla.samba.org/show_bug.cgi?id=13286
    
    Signed-off-by: Stefan Metzmacher <metze@samba.org>
    Reviewed-by: Ralph Boehme <slow@samba.org>

Introduced a use-after-free as the state variable contains the data to be returned to the client and packed into NDR after the function returned. 

This memory needs to be kept (on mem_ctx as parent) until that is pushed and freed by the caller.
Comment 1 Andrew Bartlett 2018-05-03 04:23:24 UTC
Created attachment 14172 [details]
patch for master
Comment 2 Stefan Metzmacher 2018-05-03 07:52:02 UTC
(In reply to Andrew Bartlett from comment #0)

Can you please upload backtraces (or what ever address-sanatizer produces)
it would be good to understand what went wrong.

It's ok to have the removal of TALLOC_FREE(state) as immediate fix,
but I'd like to follow up with a proper fix.
Comment 3 Andrew Bartlett 2018-05-03 08:13:41 UTC
Created attachment 14174 [details]
backtrace from addressanitiser
Comment 4 Andrew Bartlett 2018-05-10 02:24:32 UTC
Created attachment 14192 [details]
backported patch for 4.8
Comment 5 Stefan Metzmacher 2018-05-11 04:53:33 UTC
Comment on attachment 14192 [details]
backported patch for 4.8

I think this is incomplete, I'll upload an additional patch for master...
Comment 6 Stefan Metzmacher 2018-05-11 04:58:45 UTC
Created attachment 14195 [details]
Additional patch for master
Comment 7 Andrew Bartlett 2018-05-11 06:49:57 UTC
(In reply to Stefan Metzmacher from comment #6)
This is an NTLMSSP patch, not a LSA server patch
Comment 8 Stefan Metzmacher 2018-05-11 08:44:54 UTC
Created attachment 14196 [details]
Additional patch for master

Should be the correct patch now, sorry...
Comment 9 Andrew Bartlett 2018-05-11 19:30:21 UTC
Comment on attachment 14196 [details]
Additional patch for master

Looks good to me, and confirmed with AddressSanitizer.

Reviewed-by: Andrew Bartlett <abartlet@samba.org>

Please push!
Comment 10 Andrew Bartlett 2018-05-13 08:54:10 UTC
Fixed in master as 9a513304adadd79d1c63d55fcf06b67ed45d43ba for Samba 4.9.
Comment 11 Andrew Bartlett 2018-05-13 08:55:05 UTC
Created attachment 14198 [details]
patch for 4.8 cherry-picked from master
Comment 12 Karolin Seeger 2018-05-14 07:52:42 UTC
(In reply to Andrew Bartlett from comment #11)
Pushed to autobuild-v4-8-test.
Comment 13 Karolin Seeger 2018-05-17 11:33:32 UTC
(In reply to Karolin Seeger from comment #12)
Pushed to v4-8-test.
Closing out bug report.

Thanks!