Bug 14658 - string_sub_talloc can easily crash; use talloc_string_sub instead
Summary: string_sub_talloc can easily crash; use talloc_string_sub instead
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Samba QA Contact
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-03-10 01:53 UTC by Douglas Bagnall
Modified: 2021-03-11 02:59 UTC (History)
0 users

See Also:


Attachments
proof of abort (1.51 KB, patch)
2021-03-10 01:53 UTC, Douglas Bagnall
no flags Details
raw patch to remove lpcfg_add_home() (2.09 KB, patch)
2021-03-10 02:24 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Douglas Bagnall 2021-03-10 01:53:06 UTC
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.
Comment 1 Douglas Bagnall 2021-03-10 02:00:51 UTC
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.
Comment 2 Jeremy Allison 2021-03-10 02:13:28 UTC
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 ?
Comment 3 Douglas Bagnall 2021-03-10 02:23:28 UTC
(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.
Comment 4 Jeremy Allison 2021-03-10 02:24:03 UTC
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 ?
Comment 5 Jeremy Allison 2021-03-10 02:25:48 UTC
So we can ditch string_sub_talloc()+tests *and* lpcfg_add_home() and the problem is gone ?

Sounds like a plan to me :-) :-).
Comment 6 Andrew Bartlett 2021-03-10 03:30:58 UTC
MR created per your request: https://gitlab.com/samba-team/samba/-/merge_requests/1829
Comment 7 Samba QA Contact 2021-03-10 08:07:05 UTC
This bug was referenced in samba master:

202d4d6da6a1923e768799b576acf5057473a7f7
d7e620ff41d6583b5554c03abaac6c4c183d5146