Created attachment 16513 [details] proof of abort string_sub_talloc() makes simplistic calculations about the length of the eventual string, then tallocs a copy to send to string_sub2(), which arranges the string according to a more complicated algorithm. If the two lengths differ, either we go over the edge, or we abort, asserting that a null termination should be where it isn't (as seen in the attached demonstration). Apart from tests, this is only used in lib/param/loadparm.c, in lpcfg_add_home() I don't know whether the substitution there: service->path = string_sub_talloc(service, lpcfg_path(default_service, lp_ctx->sDefault, service), "%H", pszHomedir); is vulnerable or not. We should get rid of string_sub_talloc() and only use talloc_string_sub() which seems to work properly. The signatures and intentions are the same. Only the build system stands in our way. found with the help of Honggfuzz.
Possibly string_sub2() is at fault in the attached example -- there perhaps ought to have been a substitution as string_sub_talloc() expected. But the real flaw is attempting the same calculation in different ways. talloc_string_sub() -- the other one -- is tallocy all the way through, so it can talloc_realloc when it finds it got things wrong. Note also, nobody much uses string_sub or string_sub2 from lib/util/substitute.c. Maybe we could settle on the source3 ones in all cases.
Can you create a patch that moves talloc_string_sub() and any dependencies out of source3 and into lib/ so we can just use that instead ?
(In reply to Jeremy Allison from comment #2) Andrew points out that we can just get rid of it, since the caller lpcfg_add_home() is unused. Also, if it was used, it would only be for the ntvfs server.
Created attachment 16514 [details] raw patch to remove lpcfg_add_home() I can't actually find where lpcfg_add_home() is used at all... The following (raw) patch still compiles for me. Am I missing something ?
So we can ditch string_sub_talloc()+tests *and* lpcfg_add_home() and the problem is gone ? Sounds like a plan to me :-) :-).
MR created per your request: https://gitlab.com/samba-team/samba/-/merge_requests/1829
This bug was referenced in samba master: 202d4d6da6a1923e768799b576acf5057473a7f7 d7e620ff41d6583b5554c03abaac6c4c183d5146