[PATCH] Fix smbclient regression not printing session setup anymore
Stefan Metzmacher
metze at samba.org
Wed Jun 7 15:11:42 UTC 2017
Am 06.06.2017 um 18:47 schrieb Andreas Schneider via samba-technical:
> On Tuesday, 6 June 2017 18:29:59 CEST Jeremy Allison wrote:
>> On Tue, Jun 06, 2017 at 06:27:00PM +0200, Andreas Schneider via samba-
> technical wrote:
>>> On Tuesday, 6 June 2017 18:08:09 CEST Jeremy Allison wrote:
>>>> On Tue, Jun 06, 2017 at 05:56:21PM +0200, Andreas Schneider via samba-
>>>
>>> technical wrote:
>>>>> On Tuesday, 6 June 2017 17:35:52 CEST Andreas Schneider via
>>>>> samba-technical
>>>>>
>>>>> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> with Samba 4.6 we have a regression. We do not get the information
>>>>>> of
>>>>>> the
>>>>>> session setup filled out so smbclient can't print it.
>>>>>>
>>>>>> Domain=[SAMBA-TEST] OS=[Windows 6.1] Server=[Samba
>>>>>> 4.7.0pre1-DEVELOPERBUILD] smb: \>
>>>>>>
>>>>>> The attached patchset addresses the issue. I've open a bug for this:
>>>>>>
>>>>>> https://bugzilla.samba.org/show_bug.cgi?id=12824
>>>>>
>>>>> I've added a test to confirm that the message is actually printed so
>>>>> we do
>>>>> not regress again ...
>>>>
>>>> The change from 'bool align_odd = true' to 'bool align_odd = false'
>>>> concerns me.
>>>>
>>>> smb_bytes_pull_str() is used from all SMB1 session code inside
>>>> libcli/smb/smb1cli_session.c
>>>
>>> The code called smb_bytes_talloc_string() before and this didn't handle
>>> align_odd before, why should it now?
>>
>> Sorry, I'm being dumb. What called smb_bytes_talloc_string() before ?
>
> 9fffec88033a385f3ebb8fe8520b9b39c831d98f
We used to pass 'inhdr' to smb_bytes_talloc_string() before,
that way we got the alignment correct as the unicode characters
are aligned to the beginning of the SMB request buffer.
In smb_bytes_pull_str() is designed to pull the string
just from the "bytes" part of the SMB request, which starts
at an odd offset relative to the start of the complete SMB request
buffer. I'm not 100% sure if that's also the case for chained requests,
but at least samba aligns the wct field of subsequent requests to a
4-byte boundary, see smb_splice_chain() and smb1cli_req_chain_submit().
The attached patch should improve this, but it would be better to have
a regression test. I try to write a cmocka test for it with
Andreas in the next days.
metze
-------------- next part --------------
From 8659ef4023d00e7eccf6388ef58e1cf223ce2b8d Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Wed, 15 Mar 2017 17:04:30 +0000
Subject: [PATCH] libcli/smb: fix alignment problems of smb_bytes_pull_str()
This function needs to get the whole smb buffer in order to get
the alignment for unicode correct.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12779
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
libcli/smb/smb1cli_session.c | 28 +++++++++++++-------------
libcli/smb/smb_util.h | 3 ++-
libcli/smb/util.c | 47 +++++++++++++++++++++++++++++---------------
3 files changed, 47 insertions(+), 31 deletions(-)
diff --git a/libcli/smb/smb1cli_session.c b/libcli/smb/smb1cli_session.c
index 9d92aa6..11614df 100644
--- a/libcli/smb/smb1cli_session.c
+++ b/libcli/smb/smb1cli_session.c
@@ -210,16 +210,16 @@ static void smb1cli_session_setup_lm21_done(struct tevent_req *subreq)
p = bytes;
status = smb_bytes_pull_str(state, &state->out_native_os,
- use_unicode, p,
- bytes+num_bytes-p, &ret);
+ use_unicode, bytes, num_bytes,
+ p, &ret);
if (tevent_req_nterror(req, status)) {
return;
}
p += ret;
status = smb_bytes_pull_str(state, &state->out_native_lm,
- use_unicode, p,
- bytes+num_bytes-p, &ret);
+ use_unicode, bytes, num_bytes,
+ p, &ret);
if (tevent_req_nterror(req, status)) {
return;
}
@@ -493,24 +493,24 @@ static void smb1cli_session_setup_nt1_done(struct tevent_req *subreq)
p = bytes;
status = smb_bytes_pull_str(state, &state->out_native_os,
- use_unicode, p,
- bytes+num_bytes-p, &ret);
+ use_unicode, bytes, num_bytes,
+ p, &ret);
if (tevent_req_nterror(req, status)) {
return;
}
p += ret;
status = smb_bytes_pull_str(state, &state->out_native_lm,
- use_unicode, p,
- bytes+num_bytes-p, &ret);
+ use_unicode, bytes, num_bytes,
+ p, &ret);
if (tevent_req_nterror(req, status)) {
return;
}
p += ret;
status = smb_bytes_pull_str(state, &state->out_primary_domain,
- use_unicode, p,
- bytes+num_bytes-p, &ret);
+ use_unicode, bytes, num_bytes,
+ p, &ret);
if (tevent_req_nterror(req, status)) {
return;
}
@@ -754,16 +754,16 @@ static void smb1cli_session_setup_ext_done(struct tevent_req *subreq)
p += out_security_blob_length;
status = smb_bytes_pull_str(state, &state->out_native_os,
- use_unicode, p,
- bytes+num_bytes-p, &ret);
+ use_unicode, bytes, num_bytes,
+ p, &ret);
if (tevent_req_nterror(req, status)) {
return;
}
p += ret;
status = smb_bytes_pull_str(state, &state->out_native_lm,
- use_unicode, p,
- bytes+num_bytes-p, &ret);
+ use_unicode, bytes, num_bytes,
+ p, &ret);
if (tevent_req_nterror(req, status)) {
return;
}
diff --git a/libcli/smb/smb_util.h b/libcli/smb/smb_util.h
index 7e6f0a4..2884786 100644
--- a/libcli/smb/smb_util.h
+++ b/libcli/smb/smb_util.h
@@ -38,4 +38,5 @@ uint8_t *trans2_bytes_push_bytes(uint8_t *buf,
const uint8_t *bytes, size_t num_bytes);
NTSTATUS smb_bytes_pull_str(TALLOC_CTX *mem_ctx, char **_str, bool ucs2,
const uint8_t *buf, size_t buf_len,
- size_t *pbuf_consumed);
+ const uint8_t *position,
+ size_t *_consumed);
diff --git a/libcli/smb/util.c b/libcli/smb/util.c
index ef8c9fa..7ef909c 100644
--- a/libcli/smb/util.c
+++ b/libcli/smb/util.c
@@ -319,29 +319,43 @@ uint8_t *trans2_bytes_push_bytes(uint8_t *buf,
static NTSTATUS internal_bytes_pull_str(TALLOC_CTX *mem_ctx, char **_str,
bool ucs2, bool align_odd,
const uint8_t *buf, size_t buf_len,
- size_t *pbuf_consumed)
+ const uint8_t *position,
+ size_t *p_consumed)
{
size_t pad = 0;
+ size_t offset;
char *str = NULL;
size_t str_len = 0;
bool ok;
*_str = NULL;
- if (pbuf_consumed != NULL) {
- *pbuf_consumed = 0;
+ if (p_consumed != NULL) {
+ *p_consumed = 0;
+ }
+
+ if (position < buf) {
+ return NT_STATUS_INTERNAL_ERROR;
+ }
+
+ offset = PTR_DIFF(position, buf);
+ if (offset > buf_len) {
+ return NT_STATUS_BUFFER_TOO_SMALL;
}
if (ucs2 &&
- ((align_odd && (buf_len % 2 == 0)) ||
- (!align_odd && (buf_len % 2 == 1)))) {
- if (buf_len < 1) {
- return NT_STATUS_BUFFER_TOO_SMALL;
- }
- pad = 1;
- buf_len -= pad;
- buf += pad;
+ ((align_odd && (offset % 2 == 0)) ||
+ (!align_odd && (offset % 2 == 1)))) {
+ pad += 1;
+ offset += 1;
+ }
+
+ if (offset > buf_len) {
+ return NT_STATUS_BUFFER_TOO_SMALL;
}
+ buf_len -= offset;
+ buf += offset;
+
if (ucs2) {
buf_len = utf16_len_n(buf, buf_len);
} else {
@@ -361,17 +375,18 @@ static NTSTATUS internal_bytes_pull_str(TALLOC_CTX *mem_ctx, char **_str,
return map_nt_error_from_unix_common(errno);
}
- if (pbuf_consumed != NULL) {
- *pbuf_consumed = buf_len + pad;
+ if (p_consumed != NULL) {
+ *p_consumed = buf_len + pad;
}
*_str = str;
- return NT_STATUS_OK;;
+ return NT_STATUS_OK;
}
NTSTATUS smb_bytes_pull_str(TALLOC_CTX *mem_ctx, char **_str, bool ucs2,
const uint8_t *buf, size_t buf_len,
- size_t *_buf_consumed)
+ const uint8_t *position,
+ size_t *_consumed)
{
return internal_bytes_pull_str(mem_ctx, _str, ucs2, true,
- buf, buf_len, _buf_consumed);
+ buf, buf_len, position, _consumed);
}
--
1.9.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20170607/1704550a/signature.sig>
More information about the samba-technical
mailing list