Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRYPT password hash with asterisk #4817

Closed
rwinter77 opened this issue Jun 29, 2021 · 8 comments · Fixed by #4819
Closed

CRYPT password hash with asterisk #4817

rwinter77 opened this issue Jun 29, 2021 · 8 comments · Fixed by #4819
Assignees
Labels
priority_high need urgent fix / highly valuable / easy to fix
Milestone

Comments

@rwinter77
Copy link

Issue Description
If an entry contains an asterisk as the crypted password hash, binding is possible with any password for this entry
userPassword: {CRYPT}*

Package Version and Platform:

  • Platform: openSUSE Leap 15.2
  • Package and version: 389-ds-1.4.3.23~git0.f53d0132b-lp152.2.15.1.x86_64
  • Browser firefox

Steps to Reproduce
Steps to reproduce the behavior:

  1. Create an entry (e.g. a posixAccount) with the userPassword set to "{CRYPT}*", e.g. by importing it from an ldif file
  2. Try to bind with that entry using an arbitrary password (e.g. "llhh"
  3. Check if the binding was successfull

Expected results
I would expect to fail the binding with any password because the asterisk is not a vaild character in a crypted password.

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Problem occured after importing entries from a NIS database. In NIS (and in /etc/shadow), the asterisk is often used for special users like "nobody".

@rwinter77 rwinter77 added the needs triage The issue will be triaged during scrum label Jun 29, 2021
@Firstyear Firstyear self-assigned this Jun 30, 2021
@Firstyear Firstyear added this to the 1.4.3 milestone Jun 30, 2021
@Firstyear
Copy link
Contributor

@mreynolds389 We can adjust to 1.4.2 if you need to still support that version too :)

@mreynolds389
Copy link
Contributor

@mreynolds389 We can adjust to 1.4.2 if you need to still support that version too :)

Actually 1.4.3 is out of support too. We can backport a fix but there are no more releases being made.

@Firstyear
Copy link
Contributor

Firstyear commented Jun 30, 2021

I need to double check what SUSE is doing but 15.2 may still be in support for us. Happy to just manage this myself, we wont need a release to apply this. Anyway, I've written a test and I'm going to investigate this today.

EDIT: Opps, 1.4.3 is what 15.2 has, so that's fine there. We don't need it for 1.4.2.

@Firstyear
Copy link
Contributor

Firstyear commented Jun 30, 2021

Okay, it looks like the fault is here

   56  	    /* we use salt (first 2 chars) of encoded password in call to crypt_r() */
   57  	    cp = crypt_r(userpwd, dbpwd, &data);
-> 58  	    if (cp) {
   59  	        rc = slapi_ct_memcmp(dbpwd, cp, strlen(dbpwd));
   60  	    } else {
   61  	        rc = -1;
(lldb) print cp
(char *) $9 = 0x00007f696dff2160 "*0"

Were trying to set the salt into crypt_r from dbpwd, but in this case dbpwd is "*", which means that the result that is emitted as cp is "*0". Then ct_memcmp is using the length of dbpwd, which is 1, so we only check the '*'.

There are quite a number of hardening changes here that can prevent this.

Firstyear added a commit to Firstyear/389-ds-base that referenced this issue Jun 30, 2021
…words

Bug Description: Due to mishanding of short dbpwd hashes, the
crypt_r algorithm was misused and was only comparing salts
in some cases, rather than checking the actual content
of the password.

Fix Description: Stricter checks on dbpwd lengths to ensure
that content passed to crypt_r has at least 2 salt bytes and
1 hash byte, as well as stricter checks on ct_memcmp to ensure
that compared values are the same length, rather than potentially
allowing overruns/short comparisons.

fixes: 389ds#4817

Author: William Brown <william@blackhats.net.au>

Review by: ???
@mreynolds389
Copy link
Contributor

I need to double check what SUSE is doing but 15.2 may still be in support for us. Happy to just manage this myself, we wont need a release to apply this. Anyway, I've written a test and I'm going to investigate this today.

EDIT: Opps, 1.4.3 is what 15.2 has, so that's fine there. We don't need it for 1.4.2.

Like I said we can still backport fixes to 1.4.3 (assuming they cleanly apply), and if it helps we can tag them, but there will be no fedora/centos releases.

@mreynolds389 mreynolds389 added priority_high need urgent fix / highly valuable / easy to fix and removed needs triage The issue will be triaged during scrum labels Jun 30, 2021
Firstyear added a commit that referenced this issue Jul 9, 2021
…words (#4819)

Bug Description: Due to mishanding of short dbpwd hashes, the
crypt_r algorithm was misused and was only comparing salts
in some cases, rather than checking the actual content
of the password.

Fix Description: Stricter checks on dbpwd lengths to ensure
that content passed to crypt_r has at least 2 salt bytes and
1 hash byte, as well as stricter checks on ct_memcmp to ensure
that compared values are the same length, rather than potentially
allowing overruns/short comparisons.

fixes: #4817

Author: William Brown <william@blackhats.net.au>

Review by: @mreynolds389
Firstyear added a commit to Firstyear/389-ds-base that referenced this issue Jul 9, 2021
…words (389ds#4819)

Bug Description: Due to mishanding of short dbpwd hashes, the
crypt_r algorithm was misused and was only comparing salts
in some cases, rather than checking the actual content
of the password.

Fix Description: Stricter checks on dbpwd lengths to ensure
that content passed to crypt_r has at least 2 salt bytes and
1 hash byte, as well as stricter checks on ct_memcmp to ensure
that compared values are the same length, rather than potentially
allowing overruns/short comparisons.

fixes: 389ds#4817

Author: William Brown <william@blackhats.net.au>

Review by: @mreynolds389
Firstyear added a commit to Firstyear/389-ds-base that referenced this issue Jul 9, 2021
…words (389ds#4819)

Bug Description: Due to mishanding of short dbpwd hashes, the
crypt_r algorithm was misused and was only comparing salts
in some cases, rather than checking the actual content
of the password.

Fix Description: Stricter checks on dbpwd lengths to ensure
that content passed to crypt_r has at least 2 salt bytes and
1 hash byte, as well as stricter checks on ct_memcmp to ensure
that compared values are the same length, rather than potentially
allowing overruns/short comparisons.

fixes: 389ds#4817

Author: William Brown <william@blackhats.net.au>

Review by: @mreynolds389
Firstyear added a commit that referenced this issue Jul 9, 2021
…words (#4819)

Bug Description: Due to mishanding of short dbpwd hashes, the
crypt_r algorithm was misused and was only comparing salts
in some cases, rather than checking the actual content
of the password.

Fix Description: Stricter checks on dbpwd lengths to ensure
that content passed to crypt_r has at least 2 salt bytes and
1 hash byte, as well as stricter checks on ct_memcmp to ensure
that compared values are the same length, rather than potentially
allowing overruns/short comparisons.

fixes: #4817

Author: William Brown <william@blackhats.net.au>

Review by: @mreynolds389
@Firstyear
Copy link
Contributor

@rwinter77 I'll get these patches into leap in the coming days

@mreynolds389
Copy link
Contributor

Downstream bug for RHDS:

https://bugzilla.redhat.com/show_bug.cgi?id=1981833

mreynolds389 pushed a commit that referenced this issue Aug 12, 2021
…words (#4819)

Bug Description: Due to mishanding of short dbpwd hashes, the
crypt_r algorithm was misused and was only comparing salts
in some cases, rather than checking the actual content
of the password.

Fix Description: Stricter checks on dbpwd lengths to ensure
that content passed to crypt_r has at least 2 salt bytes and
1 hash byte, as well as stricter checks on ct_memcmp to ensure
that compared values are the same length, rather than potentially
allowing overruns/short comparisons.

fixes: #4817

Author: William Brown <william@blackhats.net.au>

Review by: @mreynolds389
@mossoctopus
Copy link

Hello,

I was looking into CVE-2021-3652, which seems to be the one associated with this issue, as well as looking at CVE-2017-15135. Checking out the patches for both CVEs, the fix that has been presented here, and the fix in https://bugzilla-attachments.redhat.com/attachment.cgi?id=1388078, they seem to be very similar, or at least we have a situation where the patch for CVE-2017-15135 also fixes this issue here. I was wondering if I could get more insight on what happened to the fix that addressed CVE-2017-15135 (it seems to not be included in the code here), and if it indeed fixed the same problem that is CVE-2021-3652. If they are equivalent fixes, CVE-2021-3652 might be a duplicate.

Firstyear added a commit to Firstyear/389-ds-base that referenced this issue Jun 8, 2022
…words (389ds#4819)

Bug Description: Due to mishanding of short dbpwd hashes, the
crypt_r algorithm was misused and was only comparing salts
in some cases, rather than checking the actual content
of the password.

Fix Description: Stricter checks on dbpwd lengths to ensure
that content passed to crypt_r has at least 2 salt bytes and
1 hash byte, as well as stricter checks on ct_memcmp to ensure
that compared values are the same length, rather than potentially
allowing overruns/short comparisons.

fixes: 389ds#4817

Author: William Brown <william@blackhats.net.au>

Review by: @mreynolds389
Firstyear added a commit to Firstyear/389-ds-base that referenced this issue Jun 8, 2022
…words (389ds#4819)

Bug Description: Due to mishanding of short dbpwd hashes, the
crypt_r algorithm was misused and was only comparing salts
in some cases, rather than checking the actual content
of the password.

Fix Description: Stricter checks on dbpwd lengths to ensure
that content passed to crypt_r has at least 2 salt bytes and
1 hash byte, as well as stricter checks on ct_memcmp to ensure
that compared values are the same length, rather than potentially
allowing overruns/short comparisons.

fixes: 389ds#4817

Author: William Brown <william@blackhats.net.au>

Review by: @mreynolds389
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority_high need urgent fix / highly valuable / easy to fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants