Bug 14851 - Do not upper case FQDN hostname based target principal in s4 librpc code
Summary: Do not upper case FQDN hostname based target principal in s4 librpc code
Status: ASSIGNED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: DCE-RPCs and pipes (show other bugs)
Version: 4.15.0
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Stefan Metzmacher
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-10-01 11:01 UTC by Alexander Bokovoy
Modified: 2021-10-30 06:16 UTC (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Bokovoy 2021-10-01 11:01:39 UTC
As found while testing dcerpcd changes in MR#1948, https://gitlab.com/samba-team/samba/-/merge_requests/1948#note_621616533, there is a behavior difference to MS-RPCE section 2.1.1.1 in s4's librpc code:

-----------------------------------------------------------------------------

I am using Samba Python bindings to connect to LSA pipe. Below is a fragment:
```
# -*- coding: utf-8 -*-
from samba import param
from samba import credentials
from samba.dcerpc import lsa

def get_lp(realm):
    lp = param.LoadParm()
    # lp.load_default()
    lp.set('realm',realm)
    lp.set('log level','100')
    lp.set('workgroup', 'ipa')
    lp.set('netbios name', 'dc')
    return lp

lp = get_lp("ipa.test")
creds = credentials.Credentials()
creds.set_username('admin')
creds.set_password('Secret123')
creds.set_kerberos_state(credentials.MUST_USE_KERBEROS)
creds.guess(lp)
creds.set_workstation("dc")

clsa = lsa.lsarpc('ncacn_ip_tcp:dc.ipa.test[sign,seal,krb5]', lp, creds)
objectAttribute = lsa.ObjectAttribute()
objectAttribute.sec_qos = lsa.QosInfo()
policy_handle = clsa.OpenPolicy2("", objectAttribute,
                                 (lsa.LSA_POLICY_VIEW_LOCAL_INFORMATION |
                                  lsa.LSA_POLICY_TRUST_ADMIN |
                                  lsa.LSA_POLICY_CREATE_SECRET)
                                )

```

Running it with `KRB5_TRACE=/dev/stderr python creds.py` shows that Samba RPC client code wants to use upper case FQDN hostname-based principal for the target service (`host/DC.IPA.TEST@IPA.TEST`), due to the code in `source4/librpc/rpc/dcerpc_sock.c:continue_socket_connect()` which unconditionally capitalizes the server name. This is unrelated to this MR but it would be good to clarify this without a need to require uppercased keys in server keytabs -- we never needed this in FreeIPA when running RPC services behind smbd.

According to MS-RPCE 2.1.1.1,

> When extensions that are not specified in sections 2.1.1 through 2.1.2 are enabled over the TCP transport protocol, the network address MUST be an IPv4 or IPv6 address or a server name.<2> The server name MUST be a Unicode string that represents either a NetBIOS host name (see [MS-NBTE] section 2.2.1) or a fully qualified domain name (see [RFC1035] section 3.1 and [RFC2181] section 11).

where NetBIOS name is case-sensitive and FQDN name is insensitive, so uppercasing them in all cases looks incorrect.

When I forced the target principal with `target_principal=cifs/dc.ipa.test@IPA.TEST` in the binding string, the auth passed and I was able to reach the LSA end-point authenticated.

-----------------------------------------------------------------------------

I think librpc should not upper case FQDN hostname-based principal for the target service.
Comment 1 Alexander Bokovoy 2021-10-01 11:03:06 UTC
Metze, I think this is a bug and needs fixed. I didn't get any comment on this on MR#1948 so I am filing a bug now as after merging it we'll see a regression in FreeIPA due to how librpc changes the hostname in the principal.
Comment 2 Jeremy Allison 2021-10-01 17:30:07 UTC
Alexander, how do we know when to capitalize the name here - i.e. when do we know it's a NetBIOS name that needs uppercasing ? Is it as simple as looking for a '.' in the name ?
Comment 3 Alexander Bokovoy 2021-10-01 17:45:38 UTC
My opinion is that we should not normalize the name at all. NetBIOS names are case sensitive and strictly speaking should not be capitalized.
See MS-NBTE 2.2.1: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-nbte/6f06fa0e-1dc4-4c41-accb-355aaf20546d

-----
This document clarifies the ambiguity by specifying that the name space is defined as sixteen 8-bit binary bytes, with no restrictions, except that the name SHOULD NOT<3> start with an asterisk (*).

Neither [RFC1001] nor [RFC1002] discusses whether names are case-sensitive. This document clarifies this ambiguity by specifying that because the name space is defined as sixteen 8-bit binary bytes, a comparison MUST be done for equality against the entire 16 bytes. As a result, NetBIOS names are inherently case-sensitive.
-----
Comment 4 Jeremy Allison 2021-10-01 17:52:22 UTC
Or do we just need to talloc_strdup() here and make sure the correct canonicalization is done in the callers of continue_socket_connect() (which IMHO would seem to be the correct thing to do) ?
Comment 5 Alexander Bokovoy 2021-10-01 17:54:25 UTC
I think that could be a reasonable fix. I would still suggest to not normalize the NetBIOS names — we've had customers with very weird names in past, even though it might be not a problem anymore.
Comment 6 Jeremy Allison 2021-10-01 17:57:06 UTC
(In reply to Alexander Bokovoy from comment #3)

So the correct fix is to just change this to talloc_strdup(), and expect the callers to have done any name canonicalization if they want it.

Sounds good - what would a regression test for this look like ?

(Ignoring https://gitlab.com/samba-team/samba/-/merge_requests/1948 as it isn't strictly related to it but this needs to be fixed before it can go in).
Comment 7 Alexander Bokovoy 2021-10-01 18:15:46 UTC
Regression test would be something similar to what I have in the description:

 - have a DC environment
 - enroll a client with mixed-case NetBIOS name
 - try to connect to it over ncacn_ip_tcp with Kerberos ticket

Looking into testprogs/blackbox/test_net_ads.sh, it already uses low-case NetBIOS name there for the enrolled client (sha1sum gives us low-cased output). However, in those tests we use s3 code, not s4 librpc so a test could be simply to extend that one to use s4's python bindings.
Comment 8 Jeremy Allison 2021-10-01 18:29:04 UTC
OK, I might need some help on that ("simply to extend that one to use s4's python bindings" :-). The "simply" part isn't obvious to me :-).

Can we make a test that fails first (mark as knownfail) and then removes it after the code change currently under test at:

https://gitlab.com/samba-team/devel/samba/-/pipelines/381100689

passes ? (Presuming it does pass, of course :-).
Comment 9 Jeremy Allison 2021-10-01 18:32:34 UTC
(In reply to Alexander Bokovoy from comment #7)

Also, correct me if I'm wrong but testprogs/blackbox/test_net_ads.sh is creating the machine accounts in AD, but isn't spinning up a member server to connect to.

So the "try to connect to it" part won't work unless we actually have a running member I think.
Comment 10 Alexander Bokovoy 2021-10-01 18:32:47 UTC
Looking at other tests, it supposed to have worked already:

$ git grep -A1 torture_suite_add_machine_workstation_rpc_iface_tcase
source4/torture/ntp/ntp_signd.c:        tcase = torture_suite_add_machine_workstation_rpc_iface_tcase(suite,
source4/torture/ntp/ntp_signd.c-                                  "signd", &ndr_table_netlogon, TEST_MACHINE_NAME);
--
source4/torture/rpc/netlogon.c: tcase = torture_suite_add_machine_workstation_rpc_iface_tcase(suite, "wkst",
source4/torture/rpc/netlogon.c-                                           &ndr_table_netlogon, TEST_MACHINE_NAME);
--
source4/torture/rpc/remote_pac.c:       tcase = torture_suite_add_machine_workstation_rpc_iface_tcase(suite, "netr-mem-arcfour",
source4/torture/rpc/remote_pac.c-                                                                     &ndr_table_netlogon, TEST_MACHINE_NAME_WKSTA);
--
source4/torture/rpc/remote_pac.c:       tcase = torture_suite_add_machine_workstation_rpc_iface_tcase(suite, "netr-mem-aes",
source4/torture/rpc/remote_pac.c-                                                                     &ndr_table_netlogon, TEST_MACHINE_NAME_WKSTA);
--
source4/torture/rpc/remote_pac.c:       tcase = torture_suite_add_machine_workstation_rpc_iface_tcase(suite, "netr-mem-arcfour",
source4/torture/rpc/remote_pac.c-                                                                     &ndr_table_netlogon, TEST_MACHINE_NAME_S4U2SELF_WKSTA);
--
source4/torture/rpc/remote_pac.c:       tcase = torture_suite_add_machine_workstation_rpc_iface_tcase(suite, "netr-mem-aes",
source4/torture/rpc/remote_pac.c-                                                                     &ndr_table_netlogon, TEST_MACHINE_NAME_S4U2SELF_WKSTA);
--
source4/torture/rpc/remote_pac.c:       tcase = torture_suite_add_machine_workstation_rpc_iface_tcase(suite, "netr-mem-arcfour",
source4/torture/rpc/remote_pac.c-                                                                     &ndr_table_netlogon, TEST_MACHINE_NAME_S4U2PROXY_WKSTA);
--
source4/torture/rpc/remote_pac.c:       tcase = torture_suite_add_machine_workstation_rpc_iface_tcase(suite, "netr-mem-aes",
source4/torture/rpc/remote_pac.c-                                                                     &ndr_table_netlogon, TEST_MACHINE_NAME_S4U2PROXY_WKSTA);
--
source4/torture/rpc/rpc.c:_PUBLIC_ struct torture_rpc_tcase *torture_suite_add_machine_workstation_rpc_iface_tcase(
source4/torture/rpc/rpc.c-                              struct torture_suite *suite,
--
source4/torture/rpc/samr_accessmask.c:  tcase = torture_suite_add_machine_workstation_rpc_iface_tcase(suite, "samr",
source4/torture/rpc/samr_accessmask.c-                                                                &ndr_table_samr,
--
source4/torture/rpc/spoolss_access.c:   rpc_tcase = torture_suite_add_machine_workstation_rpc_iface_tcase(suite, "workstation",
source4/torture/rpc/spoolss_access.c-                                                                     &ndr_table_spoolss,
--
source4/torture/rpc/torture_rpc.h:struct torture_rpc_tcase *torture_suite_add_machine_workstation_rpc_iface_tcase(
source4/torture/rpc/torture_rpc.h-                              struct torture_suite *suite, 

$ git grep '#define TEST_MACHINE_NAME'
source4/torture/ntp/ntp_signd.c:#define TEST_MACHINE_NAME "ntpsigndtest"
source4/torture/rpc/drsuapi.c:#define TEST_MACHINE_NAME "torturetest"
source4/torture/rpc/drsuapi_w2k8.c:#define TEST_MACHINE_NAME "torturetest"
source4/torture/rpc/forest_trust.c:#define TEST_MACHINE_NAME "lsatestmach"
source4/torture/rpc/netlogon.c:#define TEST_MACHINE_NAME "torturetest"
source4/torture/rpc/netlogon_crypto.c:#define TEST_MACHINE_NAME "torturetest"
source4/torture/rpc/remote_pac.c:#define TEST_MACHINE_NAME_BDC "torturepacbdc"
source4/torture/rpc/remote_pac.c:#define TEST_MACHINE_NAME_WKSTA "torturepacwksta"
source4/torture/rpc/remote_pac.c:#define TEST_MACHINE_NAME_S4U2SELF_BDC "tests4u2selfbdc"
source4/torture/rpc/remote_pac.c:#define TEST_MACHINE_NAME_S4U2SELF_WKSTA "tests4u2selfwk"
source4/torture/rpc/remote_pac.c:#define TEST_MACHINE_NAME_S4U2PROXY_WKSTA "tests4u2proxywk"
source4/torture/rpc/samlogon.c:#define TEST_MACHINE_NAME "samlogontest"
source4/torture/rpc/samsync.c:#define TEST_MACHINE_NAME "samsynctest"
source4/torture/rpc/schannel.c:#define TEST_MACHINE_NAME "schannel"

all those names are low-cased, however, the problem is not visible because ads_keytab_create_default() creates keytab entries with upper-cased name as well so they practically make the regression invisible.

I think we can actually have a test that filters out upper-cased name from a machine keytab and force use of the specific keytab in smbd configuration. Then uppercased version will not be present and rpc code will fail without a fix.
Comment 11 Jeremy Allison 2021-10-01 18:35:18 UTC
Woohoo ! That's the kind of insight I didn't have :-). Any chance I can impose on you to write this test ? I will certainly learn lots by reviewing it :-).
Comment 12 Alexander Bokovoy 2021-10-01 18:37:45 UTC
It is getting late here and my weekend is already packed (joys of post-house move cleanup/setup, heh) so I will try to find time next week for this test. At worst, I'll try to create something similar to IPA smbd setup as I need that anyway (without ipasam).
Comment 13 Jeremy Allison 2021-10-01 18:44:19 UTC
Ah, the ci run here:

https://gitlab.com/samba-team/devel/samba/-/pipelines/381100689

fails spectacularly with the minor 'talloc_strupper() -> talloc_strdup()' change :-(.

I think there may be more to the name canonicalization in source4 that we're seeing here at first glance (i.e. I'm guessing it's not being done right in a bunch of places).
Comment 14 Jeremy Allison 2021-10-01 19:25:31 UTC
Ignore the previous comment :-). Turns out talloc_strupper() and talloc_strdup() behave differently on being passed a NULL pointer :-).

Looks like this is the raw fix:

diff --git a/source4/librpc/rpc/dcerpc_sock.c b/source4/librpc/rpc/dcerpc_sock.c
index e7ecca73e3c..c1f1ee4664c 100644
--- a/source4/librpc/rpc/dcerpc_sock.c
+++ b/source4/librpc/rpc/dcerpc_sock.c
@@ -94,7 +94,14 @@ static void continue_socket_connect(struct composite_context *ctx)
        conn->srv_max_recv_frag = 5840;
 
        conn->transport.pending_reads = 0;
-       conn->server_name   = strupper_talloc(conn, s->target_hostname);
+       if (s->target_hostname != NULL) {
+               conn->server_name = talloc_strdup(conn, s->target_hostname);
+               if (conn->server_name == NULL) {
+                       close(sock_fd);
+                       composite_error(c, NT_STATUS_NO_MEMORY);
+                       return;
+               }
+       }
 
        rc = tstream_bsd_existing_socket(conn, sock_fd,
                                         &conn->transport.stream);
Comment 15 Jeremy Allison 2021-10-01 19:50:02 UTC
Yep, that was it. Passing ci now with the NULL check added :-).
Comment 17 Jeremy Allison 2021-10-01 21:18:44 UTC
Yep, now passes ci ! We just need the regression test and we'll be good to go I think (unless Metze can see something I can't).
Comment 18 Stefan Metzmacher 2021-10-03 13:18:25 UTC
(In reply to Alexander Bokovoy from comment #0)

servicePrincipalName values are case insensitive and a kerberos acceptor
should cope with any value or I'm I missing something?
Comment 19 Stefan Metzmacher 2021-10-03 13:21:03 UTC
(In reply to Jeremy Allison from comment #14)

> -       conn->server_name   = strupper_talloc(conn, s->target_hostname);
> +       if (s->target_hostname != NULL) {
> +               conn->server_name = talloc_strdup(conn, s->target_hostname);
> +               if (conn->server_name == NULL) {
> +                       close(sock_fd);
> +                       composite_error(c, NT_STATUS_NO_MEMORY);
> +                       return;
> +               }
> +       }

I don't against such a change, but I don't understand why this would
any difference to the problem Alexander described...
Comment 20 Alexander Bokovoy 2021-10-03 15:34:46 UTC
I am not sure why but this broke when I tested with the code in MR#1948.

FreeIPA itself does support case-insensitive TGS request and it works on Fedora 33 setup I have at home but breaks with MR#1948 test environment I had.

I'll do additional testing tomorrow and report about it.
Comment 21 Jeremy Allison 2021-10-03 23:35:58 UTC
(In reply to Stefan Metzmacher from comment #19)

I think this fix is the right thing to do - in no other place do we modify the case of the server name so it does seem a little odd to me that we arbitrarily uppercase it here.

It doesn't make a difference in our ci-tests but I really would like to fully understand it before pushing anything though :-).
Comment 22 Jeremy Allison 2021-10-07 15:50:26 UTC
Ping Alexander. Can you give us an update on any investigations of this ? (I'm too busy is of course a valid update :-).
Comment 23 Alexander Bokovoy 2021-10-07 15:56:07 UTC
Yes, sadly that was my case. I hoped to get it done on Monday and now it is end of Thursday...

I'll try to carve some time on Friday. Sorry for the delay...
Comment 24 Jeremy Allison 2021-10-07 16:40:24 UTC
No rush Alexander, Ralph wants metze to look over the code in https://gitlab.com/samba-team/samba/-/merge_requests/1948 before we merge and he's busy for a week or so anyway. If you don't get to it until next week there's no harm.
Comment 25 Jeremy Allison 2021-10-29 22:00:32 UTC
Ping ! Just want to let you know I haven't forgotten about this one :-).

We'll need a fix in the next 2 weeks or so once Metze evaluates the dcerpcd changes (soon...) :-).
Comment 26 Alexander Bokovoy 2021-10-30 06:16:25 UTC
I haven't forgotten either but been busy last two weeks with somewhat more urgent work on a set of other bugzillas (together with Metze and others). Hope to get my part finalized soon too.