From "Mitter, Lukas" <lmi@codemanufaktur.com> via security@samba.org: To whom it may concern, I am writing in regards to an unexpected behaviour on samba version 4.17.4. We have done a routine check whether an unprivileged user can modify attributes on objects. The unprivileged user was able to delete the dNSHostName attribute via "ldapmodify" and after further testing we found out that it was only possible via a specific .ldif-File. The following .ldif-File was able to delete the attribute via ldapmodify: dn: CN=PC0079-2,OU=Inactive,OU=Computers,OU=SecurityObjects,DC=ad,DC=codemanufaktur,DC=com changetype: modify delete: dNSHostName The output of ldapmodify is as follows: modifying entry "CN=PC0079-2,OU=Inactive,OU=Computers,OU=SecurityObjects,DC=ad,DC=codemanufaktur,DC=com" The similarly built .ldif-File which also should have the exact same behaviour in the example selected, but was not able to delete the attribute is the following: dn: CN=PC0079-2,OU=Inactive,OU=Computers,OU=SecurityObjects,DC=ad,DC=codemanufaktur,DC=com changetype: modify delete: dNSHostName dNSHostName: PC0079-2.ad.codemanufaktur.com The output of ldapmodify with this different .ldif-File is as follows: modifying entry "CN=PC0079-2,OU=Inactive,OU=Computers,OU=SecurityObjects,DC=ad,DC=codemanufaktur,DC=com" ldap_modify: Insufficient access (50) additional info: error in module acl: insufficient access rights during LDB_MODIFY (50) We expected the behaviour to be consistent on both instances and not diverge. We also expected that in both cases the unprivileged user is unable to delete the attribute "dNSHostName". We haven't found this inconsistent behaviour on any other attribute, it only affects dNSHostName as it seems. We have last attempted this exact same ldapmodify on the 1st July with samba version 4.16.x installed and at that point in time the dNSHostName Attribute could not be deleted. Kind Regards
There was a bit of work in this area during 4.17, In particular CVE-2022-32743 https://bugzilla.samba.org/show_bug.cgi?id=14833.
Created attachment 17718 [details] test confirming the behaviour The attachment adds tests that belong in acl.py, but acl.py is too big and unwieldy to iterate over easily, so really the answer is for acl.py to be split up. Anyway, in a testenv, the unprivileged host name delete works if no host name is specified, whether by ldif, or an empty message (as you'd expect). $ python3 /home/douglasb/src/samba-review/source4/dsdb/tests/python/acl_modify.py $SERVER -U"$USERNAME%$PASSWORD" --workgroup=$DOMAIN time: 2023-01-04 08:39:51.755353Z test: __main__.AclModifyTests.test_modify_delete_dns_host_name_ldif_specified baseDN: DC=addom,DC=samba,DC=example,DC=com time: 2023-01-04 08:39:51.867180Z time: 2023-01-04 08:39:51.867225Z successful: __main__.AclModifyTests.test_modify_delete_dns_host_name_ldif_specified time: 2023-01-04 08:39:51.867361Z test: __main__.AclModifyTests.test_modify_delete_dns_host_name_ldif_unspecified baseDN: DC=addom,DC=samba,DC=example,DC=com time: 2023-01-04 08:39:51.978212Z time: 2023-01-04 08:39:51.978715Z failure: __main__.AclModifyTests.test_modify_delete_dns_host_name_ldif_unspecified [ Traceback (most recent call last): File "/home/douglasb/src/samba-review/source4/dsdb/tests/python/acl_modify.py", line 228, in test_modify_delete_dns_host_name_ldif_unspecified self.assertRaisesLdbError( File "/home/douglasb/src/samba-review/bin/python/samba/tests/__init__.py", line 244, in assertRaisesLdbError self.fail("%s, expected " AssertionError: User able to delete dNSHostName (without specific name), expected LdbError ERR_INSUFFICIENT_ACCESS_RIGHTS, (50) but we got success ] time: 2023-01-04 08:39:51.978858Z test: __main__.AclModifyTests.test_modify_delete_dns_host_name_specified baseDN: DC=addom,DC=samba,DC=example,DC=com time: 2023-01-04 08:39:52.089607Z time: 2023-01-04 08:39:52.089666Z successful: __main__.AclModifyTests.test_modify_delete_dns_host_name_specified time: 2023-01-04 08:39:52.089808Z test: __main__.AclModifyTests.test_modify_delete_dns_host_name_unspecified baseDN: DC=addom,DC=samba,DC=example,DC=com time: 2023-01-04 08:39:52.201214Z time: 2023-01-04 08:39:52.201436Z failure: __main__.AclModifyTests.test_modify_delete_dns_host_name_unspecified [ Traceback (most recent call last): File "/home/douglasb/src/samba-review/source4/dsdb/tests/python/acl_modify.py", line 197, in test_modify_delete_dns_host_name_unspecified self.assertRaisesLdbError( File "/home/douglasb/src/samba-review/bin/python/samba/tests/__init__.py", line 244, in assertRaisesLdbError self.fail("%s, expected " AssertionError: User able to delete dNSHostName (without specified name), expected LdbError ERR_INSUFFICIENT_ACCESS_RIGHTS, (50) but we got success ] time: 2023-01-04 08:39:52.201640Z
It looks like the problem is here, where it seems we forgot the delete case with no values: https://gitlab.com/samba-team/devel/samba/-/blob/master/source4/dsdb/samdb/ldb_modules/acl.c#L949 and we need logic like "if it's a delete modify and not this or that condition, return insufficient rights error" inside the block. But I think I'll wait for Joseph to return because he will know.
Created attachment 17720 [details] test and proposed fix for master This bug was probably my fault. Thanks for spotting it. The attached patch seems to fix the problem by moving the early return to where the dNSHostName value is actually used (which is only in the add or replace case), and having it return an error instead.
I suppose the CVSS score might be: AV:N/AC:L/PR:L/UI:N/S:U/C:N/I:L/A:L (5.4) but I'm not sure to what degree availability would be affected, or whether an attacker could use this to gain extra privileges somehow.
(In reply to Joseph Sutton from comment #5) Yep, it looks to be another contestant in the "non-admin user can be a real pain in the arse" category. Do you have lines of thinking for gaining extra privileges? I am not getting very far, unless the attacker can also change dns records and/or put the dNSHostName somewhere else. I guess we should get a CVE. Also we might need to add tests around modify ADD and REPLACE in case there are regressions.
I couldn't think of anything, besides the possibility of a low-privileged user removing the dNSHostName from another account so as to add it to their own (with Write Property), thus masquerading as the other account; or of a user preventing other accounts from setting an SPN by removing their dNSHostNames. We do have an existing test in acl.py, test_modify_dns_host_name_no_value, that does a REPLACE with no values and expects either success (as with Windows) or ERR_OPERATIONS_ERROR (as with Samba 4.17).
(In reply to Joseph Sutton from comment #7) > We do have an existing test in acl.py, test_modify_dns_host_name_no_value, that > does a REPLACE with no values and expects either success (as with Windows) or > ERR_OPERATIONS_ERROR (as with Samba 4.17). It's not testing exactly the same thing, however (the REPLACE is done under Validated Write privileges). I'll probably add another test to be sure.
(In reply to Joseph Sutton from comment #7) Thankfully we don't do certificate authentication matching on the dnsHostName attribute, as that would be the best way to attack this. But certainly a user could be quite a pain, we use this in DRS to find the other server, for example (thankfully with a fallback). So I score this as CVSS3.1:AV:N/AC:L/PR:L/UI:N/S:U/C:N/I:L/A:L (5.4)
Lukas, I am preparing our advisory now. Can we credit you in our security advisory and do you have an affiliation you would like recorded? Thanks! Andrew Bartlett
Created attachment 17764 [details] Initial advisory without versions Advisory added, with credit. I'll hide this one if the reporter doesn't want their name included.
Comment on attachment 17764 [details] Initial advisory without versions Looks good. Douglas should be credited for providing patches, as he wrote the tests for this.
Created attachment 17765 [details] Updated advisory without versions (v2) Updated to credit Douglas for the patches
Created attachment 17766 [details] Advisory v3
Created attachment 17776 [details] Patch v2 with CVE tag
Andrew, You may credit me. With "affiliation" you are asking which company I work at? In that case you may record it as "codemanufaktur GmbH". Thank you! Lukas Mitter
Created attachment 17812 [details] Advisory v4 I've added the reporter's affiliation.
Created attachment 17813 [details] Patch v2 backported to Samba 4.18
Created attachment 17814 [details] Patch v2 backported to Samba 4.17
Created attachment 17817 [details] Patch v2 for master backported to Samba 4.17 as v2.1 The patch for Samba 4.17 needed some extra prerequisite patches.
Created attachment 17818 [details] Patch v2 for master backported to Samba 4.17 as v2.1
Created attachment 17833 [details] Patch v2 for master backported to Samba 4.17 as v2.2 This updated version fixes the reversal of the first two dependent cherry-picked commits and has been compile tested on each commit, as well as being otherwise identical to the end product that was fully CI tested.
Assigning to Jule for next security release.
We will make a Samba security release for this issue on Wednesday 29 March
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-17-stable (Release samba-4.17.7): b7af8aa2552e0690aac58fb98e3134b71f678ece 307b2e65d51903f6805460a2633ebe809d4052ab 54691236fc80a932f2069eef0aa21d6818445503 888c6ae8177d87e408722f67cc03359ae2533402
This bug was referenced in samba v4-18-stable (Release samba-4.18.1): 016687b3aaea76abf4ae56523aa951547f806e38 003f6c16112a45af81ed59877d3b416a2f3847d9
This bug was referenced in samba v4-17-test: b7af8aa2552e0690aac58fb98e3134b71f678ece 307b2e65d51903f6805460a2633ebe809d4052ab 54691236fc80a932f2069eef0aa21d6818445503 888c6ae8177d87e408722f67cc03359ae2533402
This bug was referenced in samba v4-18-test: 016687b3aaea76abf4ae56523aa951547f806e38 003f6c16112a45af81ed59877d3b416a2f3847d9
This bug was referenced in samba master: 62cc4302b67d33a2fd57738cc9180f7b36d0cb9d c33e78a27fbeb913b08ef7f74343c1f652d1aa41