Bug 10998 - Possible NULL-pointer dereference in lib/util/charset/pull_push.c
Summary: Possible NULL-pointer dereference in lib/util/charset/pull_push.c
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: 4.1.14
Hardware: All All
: P5 normal (vote)
Target Milestone: 4.3
Assignee: Andrew Bartlett
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-09 13:42 UTC by Torsten Kurbad
Modified: 2015-03-13 08:19 UTC (History)
3 users (show)

See Also:


Attachments
samba-4.x-pull_push.patch (397 bytes, patch)
2014-12-09 13:42 UTC, Torsten Kurbad
mdw: review-
Details
backtrace.log (37.48 KB, text/plain)
2014-12-09 16:37 UTC, Torsten Kurbad
no flags Details
backtrace.log (70.86 KB, text/plain)
2014-12-11 16:51 UTC, Torsten Kurbad
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Torsten Kurbad 2014-12-09 13:42:47 UTC
Created attachment 10512 [details]
samba-4.x-pull_push.patch

Hi Samba-team,

I reported the following issue on the samba-technical list a long time ago, and since it doesn't seem to get resolved, decided to file a bug about it.

Here are my original statements:

while trying to build a product on top of the samba Python modules, a
segmentation fault occured, whenever I tried to open a SamDB connection
to one of our DCs via ldb's ldap backend using invalid credentials.

Using valgrind, I tracked that down to a NULL-pointer dereference
occuring in lib/util/charset/pull_push.c, line 41 (Samba 4.0.8 code
base).

I somewhat circumvented the segfault by applying the attached patch.
However, I'm confident that there's a better way to deal with that,
i.e. preventing the occurence of NULL as the value of src right from
the start.

To see the issue "in action", one could run the Python script I uploaded to http://pastebin.com/r9jjfGiS against a freshly provisioned Samba 4. The script would have to be changed according to the provision (Domain DN, etc.). It then should be used to try to authenticate as 'Administrator' using the wrong password. A Python segfault will be the result.

I might also provide backtraces, etc., if necessary.

Best,
Torsten
Comment 1 Matthias Dieter Wallnöfer 2014-12-09 15:50:09 UTC
Comment on attachment 10512 [details]
samba-4.x-pull_push.patch

The patch might work but it cannot be applied like this. push_ucs2_talloc() has been written with src <> NULL in mind.

Yes, backtraces are highly appreciated. The best would be to recompile in development mode (./configure.developer) to have all debug symbols enabled, run the script under gdb (for instance gdb --python ...) and ask with "bt full" for the full backtrace after the segfault occurred.

This should make it easy to detect the cause.
Comment 2 Torsten Kurbad 2014-12-09 16:37:56 UTC
Created attachment 10513 [details]
backtrace.log

(In reply to Matthias Dieter Wallnöfer from comment #1)
> The patch might work but it cannot be applied like this. push_ucs2_talloc() has been written with src <> NULL in mind.

I thought so. :-) Unfortunately, my knowledge of C is very limited, so this was the only solution I could come up with.

Find attached a backtrace I made by request of Jelmer a while ago.

If you need more information, please let me know.

Best,
Torsten
Comment 3 Matthias Dieter Wallnöfer 2014-12-09 16:49:42 UTC
Comment on attachment 10513 [details]
backtrace.log

I see many "<optimized out>" directives. I suggest to recompile s4 with "./configure.developer" (before "make -j") to improve the backtrace output.
Comment 4 Torsten Kurbad 2014-12-10 18:01:12 UTC
(In reply to Matthias Dieter Wallnöfer from comment #3)
I'll try and rebuild with all debug symbols intact. However, this could take a while. I'll report back as soon as I've created a better backtrace.log.

Best,
Torsten
Comment 5 Matthias Dieter Wallnöfer 2014-12-11 10:11:04 UTC
auth/ntlmssp/ntlmssp_client.c lines 299ff:

        /* this generates the actual auth packet */
        nt_status = msrpc_gen(mem_ctx,
                       out, auth_gen_string,
                       "NTLMSSP",
                       NTLMSSP_AUTH,
                       lm_response.data, lm_response.length,
                       nt_response.data, nt_response.length,
                       domain,
                       user,
                       cli_credentials_get_workstation(gensec_security->credentials),
^^^ this one returns NULL
                       encrypted_session_key.data, encrypted_session_key.length,
                       ntlmssp_state->neg_flags);
        if (!NT_STATUS_IS_OK(nt_status)) {
                talloc_free(mem_ctx);
                return nt_status;
        }

I have managed to go a step ahead locating the function which returns NULL.

@abartlet: Maybe you are able to tell us what the cause of this could be?
Comment 6 Torsten Kurbad 2014-12-11 16:51:02 UTC
Created attachment 10526 [details]
backtrace.log

I now compiled python, talloc, tevent, ldb, tdb, ntdb and samba with CFLAGS="-O1 -ggdb".
In addition, I added --enable-developer --enable-debug to the configure options of each package.

The backtrace.log got rid of some, though not all, <optimized out> entries this way.

It contains the outputs of

backtrace full
info registers
thread apply all backtrace

I'm a bit lost now concerning the remaining <optimized out> entries, since I'm far from being a gdb expert...
Comment 7 Torsten Kurbad 2014-12-11 16:54:53 UTC
I forgot to mention, that I tried adding

'netbios name': '<Machine>'

to the lpOptions dict after Matthias' find. That didn't prevent the script from segfaulting, though.
Comment 8 Matthias Dieter Wallnöfer 2014-12-12 10:45:54 UTC
Comment on attachment 10526 [details]
backtrace.log

Frame #2

In the last stacktrace I see:
>        user = 0x249c190 "administrator"
>        domain = 0x249c280 "KMRC"

this time it is:
>        user = 0xf5e3f0 "tkurbad"
>        domain = 0x0

This means, that this time it could also segfault due to the missing domain. It would be good if some authentication expert could look at this bug. I hope that Andrew gets some time.