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),
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: