Quantcast

[PATCH] allow passdb backend to change trusted domain object password with clear text

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
Hi,

attached patch switches _netr_ServerPasswordSet2 to use SetUserInfo2
info level 26. This allows us to pass through clear text password change
down to passdb backend. This is critical for AD-like configurations
(FreeIPA) where it is not enough to change NT or LM hashes for TDO, as
one needs to generate Kerberos keys as well.

I'm working on a corresponding change in FreeIPA ipasam module as well.
It currently does not provide pdb_update_sam_account() callback so end
result is still NT_STATUS_NOT_IMPLEMENTED as can be witnessed with
'nltest /sc_change_pwd:ipa.domain' but we are getting closer.

Please review.


--
/ Alexander Bokovoy

samba-s3-clear-text-password-change.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On Thu, Apr 06, 2017 at 06:46:06PM +0300, Alexander Bokovoy via samba-technical wrote:

> Hi,
>
> attached patch switches _netr_ServerPasswordSet2 to use SetUserInfo2
> info level 26. This allows us to pass through clear text password change
> down to passdb backend. This is critical for AD-like configurations
> (FreeIPA) where it is not enough to change NT or LM hashes for TDO, as
> one needs to generate Kerberos keys as well.
>
> I'm working on a corresponding change in FreeIPA ipasam module as well.
> It currently does not provide pdb_update_sam_account() callback so end
> result is still NT_STATUS_NOT_IMPLEMENTED as can be witnessed with
> 'nltest /sc_change_pwd:ipa.domain' but we are getting closer.
> + const char *account_name,
> + DATA_BLOB *plain_text)
> +{
> + NTSTATUS status;
> + NTSTATUS result = NT_STATUS_OK;
> + struct dcerpc_binding_handle *h = NULL;
> + struct tsocket_address *local;
> + struct policy_handle user_handle;
> + uint32_t acct_ctrl;
> + union samr_UserInfo *info;
> + struct samr_UserInfo26 info26;
> + int rc;
> + DATA_BLOB session_key;
> +
> + ZERO_STRUCT(user_handle);
> +
> + status = session_extract_session_key(session_info,
> +     &session_key,
> +     KEY_USE_16BYTES);
> + if (!NT_STATUS_IS_OK(status)) {
> + goto out;
> + }
> +
> + rc = tsocket_address_inet_from_strings(mem_ctx,
> +       "ip",
> +       "127.0.0.1",
> +       0,
> +       &local);

Alexander - is the above going to work on an IPv6-only
box ?

Can you test that please.

Thanks,

Jeremy.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On Fri, 2017-04-07 at 12:12 -0700, Jeremy Allison via samba-technical
wrote:

> On Thu, Apr 06, 2017 at 06:46:06PM +0300, Alexander Bokovoy via
> samba-technical wrote:
> > Hi,
> >
> > attached patch switches _netr_ServerPasswordSet2 to use
> > SetUserInfo2
> > info level 26. This allows us to pass through clear text password
> > change
> > down to passdb backend. This is critical for AD-like configurations
> > (FreeIPA) where it is not enough to change NT or LM hashes for TDO,
> > as
> > one needs to generate Kerberos keys as well.
> >
> > I'm working on a corresponding change in FreeIPA ipasam module as
> > well.
> > It currently does not provide pdb_update_sam_account() callback so
> > end
> > result is still NT_STATUS_NOT_IMPLEMENTED as can be witnessed with
> > 'nltest /sc_change_pwd:ipa.domain' but we are getting closer.
> > + const char
> > *account_name,
> > + DATA_BLOB
> > *plain_text)
> > +{
> > + NTSTATUS status;
> > + NTSTATUS result = NT_STATUS_OK;
> > + struct dcerpc_binding_handle *h = NULL;
> > + struct tsocket_address *local;
> > + struct policy_handle user_handle;
> > + uint32_t acct_ctrl;
> > + union samr_UserInfo *info;
> > + struct samr_UserInfo26 info26;
> > + int rc;
> > + DATA_BLOB session_key;
> > +
> > + ZERO_STRUCT(user_handle);
> > +
> > + status = session_extract_session_key(session_info,
> > +      &session_key,
> > +      KEY_USE_16BYTES);
> > + if (!NT_STATUS_IS_OK(status)) {
> > + goto out;
> > + }
> > +
> > + rc = tsocket_address_inet_from_strings(mem_ctx,
> > +        "ip",
> > +        "127.0.0.1",
> > +        0,
> > +        &local);
>
> Alexander - is the above going to work on an IPv6-only
> box ?
>
> Can you test that please.

Those strings are just used for logging, the connection is over the
named pipes.

While well beyond what Alexander wants to take on for this (which is
following a pattern already well used) I would really like it if
netr_set_machine_account_password and this new function actually passed
over the client IP.  That way the authorization logs don't include this
fake localhost address.  If we are going to have the samr server as the
choke point in source3, it would be nicer to the future developer
adding more audit logging if we didn't mislead it.

The address is on:

struct pipes_struct {
        struct pipes_struct *next, *prev;

        const struct tsocket_address *local_address;
        const struct tsocket_address *remote_address;

Or, alternately (for things like the get_md4() call), we don't use
named pipes, but ncalrpc as the system user so we clearly know this is
an internal connection.

Thanks,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On Sat, Apr 08, 2017 at 07:46:23AM +1200, Andrew Bartlett wrote:

> On Fri, 2017-04-07 at 12:12 -0700, Jeremy Allison via samba-technical
> wrote:
> > On Thu, Apr 06, 2017 at 06:46:06PM +0300, Alexander Bokovoy via
> > samba-technical wrote:
> > > Hi,
> > >
> > > attached patch switches _netr_ServerPasswordSet2 to use
> > > SetUserInfo2
> > > info level 26. This allows us to pass through clear text password
> > > change
> > > down to passdb backend. This is critical for AD-like configurations
> > > (FreeIPA) where it is not enough to change NT or LM hashes for TDO,
> > > as
> > > one needs to generate Kerberos keys as well.
> > >
> > > I'm working on a corresponding change in FreeIPA ipasam module as
> > > well.
> > > It currently does not provide pdb_update_sam_account() callback so
> > > end
> > > result is still NT_STATUS_NOT_IMPLEMENTED as can be witnessed with
> > > 'nltest /sc_change_pwd:ipa.domain' but we are getting closer.
> > > + const char
> > > *account_name,
> > > + DATA_BLOB
> > > *plain_text)
> > > +{
> > > + NTSTATUS status;
> > > + NTSTATUS result = NT_STATUS_OK;
> > > + struct dcerpc_binding_handle *h = NULL;
> > > + struct tsocket_address *local;
> > > + struct policy_handle user_handle;
> > > + uint32_t acct_ctrl;
> > > + union samr_UserInfo *info;
> > > + struct samr_UserInfo26 info26;
> > > + int rc;
> > > + DATA_BLOB session_key;
> > > +
> > > + ZERO_STRUCT(user_handle);
> > > +
> > > + status = session_extract_session_key(session_info,
> > > +      &session_key,
> > > +      KEY_USE_16BYTES);
> > > + if (!NT_STATUS_IS_OK(status)) {
> > > + goto out;
> > > + }
> > > +
> > > + rc = tsocket_address_inet_from_strings(mem_ctx,
> > > +        "ip",
> > > +        "127.0.0.1",
> > > +        0,
> > > +        &local);
> >
> > Alexander - is the above going to work on an IPv6-only
> > box ?
> >
> > Can you test that please.
>
> Those strings are just used for logging

Nope. The code looks like:

+       rc = tsocket_address_inet_from_strings(mem_ctx,
+                                              "ip",
+                                              "127.0.0.1",
+                                              0,
+                                              &local);
+       if (rc < 0) {
+               status = NT_STATUS_NO_MEMORY;
+               goto out;
+       }

Third argument is "addr" which gets passed internally
to getaddrinfo(). So it's calling getaddrinfo("127.0.0.1",...).

What does that do an an IPv6-only box ? The right thing ?
Maybe. But if it returns any error then we exit here, despite IPv6-localhost
existing ::1 and being valid.

Maybe. But we shouldn't be encoding IPv4-specific addresses
anymore on a machine that potentially doesn't use them.

That's what I was complaining about :-). I spent a long time
making us IPv6 clean and I don't want to see regressions.

Jeremy.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On Fri, 07 Apr 2017, Jeremy Allison wrote:

> On Sat, Apr 08, 2017 at 07:46:23AM +1200, Andrew Bartlett wrote:
> > On Fri, 2017-04-07 at 12:12 -0700, Jeremy Allison via samba-technical
> > wrote:
> > > On Thu, Apr 06, 2017 at 06:46:06PM +0300, Alexander Bokovoy via
> > > samba-technical wrote:
> > > > Hi,
> > > >
> > > > attached patch switches _netr_ServerPasswordSet2 to use
> > > > SetUserInfo2
> > > > info level 26. This allows us to pass through clear text password
> > > > change
> > > > down to passdb backend. This is critical for AD-like configurations
> > > > (FreeIPA) where it is not enough to change NT or LM hashes for TDO,
> > > > as
> > > > one needs to generate Kerberos keys as well.
> > > >
> > > > I'm working on a corresponding change in FreeIPA ipasam module as
> > > > well.
> > > > It currently does not provide pdb_update_sam_account() callback so
> > > > end
> > > > result is still NT_STATUS_NOT_IMPLEMENTED as can be witnessed with
> > > > 'nltest /sc_change_pwd:ipa.domain' but we are getting closer.
> > > > + const char
> > > > *account_name,
> > > > + DATA_BLOB
> > > > *plain_text)
> > > > +{
> > > > + NTSTATUS status;
> > > > + NTSTATUS result = NT_STATUS_OK;
> > > > + struct dcerpc_binding_handle *h = NULL;
> > > > + struct tsocket_address *local;
> > > > + struct policy_handle user_handle;
> > > > + uint32_t acct_ctrl;
> > > > + union samr_UserInfo *info;
> > > > + struct samr_UserInfo26 info26;
> > > > + int rc;
> > > > + DATA_BLOB session_key;
> > > > +
> > > > + ZERO_STRUCT(user_handle);
> > > > +
> > > > + status = session_extract_session_key(session_info,
> > > > +      &session_key,
> > > > +      KEY_USE_16BYTES);
> > > > + if (!NT_STATUS_IS_OK(status)) {
> > > > + goto out;
> > > > + }
> > > > +
> > > > + rc = tsocket_address_inet_from_strings(mem_ctx,
> > > > +        "ip",
> > > > +        "127.0.0.1",
> > > > +        0,
> > > > +        &local);
> > >
> > > Alexander - is the above going to work on an IPv6-only
> > > box ?
> > >
> > > Can you test that please.
> >
> > Those strings are just used for logging
>
> Nope. The code looks like:
>
> +       rc = tsocket_address_inet_from_strings(mem_ctx,
> +                                              "ip",
> +                                              "127.0.0.1",
> +                                              0,
> +                                              &local);
> +       if (rc < 0) {
> +               status = NT_STATUS_NO_MEMORY;
> +               goto out;
> +       }
>
> Third argument is "addr" which gets passed internally
> to getaddrinfo(). So it's calling getaddrinfo("127.0.0.1",...).
>
> What does that do an an IPv6-only box ? The right thing ?
> Maybe. But if it returns any error then we exit here, despite IPv6-localhost
> existing ::1 and being valid.
>
> Maybe. But we shouldn't be encoding IPv4-specific addresses
> anymore on a machine that potentially doesn't use them.
>
> That's what I was complaining about :-). I spent a long time
> making us IPv6 clean and I don't want to see regressions.
I'll see what I can do there but this code is a copy/paste from another
helper we have for NT/LM hash pass-through. Guenther already asked me to
consider how I can going these two functions in a common piece that
could be called for both cases, so I'll do refactoring for this too.

--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On Fri, Apr 07, 2017 at 11:08:44PM +0300, Alexander Bokovoy wrote:

> I'll see what I can do there but this code is a copy/paste from another
> helper we have for NT/LM hash pass-through. Guenther already asked me to
> consider how I can going these two functions in a common piece that
> could be called for both cases, so I'll do refactoring for this too.

Thanks. That other code might be wrong too :-).

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On Fri, 2017-04-07 at 13:22 -0700, Jeremy Allison via samba-technical
wrote:

> On Fri, Apr 07, 2017 at 11:08:44PM +0300, Alexander Bokovoy wrote:
>
> > I'll see what I can do there but this code is a copy/paste from
> > another
> > helper we have for NT/LM hash pass-through. Guenther already asked
> > me to
> > consider how I can going these two functions in a common piece that
> > could be called for both cases, so I'll do refactoring for this
> > too.
>
> Thanks. That other code might be wrong too :-).

Sadly I found during the audit work that the pattern is the standard
way internal IPC is done in source3, so the pattern is repeated often.

When working in this area, please take care regarding the remote_client
address and the local_server address.  We did find cases in Samba where
this was reversed, and the two parameters are easy to swap as they are
the same type.

Additionally, when backporting or forward porting, carefully check the
parameters as we worked to ensure they were consistently ordered, but
that means some function definitions changed.

Thanks,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On pe, 07 huhti 2017, Jeremy Allison wrote:
> On Fri, Apr 07, 2017 at 11:08:44PM +0300, Alexander Bokovoy wrote:
>
> > I'll see what I can do there but this code is a copy/paste from another
> > helper we have for NT/LM hash pass-through. Guenther already asked me to
> > consider how I can going these two functions in a common piece that
> > could be called for both cases, so I'll do refactoring for this too.
>
> Thanks. That other code might be wrong too :-).
Attached is the patch that unifies NT hash/plain text pass-through.
This is a more concise change that doesn't duplicate existing code.

I think we should handle IPv6 concerns in a separate patchset. Below is
the list of places where we hardcode 127.0.0.1 explicitly.

source3/librpc/rpc/dcerpc_ep.c:         rc = tsocket_address_inet_from_strings(tmp_ctx,
source3/librpc/rpc/dcerpc_ep.c-                                                "ip",
source3/librpc/rpc/dcerpc_ep.c-                                                "127.0.0.1",
--
source3/rpc_client/cli_winreg_int.c:    rc = tsocket_address_inet_from_strings(mem_ctx,
source3/rpc_client/cli_winreg_int.c-                                           "ip",
source3/rpc_client/cli_winreg_int.c-                                           "127.0.0.1",
--
source3/rpc_server/netlogon/srv_netlog_nt.c:    rc = tsocket_address_inet_from_strings(mem_ctx,
source3/rpc_server/netlogon/srv_netlog_nt.c-                                           "ip",
source3/rpc_server/netlogon/srv_netlog_nt.c-                                           "127.0.0.1",
--
source3/rpc_server/netlogon/srv_netlog_nt.c:    rc = tsocket_address_inet_from_strings(mem_ctx,
source3/rpc_server/netlogon/srv_netlog_nt.c-                                           "ip",
source3/rpc_server/netlogon/srv_netlog_nt.c-                                           "127.0.0.1",
--
source3/rpc_server/rpc_ncacn_np.c:              rc = tsocket_address_inet_from_strings(mem_ctx,
source3/rpc_server/rpc_ncacn_np.c-                                                     "ip",
source3/rpc_server/rpc_ncacn_np.c-                                                     "127.0.0.1",
--
source3/rpc_server/spoolss/srv_spoolss_util.c:  rc = tsocket_address_inet_from_strings(mem_ctx,
source3/rpc_server/spoolss/srv_spoolss_util.c-                                         "ip",
source3/rpc_server/spoolss/srv_spoolss_util.c-                                         "127.0.0.1",
--
source3/winbindd/winbindd_pam.c:        rc = tsocket_address_inet_from_strings(frame,
source3/winbindd/winbindd_pam.c-                                               "ip",
source3/winbindd/winbindd_pam.c-                                               "127.0.0.1",
--

I filed bug https://bugzilla.samba.org/show_bug.cgi?id=12738 to track this effort.

We also have NBNS replication (MS-WINSRA) implementation that only
supports IPv4 because NBNS only supports IPv4:

source4/libcli/wrepl/winsrepl.c:        ret = tsocket_address_inet_from_strings(state, "ipv4",
source4/libcli/wrepl/winsrepl.c-                                                our_ip, 0,
source4/libcli/wrepl/winsrepl.c-                                                &state->local_address);
--
source4/libcli/wrepl/winsrepl.c:        ret = tsocket_address_inet_from_strings(state, "ipv4",
source4/libcli/wrepl/winsrepl.c-                                                peer_ip, WINS_REPLICATION_PORT,
source4/libcli/wrepl/winsrepl.c-                                                &state->remote_address);
--

--
/ Alexander Bokovoy

samba-s3-clear-text-password-change.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On Mon, 2017-04-10 at 10:22 +0300, Alexander Bokovoy wrote:

> On pe, 07 huhti 2017, Jeremy Allison wrote:
> > On Fri, Apr 07, 2017 at 11:08:44PM +0300, Alexander Bokovoy wrote:
> >
> > > I'll see what I can do there but this code is a copy/paste from
> > > another
> > > helper we have for NT/LM hash pass-through. Guenther already
> > > asked me to
> > > consider how I can going these two functions in a common piece
> > > that
> > > could be called for both cases, so I'll do refactoring for this
> > > too.
> >
> > Thanks. That other code might be wrong too :-).
>
> Attached is the patch that unifies NT hash/plain text pass-through.
> This is a more concise change that doesn't duplicate existing code.

How certain are we that our plaintext password is null terminated?

+ cr.creds.password = (const char*) plaintext.data;

Thanks,

Andrew Bartlett
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On ma, 10 huhti 2017, Andrew Bartlett wrote:

> On Mon, 2017-04-10 at 10:22 +0300, Alexander Bokovoy wrote:
> > On pe, 07 huhti 2017, Jeremy Allison wrote:
> > > On Fri, Apr 07, 2017 at 11:08:44PM +0300, Alexander Bokovoy wrote:
> > >
> > > > I'll see what I can do there but this code is a copy/paste from
> > > > another
> > > > helper we have for NT/LM hash pass-through. Guenther already
> > > > asked me to
> > > > consider how I can going these two functions in a common piece
> > > > that
> > > > could be called for both cases, so I'll do refactoring for this
> > > > too.
> > >
> > > Thanks. That other code might be wrong too :-).
> >
> > Attached is the patch that unifies NT hash/plain text pass-through.
> > This is a more concise change that doesn't duplicate existing code.
>
> How certain are we that our plaintext password is null terminated?
>
> + cr.creds.password = (const char*) plaintext.data;
Good question. A plain text password is a data blob represented as up to
256 WCHARs. It is UTF-16 coded on wire and we have its length from the
buffer.  SetUserInfo2 SAMR call chain in decode_pw_buffer() does explicitly
expect 512+4 bytes in the buffer. It then calls convert_string_talloc() to
convert it to UNIX charset passing the correct value of the plaintext
password length. However, convert_string_talloc() expects the length of
input string *including* the terminating null and we pass just the
string length.

convert_string_talloc() then explicitly null-terminates the resulting
string by adding two nulls. In most cases UNIX charset is UTF-8, so we
get null-terminated UTF-8 string down to PASSDB layer.

However, MS-SAMR does not limit what does the password should contain.
It says it is 'userPassword' value. Either 'userPassword' or
'unicodePwd' cannot contain null characters according to MS-ADTS
3.1.1.3.1.5 because they must be proper UTF-8 and UTF-16 strings
accordingly.

So I think we are good here,

--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On Mon, 2017-04-10 at 16:51 +0300, Alexander Bokovoy wrote:

> On ma, 10 huhti 2017, Andrew Bartlett wrote:
> > On Mon, 2017-04-10 at 10:22 +0300, Alexander Bokovoy wrote:
> > > On pe, 07 huhti 2017, Jeremy Allison wrote:
> > > > On Fri, Apr 07, 2017 at 11:08:44PM +0300, Alexander Bokovoy
> > > > wrote:
> > > >
> > > > > I'll see what I can do there but this code is a copy/paste
> > > > > from
> > > > > another
> > > > > helper we have for NT/LM hash pass-through. Guenther already
> > > > > asked me to
> > > > > consider how I can going these two functions in a common
> > > > > piece
> > > > > that
> > > > > could be called for both cases, so I'll do refactoring for
> > > > > this
> > > > > too.
> > > >
> > > > Thanks. That other code might be wrong too :-).
> > >
> > > Attached is the patch that unifies NT hash/plain text pass-
> > > through.
> > > This is a more concise change that doesn't duplicate existing
> > > code.
> >
> > How certain are we that our plaintext password is null terminated?
> >
> > + cr.creds.password = (const char*) plaintext.data;
>
> Good question. A plain text password is a data blob represented as up
> to
> 256 WCHARs. It is UTF-16 coded on wire and we have its length from
> the
> buffer.  SetUserInfo2 SAMR call chain in decode_pw_buffer() does
> explicitly
> expect 512+4 bytes in the buffer. It then calls
> convert_string_talloc() to
> convert it to UNIX charset passing the correct value of the plaintext
> password length. However, convert_string_talloc() expects the length
> of
> input string *including* the terminating null and we pass just the
> string length.
>
> convert_string_talloc() then explicitly null-terminates the resulting
> string by adding two nulls. In most cases UNIX charset is UTF-8, so
> we
> get null-terminated UTF-8 string down to PASSDB layer.
>
> However, MS-SAMR does not limit what does the password should
> contain.
> It says it is 'userPassword' value. Either 'userPassword' or
> 'unicodePwd' cannot contain null characters according to MS-ADTS
> 3.1.1.3.1.5 because they must be proper UTF-8 and UTF-16 strings
> accordingly. 
>
> So I think we are good here,

Please copy this into a comment, so we remember and an auditor can be
re-assured.

Thanks,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On ti, 11 huhti 2017, Andrew Bartlett wrote:

> On Mon, 2017-04-10 at 16:51 +0300, Alexander Bokovoy wrote:
> > On ma, 10 huhti 2017, Andrew Bartlett wrote:
> > > On Mon, 2017-04-10 at 10:22 +0300, Alexander Bokovoy wrote:
> > > > On pe, 07 huhti 2017, Jeremy Allison wrote:
> > > > > On Fri, Apr 07, 2017 at 11:08:44PM +0300, Alexander Bokovoy
> > > > > wrote:
> > > > >
> > > > > > I'll see what I can do there but this code is a copy/paste
> > > > > > from
> > > > > > another
> > > > > > helper we have for NT/LM hash pass-through. Guenther already
> > > > > > asked me to
> > > > > > consider how I can going these two functions in a common
> > > > > > piece
> > > > > > that
> > > > > > could be called for both cases, so I'll do refactoring for
> > > > > > this
> > > > > > too.
> > > > >
> > > > > Thanks. That other code might be wrong too :-).
> > > >
> > > > Attached is the patch that unifies NT hash/plain text pass-
> > > > through.
> > > > This is a more concise change that doesn't duplicate existing
> > > > code.
> > >
> > > How certain are we that our plaintext password is null terminated?
> > >
> > > + cr.creds.password = (const char*) plaintext.data;
> >
> > Good question. A plain text password is a data blob represented as up
> > to
> > 256 WCHARs. It is UTF-16 coded on wire and we have its length from
> > the
> > buffer.  SetUserInfo2 SAMR call chain in decode_pw_buffer() does
> > explicitly
> > expect 512+4 bytes in the buffer. It then calls
> > convert_string_talloc() to
> > convert it to UNIX charset passing the correct value of the plaintext
> > password length. However, convert_string_talloc() expects the length
> > of
> > input string *including* the terminating null and we pass just the
> > string length.
> >
> > convert_string_talloc() then explicitly null-terminates the resulting
> > string by adding two nulls. In most cases UNIX charset is UTF-8, so
> > we
> > get null-terminated UTF-8 string down to PASSDB layer.
> >
> > However, MS-SAMR does not limit what does the password should
> > contain.
> > It says it is 'userPassword' value. Either 'userPassword' or
> > 'unicodePwd' cannot contain null characters according to MS-ADTS
> > 3.1.1.3.1.5 because they must be proper UTF-8 and UTF-16 strings
> > accordingly. 
> >
> > So I think we are good here,
>
> Please copy this into a comment, so we remember and an auditor can be
> re-assured.
Re-sent with updated comment.
--
/ Alexander Bokovoy

samba-s3-clear-text-password-change.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On ti, 11 huhti 2017, Alexander Bokovoy via samba-technical wrote:

> On ti, 11 huhti 2017, Andrew Bartlett wrote:
> > On Mon, 2017-04-10 at 16:51 +0300, Alexander Bokovoy wrote:
> > > On ma, 10 huhti 2017, Andrew Bartlett wrote:
> > > > On Mon, 2017-04-10 at 10:22 +0300, Alexander Bokovoy wrote:
> > > > > On pe, 07 huhti 2017, Jeremy Allison wrote:
> > > > > > On Fri, Apr 07, 2017 at 11:08:44PM +0300, Alexander Bokovoy
> > > > > > wrote:
> > > > > >
> > > > > > > I'll see what I can do there but this code is a copy/paste
> > > > > > > from
> > > > > > > another
> > > > > > > helper we have for NT/LM hash pass-through. Guenther already
> > > > > > > asked me to
> > > > > > > consider how I can going these two functions in a common
> > > > > > > piece
> > > > > > > that
> > > > > > > could be called for both cases, so I'll do refactoring for
> > > > > > > this
> > > > > > > too.
> > > > > >
> > > > > > Thanks. That other code might be wrong too :-).
> > > > >
> > > > > Attached is the patch that unifies NT hash/plain text pass-
> > > > > through.
> > > > > This is a more concise change that doesn't duplicate existing
> > > > > code.
> > > >
> > > > How certain are we that our plaintext password is null terminated?
> > > >
> > > > + cr.creds.password = (const char*) plaintext.data;
> > >
> > > Good question. A plain text password is a data blob represented as up
> > > to
> > > 256 WCHARs. It is UTF-16 coded on wire and we have its length from
> > > the
> > > buffer.  SetUserInfo2 SAMR call chain in decode_pw_buffer() does
> > > explicitly
> > > expect 512+4 bytes in the buffer. It then calls
> > > convert_string_talloc() to
> > > convert it to UNIX charset passing the correct value of the plaintext
> > > password length. However, convert_string_talloc() expects the length
> > > of
> > > input string *including* the terminating null and we pass just the
> > > string length.
> > >
> > > convert_string_talloc() then explicitly null-terminates the resulting
> > > string by adding two nulls. In most cases UNIX charset is UTF-8, so
> > > we
> > > get null-terminated UTF-8 string down to PASSDB layer.
> > >
> > > However, MS-SAMR does not limit what does the password should
> > > contain.
> > > It says it is 'userPassword' value. Either 'userPassword' or
> > > 'unicodePwd' cannot contain null characters according to MS-ADTS
> > > 3.1.1.3.1.5 because they must be proper UTF-8 and UTF-16 strings
> > > accordingly. 
> > >
> > > So I think we are good here,
> >
> > Please copy this into a comment, so we remember and an auditor can be
> > re-assured.
> Re-sent with updated comment.
Could please anyone to review updated patch (in previous email)?

--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, 2017-04-11 at 08:11 +0300, Alexander Bokovoy via samba-
technical wrote:
>
> Re-sent with updated comment.

Reviewed-by: Andrew Bartlett <[hidden email]>

Please push.

Thanks,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On ti, 18 huhti 2017, Andrew Bartlett wrote:
> On Tue, 2017-04-11 at 08:11 +0300, Alexander Bokovoy via samba-
> technical wrote:
> >
> > Re-sent with updated comment.
>
> Reviewed-by: Andrew Bartlett <[hidden email]>
>
> Please push.
Pushed, thanks.
--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On ti, 18 huhti 2017, Alexander Bokovoy via samba-technical wrote:

> On ti, 18 huhti 2017, Andrew Bartlett wrote:
> > On Tue, 2017-04-11 at 08:11 +0300, Alexander Bokovoy via samba-
> > technical wrote:
> > >
> > > Re-sent with updated comment.
> >
> > Reviewed-by: Andrew Bartlett <[hidden email]>
> >
> > Please push.
> Pushed, thanks.
Failed, I'm checking what's wrong, found already some bugs in a
net_change_cred test...

--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On ti, 18 huhti 2017, Alexander Bokovoy via samba-technical wrote:

> On ti, 18 huhti 2017, Alexander Bokovoy via samba-technical wrote:
> > On ti, 18 huhti 2017, Andrew Bartlett wrote:
> > > On Tue, 2017-04-11 at 08:11 +0300, Alexander Bokovoy via samba-
> > > technical wrote:
> > > >
> > > > Re-sent with updated comment.
> > >
> > > Reviewed-by: Andrew Bartlett <[hidden email]>
> > >
> > > Please push.
> > Pushed, thanks.
> Failed, I'm checking what's wrong, found already some bugs in a
> net_change_cred test...
I forgot to decode the buffer, not just extract as it was before with NT
Hash for netr_ServerPasswordSet2. Now all tests pass.

I also fixed an error in samba3.blackbox.net_cred_change that caused
shell error due to wrong shell syntax. This patch is attached too.

Jeremy, could you please review it one more time and push?

--
/ Alexander Bokovoy

samba-s3-clear-text-password-change-1.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] allow passdb backend to change trusted domain object password with clear text

Samba - samba-technical mailing list
On Tue, Apr 18, 2017 at 06:39:49PM +0300, Alexander Bokovoy wrote:

> On ti, 18 huhti 2017, Alexander Bokovoy via samba-technical wrote:
> > On ti, 18 huhti 2017, Alexander Bokovoy via samba-technical wrote:
> > > On ti, 18 huhti 2017, Andrew Bartlett wrote:
> > > > On Tue, 2017-04-11 at 08:11 +0300, Alexander Bokovoy via samba-
> > > > technical wrote:
> > > > >
> > > > > Re-sent with updated comment.
> > > >
> > > > Reviewed-by: Andrew Bartlett <[hidden email]>
> > > >
> > > > Please push.
> > > Pushed, thanks.
> > Failed, I'm checking what's wrong, found already some bugs in a
> > net_change_cred test...
> I forgot to decode the buffer, not just extract as it was before with NT
> Hash for netr_ServerPasswordSet2. Now all tests pass.
>
> I also fixed an error in samba3.blackbox.net_cred_change that caused
> shell error due to wrong shell syntax. This patch is attached too.
>
> Jeremy, could you please review it one more time and push?

RB+. Pushed. Thanks !

> From c022f83e8e6a333247e9dc7194de31071780c42b Mon Sep 17 00:00:00 2001
> From: Alexander Bokovoy <[hidden email]>
> Date: Fri, 31 Mar 2017 12:44:58 +0300
> Subject: [PATCH 1/2] _netr_ServerPasswordSet2: use info level 26 to set plain
>  text machine password
>
> To support password change for machine or trusted domain accounts in Active
> Directory environment we need to pass down actual plain text password
> instead of NT hashes. This would allow a backend like ipasam to update
> Kerberos keys as well as NT hashes.
>
> By calling samr_SetUserInfo2 info level 26 we ensure PASSDB layer can
> actually get the plain text password. If PASSDB backend implements
> pdb_update_sam_account() callback, it then gets the plain text password
> from samr_SetUserInfo2.
>
> A plain text password is a data blob represented as up to 256 WCHARs. It
> is UTF-16 coded on wire and we have its length from the buffer.
> SetUserInfo2 SAMR call chain in decode_pw_buffer() does explicitly
> expect 512+4 bytes in the buffer. It then calls convert_string_talloc()
> to convert it to UNIX charset passing the correct value of the plaintext
> password length. However, convert_string_talloc() expects the length of
> input string *including* the terminating null and we pass just the
> string length.
>
> convert_string_talloc() then explicitly null-terminates the resulting
> string by adding two nulls. In most cases UNIX charset is UTF-8, so we
> get null-terminated UTF-8 string down to PASSDB layer.
>
> MS-SAMR does not limit what does the password should contain.  It says
> it is 'userPassword' value. Either 'userPassword' or 'unicodePwd' cannot
> contain null characters according to MS-ADTS 3.1.1.3.1.5 because they
> must be proper UTF-8 and UTF-16 strings accordingly.
>
> We are talking to our own SAMR service here.
>
> Signed-off-by: Alexander Bokovoy <[hidden email]>
> Reviewed-by: Andrew Bartlett <[hidden email]>
> ---
>  source3/rpc_server/netlogon/srv_netlog_nt.c | 79 +++++++++++++++++++++++------
>  1 file changed, 64 insertions(+), 15 deletions(-)
>
> diff --git a/source3/rpc_server/netlogon/srv_netlog_nt.c b/source3/rpc_server/netlogon/srv_netlog_nt.c
> index 6a42f34..c36e659 100644
> --- a/source3/rpc_server/netlogon/srv_netlog_nt.c
> +++ b/source3/rpc_server/netlogon/srv_netlog_nt.c
> @@ -33,6 +33,7 @@
>  #include "../librpc/gen_ndr/ndr_lsa_c.h"
>  #include "rpc_client/cli_lsarpc.h"
>  #include "rpc_client/init_lsa.h"
> +#include "rpc_client/init_samr.h"
>  #include "rpc_server/rpc_ncacn_np.h"
>  #include "../libcli/security/security.h"
>  #include "../libcli/security/dom_sid.h"
> @@ -1140,14 +1141,27 @@ static NTSTATUS netr_creds_server_step_check(struct pipes_struct *p,
>   return status;
>  }
>  
> +
>  /*************************************************************************
>   *************************************************************************/
>  
> +struct _samr_Credentials_t {
> + enum {
> + CRED_TYPE_NT_HASH,
> + CRED_TYPE_PLAIN_TEXT,
> + } cred_type;
> + union {
> + struct samr_Password *nt_hash;
> + const char *password;
> + } creds;
> +};
> +
> +
>  static NTSTATUS netr_set_machine_account_password(TALLOC_CTX *mem_ctx,
>    struct auth_session_info *session_info,
>    struct messaging_context *msg_ctx,
>    const char *account_name,
> -  struct samr_Password *nt_hash)
> +  struct _samr_Credentials_t *cr)
>  {
>   NTSTATUS status;
>   NTSTATUS result = NT_STATUS_OK;
> @@ -1157,9 +1171,11 @@ static NTSTATUS netr_set_machine_account_password(TALLOC_CTX *mem_ctx,
>   uint32_t acct_ctrl;
>   union samr_UserInfo *info;
>   struct samr_UserInfo18 info18;
> + struct samr_UserInfo26 info26;
>   DATA_BLOB in,out;
>   int rc;
>   DATA_BLOB session_key;
> + enum samr_UserInfoLevel infolevel;
>  
>   ZERO_STRUCT(user_handle);
>  
> @@ -1232,22 +1248,44 @@ static NTSTATUS netr_set_machine_account_password(TALLOC_CTX *mem_ctx,
>   goto out;
>   }
>  
> - ZERO_STRUCT(info18);
> + switch(cr->cred_type) {
> + case CRED_TYPE_NT_HASH:
> + ZERO_STRUCT(info18);
> +
> + infolevel = UserInternal1Information;
> +
> + in = data_blob_const(cr->creds.nt_hash, 16);
> + out = data_blob_talloc_zero(mem_ctx, 16);
> + sess_crypt_blob(&out, &in, &session_key, true);
> + memcpy(info18.nt_pwd.hash, out.data, out.length);
> +
> + info18.nt_pwd_active = true;
> +
> + info->info18 = info18;
> + break;
> + case CRED_TYPE_PLAIN_TEXT:
> + ZERO_STRUCT(info26);
>  
> - in = data_blob_const(nt_hash->hash, 16);
> - out = data_blob_talloc_zero(mem_ctx, 16);
> - sess_crypt_blob(&out, &in, &session_key, true);
> - memcpy(info18.nt_pwd.hash, out.data, out.length);
> + infolevel = UserInternal5InformationNew;
>  
> - info18.nt_pwd_active = true;
> + init_samr_CryptPasswordEx(cr->creds.password,
> +  &session_key,
> +  &info26.password);
>  
> - info->info18 = info18;
> + info26.password_expired = PASS_DONT_CHANGE_AT_NEXT_LOGON;
> + info->info26 = info26;
> + break;
> + default:
> + status = NT_STATUS_INTERNAL_ERROR;
> + goto out;
> + break;
> + }
>  
>   become_root();
>   status = dcerpc_samr_SetUserInfo2(h,
>    mem_ctx,
>    &user_handle,
> -  UserInternal1Information,
> +  infolevel,
>    info,
>    &result);
>   unbecome_root();
> @@ -1277,6 +1315,7 @@ NTSTATUS _netr_ServerPasswordSet(struct pipes_struct *p,
>   NTSTATUS status = NT_STATUS_OK;
>   int i;
>   struct netlogon_creds_CredentialState *creds = NULL;
> + struct _samr_Credentials_t cr = { CRED_TYPE_NT_HASH, {0}};
>  
>   DEBUG(5,("_netr_ServerPasswordSet: %d\n", __LINE__));
>  
> @@ -1311,11 +1350,12 @@ NTSTATUS _netr_ServerPasswordSet(struct pipes_struct *p,
>   DEBUG(100,("%02X ", r->in.new_password->hash[i]));
>   DEBUG(100,("\n"));
>  
> + cr.creds.nt_hash = r->in.new_password;
>   status = netr_set_machine_account_password(p->mem_ctx,
>     p->session_info,
>     p->msg_ctx,
>     creds->account_name,
> -   r->in.new_password);
> +   &cr);
>   return status;
>  }
>  
> @@ -1330,7 +1370,7 @@ NTSTATUS _netr_ServerPasswordSet2(struct pipes_struct *p,
>   struct netlogon_creds_CredentialState *creds = NULL;
>   DATA_BLOB plaintext;
>   struct samr_CryptPassword password_buf;
> - struct samr_Password nt_hash;
> + struct _samr_Credentials_t cr = { CRED_TYPE_PLAIN_TEXT, {0}};
>  
>   become_root();
>   status = netr_creds_server_step_check(p, p->mem_ctx,
> @@ -1353,6 +1393,10 @@ NTSTATUS _netr_ServerPasswordSet2(struct pipes_struct *p,
>   return status;
>   }
>  
> + DEBUG(3,("_netr_ServerPasswordSet2: Server Password Seti2 by remote "
> + "machine:[%s] on account [%s]\n",
> + r->in.computer_name, creds->computer_name));
> +
>   memcpy(password_buf.data, r->in.new_password->data, 512);
>   SIVAL(password_buf.data, 512, r->in.new_password->length);
>  
> @@ -1362,18 +1406,23 @@ NTSTATUS _netr_ServerPasswordSet2(struct pipes_struct *p,
>   netlogon_creds_arcfour_crypt(creds, password_buf.data, 516);
>   }
>  
> - if (!extract_pw_from_buffer(p->mem_ctx, password_buf.data, &plaintext)) {
> + if (!decode_pw_buffer(p->mem_ctx,
> +      password_buf.data,
> +      (char**) &plaintext.data,
> +      &plaintext.length,
> +      CH_UTF16)) {
> + DEBUG(2,("_netr_ServerPasswordSet2: unable to extract password "
> + "from a buffer. Rejecting auth request as a wrong password\n"));
>   TALLOC_FREE(creds);
>   return NT_STATUS_WRONG_PASSWORD;
>   }
>  
> - mdfour(nt_hash.hash, plaintext.data, plaintext.length);
> -
> + cr.creds.password = (const char*) plaintext.data;
>   status = netr_set_machine_account_password(p->mem_ctx,
>     p->session_info,
>     p->msg_ctx,
>     creds->account_name,
> -   &nt_hash);
> +   &cr);
>   TALLOC_FREE(creds);
>   return status;
>  }
> --
> 2.9.3
>
>
> From 5cdc7927e7cef473252ccf34b0055b4749c88cc3 Mon Sep 17 00:00:00 2001
> From: Alexander Bokovoy <[hidden email]>
> Date: Tue, 18 Apr 2017 18:28:29 +0300
> Subject: [PATCH 2/2] s3-tests: assignement in shell shall have no spaces
>  around equal sign
>
> When assigning value to 'failed', no spaces should be around '=' sign.
>
> Signed-off-by: Alexander Bokovoy <[hidden email]>
> ---
>  source3/script/tests/test_net_cred_change.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/source3/script/tests/test_net_cred_change.sh b/source3/script/tests/test_net_cred_change.sh
> index 9013d07..de56be5 100755
> --- a/source3/script/tests/test_net_cred_change.sh
> +++ b/source3/script/tests/test_net_cred_change.sh
> @@ -9,8 +9,8 @@ fi
>  
>  incdir=`dirname $0`/../../../testprogs/blackbox
>  . $incdir/subunit.sh
> -testit "first change" $VALGRIND $BINDIR/wbinfo -c || failed =`expr $failed + 1`
> -testit "first join" $VALGRIND $BINDIR/net rpc testjoin $@ || failed =`expr $failed + 1`
> -testit "second change" $VALGRIND $BINDIR/wbinfo -c || failed =`expr $failed + 1`
> +testit "first change" $VALGRIND $BINDIR/wbinfo -c || failed=`expr $failed + 1`
> +testit "first join" $VALGRIND $BINDIR/net rpc testjoin $@ || failed=`expr $failed + 1`
> +testit "second change" $VALGRIND $BINDIR/wbinfo -c || failed=`expr $failed + 1`
>  
>  testok $0 $failed
> --
> 2.9.3
>


Loading...