Skip to content

Several formats' prepare(): Fix bugs when checking fields #5776

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

Open
wants to merge 2 commits into
base: bleeding-jumbo
Choose a base branch
from

Conversation

magnumripper
Copy link
Member

fields[n] is never NULL, and the string fields[0] points to is only a nullstring if there is an explicit field saying so (i.e. line starts with a ":")

For a missing login field (no ":", bare hash) fields[0] is set to "?" in loader.

This bug made some formats make up uncrackable hashes from thin air instead of rejecting when salt is unknown - leading to false negatives. Those formats are in a separate commit.

This was tested manually and with Test Suite.

…ogin

fields[0] is never NULL, and the string it points to is only a nullstring
if there is an explicit field saying so (i.e. line starts with a ":")

For a missing login field (no ":", bare hash) field[0] is set to "?" in
loader.

This bug made the formats make up uncrackable hashes instead of rejecting
when salt is unknown - leading to false negatives.
fields[n] is never NULL so no test for that needed. The intended checks
were otherwise typically to check that a field wasn't an empty string.
Copy link
Member

@solardiz solardiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checks for ? are not ideal - they'd also be triggered on literal username ?. OTOH, such a username would also be confusing in our output.

Anyway, I agree these changes are what we need right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants