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

Update ldap.py for parse_result_attributes #471

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

termanix
Copy link
Contributor

@termanix termanix commented Oct 21, 2024

This PR was opened in ldap.py to handle all parse_result_attributes operations, and PR 441 was opened to avoid overlapping some changes. Relevant changes will occur slowly and with frequent checks.

@termanix termanix marked this pull request as draft October 22, 2024 09:33
@termanix
Copy link
Contributor Author

Only gmsa left. Can't test it yet. It can be review for now.

@termanix termanix marked this pull request as ready for review October 28, 2024 19:59
@termanix
Copy link
Contributor Author

The errors I've encountered on ldap_results.py.

Important Note: Same error in ldap_results.py in the current main iirc and #441 ldap_results.py . This bug was fixed in #381. After merging #441 #381, this PR should be checke and review with the new ldap_results.py.

  1. check-if-admin (not only for this, also o other flags.), got utf-8 byte error.

    "This fix addresses UnicodeDecodeError encountered while parsing LDAP attribute values that contain non-UTF-8 compatible byte sequences. The code first attempts to decode each attribute value as UTF-8, and if it fails (triggering UnicodeDecodeError or AttributeError), it falls back to hexadecimal encoding for unsupported byte sequences. This approach improves reliability and flexibility when handling mixed or non-UTF-8 data in LDAP responses."

image

After fix ldap_results.py, other bug-fixes on ldap.py

  1. check-if-admin.
    After parsing (with newest ldap_results.py), An error was occurring due to SID conversion.

    "This fix resolves decoding issues encountered when processing the objectSid attribute in LDAP parsing. By encoding the SID data as Latin-1 (ISO-8859-1), we handle the byte-to-character conversion without altering byte values. This works because Latin-1 is a single-byte encoding that maps byte values directly to corresponding code points, preserving the original byte structure of non-UTF-8 data. Using latin1 ensures compatibility with the self.sid_to_str function, which requires a byte-accurate input, making it suitable for decoding SIDs that contain non-UTF-8-compatible sequences."

[Before fix]. Top SID value was coming from self.sid_to_str (which is wrong), The second one comes from the currently running code.

image

[Fixed code]
image

  1. query
    After parsing (with newest ldap_results.py), some values come with non-display format.

[Before fix]. To display non-printable characters, such as ï3o®w{\F¥òLßö, in a readable format, need to convert these bytes to their hexadecimal representation (latin1). This approach preserves the data while avoiding unreadable symbols.

image

[Fixed code]
image

  1. admin-count
    After parsing (with newest ldap_results.py), The new output is not same with old one.

[Before Fix] All admin accounts and groups are displaying with the currently running code. But After parsing it does display only accounts.

image

[After Parsing]
image

[After Fix] In the old code, if there was no attributes value, an empty value was assigned and append was performed. Relevant sections for this were added to the new code. This correction was then made to all similar flags 26391a3.

sAMAccountName = item.get("sAMAccountName", "")
mustCommit = sAMAccountName is not None
userAccountControl = f"0x{int(item.get('userAccountControl', 0)):x}"
memberOf = str(item.get("memberOf", " "))
pwdLastSet = "<never>" if str(item.get("pwdLastSet", 0)) == "0" else str(datetime.fromtimestamp(self.getUnixTime(int(str(item.get("pwdLastSet", 0))))))
lastLogon = "<never>" if str(item.get("lastLogon", 0)) == "0" else str(datetime.fromtimestamp(self.getUnixTime(int(str(item.get("lastLogon", 0))))))

image

@NeffIsBack NeffIsBack added the enhancement New feature or request label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants