Bug 13956 - idmap_tdb2 stops working after some time
idmap_tdb2 stops working after some time
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Winbind
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2019-05-20 15:32 UTC by Heinrich Mislik
Modified: 2019-06-20 09:47 UTC (History)
1 user (show)

See Also:


Attachments
log of winbindd under valgrind (43.43 KB, text/plain)
2019-05-22 11:51 UTC, Heinrich Mislik
no flags Details
git-am fix for master. (4.14 KB, patch)
2019-05-23 20:35 UTC, Jeremy Allison
no flags Details
git-am fix for 4.10.next, 4.9.next. (4.25 KB, patch)
2019-05-28 17:10 UTC, Jeremy Allison
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Heinrich Mislik 2019-05-20 15:32:33 UTC
Hello,

I am using idmap_tdb2 with a script to do mappings. After running winbind 2 or 3 days, new mappings no longer work. Restarting winbind resolves this. Samba log shows strange messages:

sh: $'\001': command not found
sh: al/bin/sfsh: No such file or directory
sh: /local/bin/sfsh: No such file or directory
sh: al/bin/sfsh: No such file or directory
sh: SIDTOID: command not found
sh: al/bin/sfsh: No such file or directory
sh: IDTOSID: command not found
sh: SIDTOID: command not found
sh: @�^: command not found
sh: $'\337U': command not found
sh: IDTOSID: command not found

Looking into idmap_tdb2.c I see, that a pointer to the script's path is stored in the private data during init. This pointer points into the currently loaded config. I could not prove or force this, but i assume, reloading the config makes this pointer invalid. Eventually the freed memory gets overwritten leading to the messages above. This may even be a security issue, since when a valid command is found at this location, it gets executed as root.

Additional note: return value of pclose should be checked.

Cheers

Heinrich
Comment 1 Jeremy Allison 2019-05-20 16:38:58 UTC
I don't see where this memory should be freed or reloaded. It's on a talloc context stored off the vector table for the methods along with other idmap data.

Can you catch this under valgrind to see where it might be being corrupted ?
Comment 2 Heinrich Mislik 2019-05-22 11:50:17 UTC
My config (idmap2.conf):

[global]
security = domain
workgroup = share
netbios name = share2
wins server = 131.130.1.111
client ipc signing = auto
idmap config * : backend = tdb2
idmap config * : range = 200-20000000
idmap config * : script = echo XXXXXXXXXXXXXXX 1>&2
idmap config * : read only = yes

log file = /tmp/samba
lock directory = /tmp
private dir = /tmp
pid directory = /tmp
cache directory = /tmp
state directory = /tmp

Doesn't do any useful mappings, but sufficient to prove memory corruption.

Commands:

valgrind winbindd -d 5 -is idmap2.conf >/tmp/winbind_valgrind.log 2>&1 &

wbinfo -U 1000
kill -HUP -$(</tmp/winbindd.pid)
wbinfo -U 1000

Logfile attached. Search for "Invalid read".

Cheers Heinrich
Comment 3 Heinrich Mislik 2019-05-22 11:51:26 UTC
Created attachment 15179 [details]
log of winbindd under valgrind
Comment 4 Jeremy Allison 2019-05-23 19:57:34 UTC
Helpful backtrace thanks ! Can you get me one with symbols loaded, as that would make it a little easier to track down ?
Comment 5 Jeremy Allison 2019-05-23 20:19:12 UTC
Never mind, I see the problem. I think the correct thing to do is to keep the script cache from the initial config by doing a talloc_strdup() instead of keeping a reference to the string, so any changes to the script are ignored on config file reload.

Restarting winbind will reload the script path.
Comment 6 Jeremy Allison 2019-05-23 20:35:53 UTC
Created attachment 15187 [details]
git-am fix for master.

Heinrich, can you confirm this fixes your problem ?

Thanks ! Jeremy.
Comment 7 Heinrich Mislik 2019-05-24 12:30:27 UTC
Problem fixed.

Thanks!

Heinrich
Comment 8 Jeremy Allison 2019-05-28 17:10:44 UTC
Created attachment 15196 [details]
git-am fix for 4.10.next, 4.9.next.

Cherry-picked from master, applies cleanly to 4.10.next, 4.9.next.
Comment 9 Ralph Böhme 2019-05-28 17:26:18 UTC
Reassigning to Karolin for inclusion in 4.9 and 4.10.
Comment 10 Karolin Seeger 2019-06-04 09:36:42 UTC
(In reply to Ralph Böhme from comment #9)
Pushed to autobuild-v4-{10,9}-test.
Comment 11 Karolin Seeger 2019-06-20 09:47:47 UTC
(In reply to Karolin Seeger from comment #10)
Pushed to both branches.
Closing out bug report.

Thanks!