--regen-lost-salts NULL pointer dereference #4517
Comments
|
I tried to follow Jim's code for regen lost salts. The canonical way to do it would be to synthesize all salts at load time, resulting in a perfectly normal database, identical to what we'd get if we had simply generated an input file with all salts for each hash instead of relying on this feature. For this test case, it would say "20 password hashes with 10 different salts (2.0x same-salt boost)". From that point, nothing much could go wrong at all (the only magic would be the pot file matching code). Alas, this doesn't quite seem to be the case with the current code: For this test case, john reports "2 password hashes with no different salts". Yet there are 10 salts in the database, and I tried adding corrections for Maybe the problem is the current code re-uses the password list for all salts (copying Oh and BTW if you try --regen-lost-salts with invalid parameters, it bails out refering to |
This copying does make sense in that if a hash gets cracked, it's removed from all salts. So if we loaded two hashes in this case it'd start saying "Loaded 20 password hashes with 10 different salts" but after just one of them is cracked there will be "Remaining 10 password hashes with 10 different salts". On a side note we could change those messages when this feature is active, so they'd say something like "Loaded 2 password hashes with 10 possible salts each" and "Remaining 1 password hash with 10 possible salts". |
Since every salt copied the same salt->list pointer, a special fixup is needed after removing *the first* binary in the list in crk_process_guess(). The generated salts written to the salt database lacked some info and the total number of salts wasn't updated. Error messages pointed to a non-existing file in a non-existing directory. Change the "Remaining xx hashes with yy different salts" output to be more accurate when this feature is used.
Since every salt copied the same salt->list pointer, a special fixup is needed after removing *the first* binary in the list in crk_process_guess(). The generated salts written to the salt database lacked some info and the total number of salts wasn't updated. Error messages pointed to a non-existing file in a non-existing directory. Change the "Remaining xx hashes with yy different salts" output to be more accurate when this feature is used.
|
I think I nailed it. PR in #4795. That pointer-copying was actually a good idea, but a fixup was needed for the case where the first binary in the list was removed by a successful guess. This bug must have been present since day 1 of the feature. |
|
Here's the new Loaded/Remaining output (running with A correct salt-boost figure is shown, and the wording (IMHO) is better for this special feature: We only have 2 hashes, not 20, but each of them have 10 possible salts. |
Since every salt copied the same salt->list pointer, a special fixup is needed after removing *the first* binary in the list in crk_process_guess(). The generated salts written to the salt database lacked some info and the total number of salts wasn't updated. Error messages pointed to a non-existing file in a non-existing directory. Change the "Remaining xx hashes with yy different salts" output to be more accurate when this feature is used.
Since every salt has a copy of the initial salt->list pointer, a special fixup is needed after removing the first binary in the list in crk_process_guess(). This was the actual openwall#4517 issue. Other fixes: The generated salts written to the salt database lacked some info and the total number of salts wasn't updated. From now we copy the whole initial database struct, avoiding several current and at least some possible future problems. Improved the "Remaining xx hashes with yy different salts" output to be more accurate/informative when this feature is used. Error messages referred to a non-existing file in a non-existing directory. Several comments were bogus, eg. referring to code that was long gone. General code-style fixes (including a couple of barely related, made while trying to understand the code paths involved). Add bodges for --show to work with hashes cracked using this feature. For this to work you need to supply the --regen-lost-salts option with --show too. Since we unfortunately give the exact format as one of the parameters to --regen-lost-salts (a fairly stupid decision made 8 years ago), add code to automatically pick that format wheneven a --format parameter isn't given, or complain if it is and they don't match. Minor updates to doc/Regen-Lost-Salts, eg. dropping references to things that were deprecated eight years ago.
|
Since every salt has a copy of the initial salt->list pointer, a special fixup is needed after removing the first binary in the list in crk_process_guess(). This was the actual openwall#4517 issue. Other fixes: The generated salts written to the salt database lacked some info and the total number of salts wasn't updated. From now we copy the whole initial database struct, avoiding several current and at least some possible future problems. Improved the "Remaining xx hashes with yy different salts" output to be more accurate/informative when this feature is used. Error messages referred to a non-existing file in a non-existing directory. Several comments were bogus, eg. referring to code that was long gone. General code-style fixes (including a couple of barely related, made while trying to understand the code paths involved). Add bodges for --show to work with hashes cracked using this feature. For this to work you need to supply the --regen-lost-salts option with --show too. Since we unfortunately give the exact format as one of the parameters to --regen-lost-salts (a fairly stupid decision made 8 years ago), add code to automatically pick that format wheneven a --format parameter isn't given, or complain if it is and they don't match. Minor updates to doc/Regen-Lost-Salts, eg. dropping references to things that were deprecated eight years ago.
Since every salt has a copy of the initial salt->list pointer, a special fixup is needed after removing the first binary in the list in crk_process_guess(). This was the actual openwall#4517 issue. Other fixes: The generated salts written to the salt database lacked some info and the total number of salts wasn't updated. From now we copy the whole initial database struct, avoiding several current and at least some possible future problems. Improved the "Remaining xx hashes with yy different salts" output to be more accurate/informative when this feature is used - correctly showing same-salt boost or salt BF penalty. Error messages referred to a non-existing file in a non-existing directory. Several comments were bogus, eg. referring to code that was long gone. General code-style fixes (including a couple of barely related, made while trying to understand the code paths involved). Add bodges for --show to work with hashes cracked using this feature. For this to work you need to supply the --regen-lost-salts option with --show too. Since we unfortunately give the exact format as one of the parameters to --regen-lost-salts (a fairly stupid decision made 8 years ago), add code to automatically infer that format whenever a --format parameter isn't given, or complain if it is and they don't match. Minor updates to doc/Regen-Lost-Salts, eg. dropping references to things that were deprecated eight years ago.
As reported by @kernelr00ter in #1463 (comment):
Reproduced by me:
The text was updated successfully, but these errors were encountered: