Bug 8040 - smbclient segfaults when a Cyrillic netbios name or workgroup is configured
smbclient segfaults when a Cyrillic netbios name or workgroup is configured
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: libsmbclient
unspecified
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-25 10:22 UTC by David Disseldorp
Modified: 2011-04-04 18:50 UTC (History)
0 users

See Also:


Attachments
git-am fix for 3.5.next (2.61 KB, patch)
2011-03-25 22:38 UTC, Jeremy Allison
no flags Details
First part of fix. (2.54 KB, patch)
2011-04-01 18:16 UTC, Jeremy Allison
vl: review+
Details
Second part of fix. (1.68 KB, patch)
2011-04-01 18:48 UTC, Jeremy Allison
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Disseldorp 2011-03-25 10:22:15 UTC
Currently smbclient segfaults when marshalling an ntlmssp 
NEGOTIATE_MESSAGE if a wide character (e.g. Cyrillic) netbios name or
workgroup is configured in smb.conf.

/etc/samba/smb.conf:
...
[global]
        netbios name = нФЏБа
...

Core was generated by `/opt/samba/bin/smbclient -N -L 10.10.101.248'.
Program terminated with signal 11, Segmentation fault.
#0  0x00007f76fc7e05ab in memcpy () from /lib64/libc.so.6
(gdb) bt
#0  0x00007f76fc7e05ab in memcpy () from /lib64/libc.so.6
#1  0x0000000000525f9a in msrpc_gen (mem_ctx=0xe7dea0, blob=0x7fff849524b0,
format=0xa6ee32 "CddAA") at ../libcli/auth/msrpc_parse.c:156
#2  0x000000000051024b in ntlmssp_client_initial (ntlmssp_state=0xe7dea0,
out_mem_ctx=0xe7dea0, reply=..., next_request=0x7fff849524b0)
    at libsmb/ntlmssp.c:391
#3  0x000000000050febc in ntlmssp_update (ntlmssp_state=0xe7dea0, input=...,
out=0x7fff849524b0) at libsmb/ntlmssp.c:268
#4  0x00000000004caf00 in cli_session_setup_ntlmssp_send (mem_ctx=0xe7f350,
ev=0xe7f350, cli=0xe5d5b0, 
    user=0xe7e8b0 'E' <repeats 41 times>"\200, ", pass=0xe52cc0 "",
domain=0xe4c120 "WORKGROUP") at libsmb/cliconnect.c:1034
#5  0x00000000004cb6d1 in cli_session_setup_ntlmssp (cli=0xe5d5b0,
user=0xe7e8b0 'E' <repeats 41 times>"\200, ", pass=0xe52cc0 "", 
    domain=0xe4c120 "WORKGROUP") at libsmb/cliconnect.c:1178
#6  0x00000000004cc17f in cli_session_setup_spnego (cli=0xe5d5b0, user=0xe52c40
"root", pass=0xe52cc0 "", 
    user_domain=0xe4c120 "WORKGROUP", dest_realm=0x0) at
libsmb/cliconnect.c:1354
#7  0x00000000004cc57e in cli_session_setup (cli=0xe5d5b0, user=0xe52c40
"root", pass=0xe52cc0 "", passlen=0, ntpass=0xe52cc0 "", 
    ntpasslen=0, workgroup=0xe4c120 "WORKGROUP") at libsmb/cliconnect.c:1446
#8  0x00000000004faf4f in do_connect (ctx=0xe4c0f0, server=0xe52ba0
"10.10.101.248", share=0xa4d370 "IPC$", auth_info=0xe52740, 
    show_sessetup=true, force_encrypt=false, max_protocol=5, port=0,
name_type=32) at libsmb/clidfs.c:188
#9  0x00000000004fb584 in cli_cm_connect (ctx=0xe4c0f0, referring_cli=0x0,
server=0xe52ba0 "10.10.101.248", share=0xa4d370 "IPC$", 
    auth_info=0xe52740, show_hdr=true, force_encrypt=false, max_protocol=5,
port=0, name_type=32) at libsmb/clidfs.c:307
#10 0x00000000004fb90e in cli_cm_open (ctx=0xe4c0f0, referring_cli=0x0,
server=0xe52ba0 "10.10.101.248", share=0xa4d370 "IPC$", 
    auth_info=0xe52740, show_hdr=true, force_encrypt=false, max_protocol=5,
port=0, name_type=32) at libsmb/clidfs.c:401
#11 0x00000000004821b8 in do_host_query (query_host=0xe52ba0 "10.10.101.248")
at client/client.c:4942
#12 0x00000000004835ae in main (argc=4, argv=0x7fff849532b8) at
client/client.c:5369

push_ascii_talloc() fails to marshall the UTF netbios_name and returns an empty
string and zero length, return status is successful due to the
allow_bad_conv=true argument passed to the convert_string_talloc() helper
function.

On success as a null terminator is assumed to be present and included in the
length value. The zero length returned in this case is decremented which causes
the value to wrap.

The length is then used for a memcopy operation.
Comment 1 David Disseldorp 2011-03-25 10:24:01 UTC
A proposed fix has been sent to samba-technical:

http://lists.samba.org/archive/samba-technical/2011-March/076937.html
Comment 2 Jeremy Allison 2011-03-25 22:38:07 UTC
Created attachment 6346 [details]
git-am fix for 3.5.next

Here's the patch I added to v3-6-test. I think it fixes the root cause of the problem rather than patching the msrpc_gen() code. David - can you test this please ?

Thanks,

Jeremy.
Comment 3 David Disseldorp 2011-03-27 18:30:18 UTC
(In reply to comment #2)
> Created attachment 6346 [details]
> git-am fix for 3.5.next
> 
> Here's the patch I added to v3-6-test. I think it fixes the root cause of the
> problem rather than patching the msrpc_gen() code. David - can you test this
> please ?

The fix looks sane Jeremy, however a commit to cleanup convert_string_talloc() (15e84a9) which was pushed in the meantime uncovers further issues.

smbclient and smbd now panic in unix_strlower() following convert_string_talloc() failure.
set_local_machine_name() calls alpha_strcpy(), which from what I can gather is supposed to replace any characters not part of A-z0-9 or SAFE_NETBIOS_CHARS. This does not occur for the netbios name ТАНЦЕВАТЬ.

I'm currently testing with the following additional changes:

From 200dea0d11d76e04cdf495e9a1e127255a330265 Mon Sep 17 00:00:00 2001
From: David Disseldorp <ddiss@suse.de>
Date: Sun, 27 Mar 2011 19:26:08 +0200
Subject: [PATCH 1/2] s3-client: check for failures generating ntlmssp negotiate

Since commit 15e84a9 was pushed to cleanup convert_string_talloc(),
character conversion errors may propagate up to msrpc_gen() when a
wide-character netbios name or workgroup is configured.

These errors should not be ignored to avoid sending malformed ntlmssp
negotiate requests.
---
 source3/libsmb/ntlmssp.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/source3/libsmb/ntlmssp.c b/source3/libsmb/ntlmssp.c
index 7a006a3..9251cb7 100644
--- a/source3/libsmb/ntlmssp.c
+++ b/source3/libsmb/ntlmssp.c
@@ -377,6 +377,8 @@ static NTSTATUS ntlmssp_client_initial(struct ntlmssp_state *ntlmssp_state,
                                  TALLOC_CTX *out_mem_ctx, /* Unused at this time */
                                  DATA_BLOB reply, DATA_BLOB *next_request)
 {
+       bool ok;
+
        if (ntlmssp_state->unicode) {
                ntlmssp_state->neg_flags |= NTLMSSP_NEGOTIATE_UNICODE;
        } else {
@@ -388,12 +390,16 @@ static NTSTATUS ntlmssp_client_initial(struct ntlmssp_state *ntlmssp_state,
        }
 
        /* generate the ntlmssp negotiate packet */
-       msrpc_gen(ntlmssp_state, next_request, "CddAA",
-                 "NTLMSSP",
-                 NTLMSSP_NEGOTIATE,
-                 ntlmssp_state->neg_flags,
-                 ntlmssp_state->client.netbios_domain,
-                 ntlmssp_state->client.netbios_name);
+       ok = msrpc_gen(ntlmssp_state, next_request, "CddAA",
+                      "NTLMSSP",
+                      NTLMSSP_NEGOTIATE,
+                      ntlmssp_state->neg_flags,
+                      ntlmssp_state->client.netbios_domain,
+                      ntlmssp_state->client.netbios_name);
+       if (!ok) {
+               DEBUG(0, ("failed to generate ntlmssp negotiate packet\n"));
+               return NT_STATUS_INVALID_PARAMETER;
+       }
 
        if (DEBUGLEVEL >= 10) {
                struct NEGOTIATE_MESSAGE *negotiate = talloc(
-- 
1.7.3.4

From cee29a9d7f1cbd9a35b0e91c58ec1f2e46c62604 Mon Sep 17 00:00:00 2001
From: David Disseldorp <ddiss@suse.de>
Date: Sun, 27 Mar 2011 20:07:54 +0200
Subject: [PATCH 2/2] s3-charcnv: use isascii() in alpha_strcpy()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

alpha_strcpy() is a utility function which reportedly:
Strips out all but 'a-Z0-9' and the character in other_safe_chars and
replaces with '_'.
This statement does not currently hold true in all cases (e.g. src =
"ТАНЦЕВАТЬ").

Use C99 isascii() instead of the isupper_ascii() islower_ascii()
helpers to resolve this.
---
 source3/lib/util_str.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/source3/lib/util_str.c b/source3/lib/util_str.c
index 1996174..b7e8d9a 100644
--- a/source3/lib/util_str.c
+++ b/source3/lib/util_str.c
@@ -534,11 +534,12 @@ char *alpha_strcpy(char *dest,
 
        for(i = 0; i < len; i++) {
                int val = (src[i] & 0xff);
-               if (isupper_ascii(val) || islower_ascii(val) ||
-                               isdigit(val) || strchr_m(other_safe_chars, val))
+               if ((isascii(val) && isalnum(val))
+                || strchr_m(other_safe_chars, val)) {
                        dest[i] = src[i];
-               else
+               } else {
                        dest[i] = '_';
+               }
        }
 
        dest[i] = '\0';
-- 
1.7.3.4
Comment 4 David Disseldorp 2011-03-27 21:20:57 UTC
The following changes since commit b47b4c9e3dd1a3776f38b879041fb83a0edb6c36:

  s3: Remove two calls to cli_errstr (2011-03-27 13:17:47 +0200)

are available in the git repository at:
  git://oss.sgi.com/ddiss/samba/ bnc680169-smbclient-segv-rb2

David Disseldorp (2):
      s3-client: check for failures generating ntlmssp negotiate
      s3-charcnv: use isascii() in alpha_strcpy()

 source3/lib/util_str.c   |    7 ++++---
 source3/libsmb/ntlmssp.c |   18 ++++++++++++------
 2 files changed, 16 insertions(+), 9 deletions(-)
Comment 5 Jeremy Allison 2011-03-28 21:32:41 UTC
Ok from this comment :

"The fix looks sane Jeremy, however a commit to cleanup convert_string_talloc()
(15e84a9) which was pushed in the meantime uncovers further issues."

This fix (15e84a9) only went into master, so I'm assuming the alpha_strcpy() change is only for master (4.0.x) - yes ?

I'm going to adapt your msrpc_gen fix for 3.5.x and add this to as an additional patch to the bug report. Can you confirm this is then fixed in 3.5.next with the two fixes added as attachments ?

Jeremy.
Comment 6 David Disseldorp 2011-03-30 17:19:59 UTC
Hi Jeremy,

(In reply to comment #5)
...
> I'm going to adapt your msrpc_gen fix for 3.5.x and add this to as an
> additional patch to the bug report. Can you confirm this is then fixed in
> 3.5.next with the two fixes added as attachments ?
> 

I've tested against v3-5-test with your fix (00834d0) cherry-picked, the results were successful. Sorry for the delay.

Cheers, David
Comment 7 Jeremy Allison 2011-04-01 18:16:13 UTC
Created attachment 6361 [details]
First part of fix.

Volker, both fixes are needed for this bug. Once you've ack'ed please re-assign to ks for inclusion in 3.5.next.

Thanks !

Jeremy.
Comment 8 Jeremy Allison 2011-04-01 18:18:05 UTC
Comment on attachment 6361 [details]
First part of fix.

Sorry - mixed up the two patches here. Wait until I've sorted out the second patch...

Jeremy.
Comment 9 Jeremy Allison 2011-04-01 18:48:25 UTC
Created attachment 6362 [details]
Second part of fix.

Ok, finally got part #1 and part #2 correctly sorted out. They're both git-am patches.

Jeremy.
Comment 10 Karolin Seeger 2011-04-04 18:50:36 UTC
Pushed both patches to v3-5-test.
Closing out bug.

Thanks!