Created attachment 17292 [details]
patch

The reproducer crashes winbind with wbinfo -a

Created attachment 17293 [details]
patch

Ping ? Do we need a CVE for this or can we just fix with the attached patch ?

If we need a CVE it would be nice to get this in with the upcoming security release.

According to Volker via Ralph this crashes the parent winbindd, so we need a CVE for this.

Requested a CVE from Red Hat product security.

OK, checked this some more and it's using WINBINDD_PAM_AUTH_CRAP, which is only available on the winbind privilaged pipe via wbcRequestResponsePriv().

In this case, is it a CVE-worthy bug ? I'm inclined to think this is now a self-DOS (you already need root to trigger it), which by default we don't treat as a CVE.

Ralph, can you help make an executive decision here when you're back from vacation ?

Thanks,

Jeremy.

Do we still want the CVE? Apparently CVE-2022-2127 was offered.

Looks like this one got dropped. Ralph, do you want to make the executive decision here ? As it seems to be a self-root DoS I think we can just fix and move on.

I think, but have not proved, that the length of an LM element from an NTLMSSP packet parsed by ntlm_auth (eg as presented to squid or mod_auth_ntlm_winbind) is passed along here.

Is comment 9 countering comment 8 and saying maybe we really do need the CVE?

(In reply to Douglas Bagnall from comment #10)
Yes, that is my view.

CVSS3.1:AV:N/AC:H/PR:N/UI:N/S:U/C:N/I:N/A:H (5.9) on the basis that it would, if done successfully, crash the parent winbindd.  Attack complexity is "High" as I so far have been unable to make it crash, but have proved the flow described in comment #9 is possible.

Hi Volker!

Can you draft a CVE advisory text please? Cf the template in the team repo at doc/releases/template_security_advisory.txt.

Thanks!

Created attachment 17909 [details]
draft advisory

Comment on attachment 17909 [details]
draft advisory

Thanks! Lgtm.

Created attachment 17923 [details]
Patch for master

We should make really clear that the exploitable path is only via ntlm_auth (to remote users) or local privileged users (with rights to access the privileged socket).

The reason is the differences between the access libraries:

this in wbc_pam.c (as used by auth_winbind in smbd as a file server)

		request.data.auth_crap.lm_resp_len =
				MIN(params->password.response.lm_length,
				    sizeof(request.data.auth_crap.lm_resp));

this is ntlm_auth.c
		request.data.auth_crap.lm_resp_len = lm_response->length;

(The AD DC, from the source4 auth_winbind module, does a SamLogon over IRPC, and is not using the winbind pipe protocol)

(In reply to Andrew Bartlett from comment #18)
I guess what you're saying is that the advisory should be expanded a bit to cover this? I see what I can do. :)

(In reply to Ralph Böhme from comment #19)
Ok, so what about adding this paragraph to the description:

To exploit this vulnarebility the user must have either access to the
priviliged winbindd UNIX domain socket or, if the system is running
Samba ntlm_auth as authentication backend for services like Squid or
FreeRADIUS, the vulnarebility is remotely exploitable.

(In reply to Ralph Böhme from comment #20)
> To exploit this vulnarebility the user must have either access to the
> priviliged winbindd UNIX domain socket or, if the system is running
        ^
  I think this is "privileged"

and I would add "(which means being root already)" to that sentence.

(In reply to Volker Lendecke from comment #21)
Fixed and added. Thanks!

Doesn't the ntlm_auth vector also imply Attack Complexity Low instead of High which gives a CVSS score of 7.5?

What about chaning the CVSS section to:

---8<---
Systems without ntlm_auth configured:

CVSS3.1:AV:N/AC:H/PR:N/UI:N/S:U/C:N/I:N/A:H (5.9)

With ntlm_auth configured:

CVSS3.1:AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H (7.5)
---8<---

(In reply to Ralph Böhme from comment #22)
For the case with ntlm_auth I had the attack complexity as high as I couldn't make it crash 'on demand' even in the  controlled conditions in testenv.  I did however show the overflow with valgrind or address santitizer (I can't recall which), but that flow path including what you can pass over NTLMSSP actually has some restrictions (thankfully!), you can't just set the length to 0x1000000. 

The local case that Volker reproduced in comment #1 has fewer restrictions on the length, but has privileges required.  

I rate it AV:L/AC:L/PR:H/UI:N/S:U/C:N/I:N/A:H (4.4) for the local case (no ntlm_auth in use).

Created attachment 17924 [details]
Advisory v2

I wonder whether we should do an additional change to ntlm_auth that fixes setting the correct lm buffer size, like so:

--- a/source3/utils/ntlm_auth.c
+++ b/source3/utils/ntlm_auth.c
@@ -575,10 +575,14 @@ NTSTATUS contact_winbind_auth_crap(const char *username,
        memcpy(request.data.auth_crap.chal, challenge->data, MIN(challenge->length, 8));
 
        if (lm_response && lm_response->length) {
+               size_t capped_lm_response_len = MIN(
+                       lm_response->length,
+                       sizeof(request.data.auth_crap.lm_resp));
+
                memcpy(request.data.auth_crap.lm_resp,
                       lm_response->data,
-                      MIN(lm_response->length, sizeof(request.data.auth_crap.lm_resp)));
-               request.data.auth_crap.lm_resp_len = lm_response->length;
+                      capped_lm_response_len);
+               request.data.auth_crap.lm_resp_len = capped_lm_response_len;
        }
 
Thoughts?

Comment on attachment 17924 [details]
Advisory v2

Thanks, this is better but to be clear, the winbind privileged socket is not root only, the typical pattern for use with ntlm_auth is to have it made available to a group that you add eg FreeRADIUS, apache2 or squid to. 

Say:

(which means being root or in a special group already, typically assigned to services like FreeRADIUS or Squid that use the ntlm_auth tool)

For workaround, say:

Removing non-root access to the winbind privileged socket (***with a detail on what that is and particularly where that is***), however this will disable use of ntlm_auth for use in (eg) Squid or FreeRADIUS.

Created attachment 17926 [details]
Advisory v3

I think this is clearer, and rather than ask to have you fix things and ask me back, I had a go at updating the text.

The workaround instructions should be manually tested (I've not done that), but a chgrp to root should be harmless. 

I'm happy for my words to be further updated, I am just keen to help where I can with the text.

Comment on attachment 17926 [details]
Advisory v3

Excellent, thanks! Saves me a lot of headscratching... :)

(In reply to Ralph Böhme from comment #25)
ping

Created attachment 17927 [details]
Patch for master

Comment on attachment 17927 [details]
Patch for master

There's a typo in the 2nd commit message (lanmen vs lanman), but apart from that it's good.

Created attachment 17939 [details]
Patch for 4.18

Created attachment 17940 [details]
Patch for 4.17

Created attachment 17949 [details]
Patch for 4.16

Created attachment 17954 [details]
Patch for master

Fixed typo lanmen -> lanman

Created attachment 17955 [details]
Patch for 4.18

Fixed typo lanmen -> lanman

Created attachment 17956 [details]
Patch for 4.17

Created attachment 17957 [details]
Patch for 4.16

Fixed typo lanmen -> lanman

Proposed release date for this CVE is the 19th of July.

Removing vendor CC (so that any public comments don't need to be broadcast so widely) and opening these bugs to the public.
If you wish to continue to be informed about any changes here please CC individually.

This bug was referenced in samba v4-16-stable (Release samba-4.16.11):

5c6fe5a491b16bb658c191cfafb5edc0beb5fab2
2eabbe31f64a8456813a502afb05907beb46ffad

This bug was referenced in samba v4-17-stable (Release samba-4.17.10):

a3944de6990686bf674e7a9badded501873a7cfa
53838682570135b753fa622dfcde111528563c2d

This bug was referenced in samba v4-18-stable (Release samba-4.18.5):

19dcb036cb8d21bf4e3e30d81eee3c79e54d3eff
b09567397c2f394ca224c189cfbb1bee9688a96f

This bug was referenced in samba v4-16-test:

5c6fe5a491b16bb658c191cfafb5edc0beb5fab2
2eabbe31f64a8456813a502afb05907beb46ffad

This bug was referenced in samba v4-17-test:

a3944de6990686bf674e7a9badded501873a7cfa
53838682570135b753fa622dfcde111528563c2d

This bug was referenced in samba v4-18-test:

19dcb036cb8d21bf4e3e30d81eee3c79e54d3eff
b09567397c2f394ca224c189cfbb1bee9688a96f

This bug was referenced in samba master:

b2de71734f09ee4eb80cf70de8a66f628246f2ba
e067c523b17951a93b6daafd349e9371f8f81e56

Pushed to all branches.
Closing out bug report.
Thanks!

This bug was referenced in samba v4-19-test:

b2de71734f09ee4eb80cf70de8a66f628246f2ba
e067c523b17951a93b6daafd349e9371f8f81e56

This bug was referenced in samba v4-19-stable (Release samba-4.19.0rc1):

b2de71734f09ee4eb80cf70de8a66f628246f2ba
e067c523b17951a93b6daafd349e9371f8f81e56