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

md5crypt-ztex, phpass-ztex #3420

Merged
merged 1 commit into from Oct 5, 2018
Merged

md5crypt-ztex, phpass-ztex #3420

merged 1 commit into from Oct 5, 2018

Conversation

Apingis
Copy link
Collaborator

@Apingis Apingis commented Oct 5, 2018

No description provided.

Copy link
Member

@solardiz solardiz left a comment

I made some minor comments. Also, please add info on these two new formats to README-ZTEX.

I'll merge this, then test. Any additional changes (such as to address my comments) should be in a separate PR. Thanks!

if (target_rounds < 512) {
fprintf(stderr, "Warning: invalid TargetRounds=%d in john.conf."
" Valid values are 1000-999999999\n", target_rounds);
target_rounds = 512;
Copy link
Member

@solardiz solardiz Oct 5, 2018

Choose a reason for hiding this comment

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

The message is inconsistent with the check here, and only powers of 2 may be used with phpass, so perhaps the message should say 512-999999999?

Copy link
Collaborator Author

@Apingis Apingis Oct 6, 2018

Choose a reason for hiding this comment

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

Ageed.

1536 * 1024, // Would be 48 MB of USB traffic on 32-byte keys
512, // Max. number of entries in onboard comparator.
384, // Min. number of keys for effective device utilization
1, { 135 }, // Programmable clocks
Copy link
Member

@solardiz solardiz Oct 5, 2018

Choose a reason for hiding this comment

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

Should probably use same default frequency as we have in the comment in john.conf, currently 180.

Copy link
Collaborator Author

@Apingis Apingis Oct 6, 2018

Choose a reason for hiding this comment

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

I'm unsure in a scenario where a user decides to set frequency below 180.
This number 135, hardcoded in struct device_bitstream is used when resetting the FPGA with Global Set Reset.
Then frequency from john.conf applies. If a user decides, lets say 160 or 170 then one can be sure the FPGA will never run at higher frequency.

Copy link
Member

@solardiz solardiz Oct 6, 2018

Choose a reason for hiding this comment

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

Oh, I thought the frequency from john.conf always overrides this one, and conversely this one is always used when there's no frequency specified in john.conf. Is this not the case? Can/should we make it the case? Any other behavior feels non-intuitive.

1536 * 1024, // Would be 48 MB of USB traffic on 32-byte keys
512, // Max. number of entries in onboard comparator.
384, // Min. number of keys for effective device utilization
1, { 135 }, // Programmable clocks
Copy link
Member

@solardiz solardiz Oct 5, 2018

Choose a reason for hiding this comment

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

Should probably use same default frequency as we have in the comment in john.conf, currently 180.

1,
FMT_CASE | FMT_8_BIT | FMT_MASK,
{
"iteration count",
Copy link
Member

@solardiz solardiz Oct 5, 2018

Choose a reason for hiding this comment

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

We shouldn't report any tunable costs for md5crypt.

Copy link
Collaborator Author

@Apingis Apingis Oct 6, 2018

Choose a reason for hiding this comment

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

I can do it and I'm afraid that would be time consuming; i've added that (I'd say "a hack") to simplify development.
The problem is: while a program for computing units' CPUs is selected at runtime (md5crypt or phpass), input to computing units (from the host via cmp_config and arbiter_tx) is common for both programs, including iteration count.

I can do one of the following:

  • introduce different salt types on the FPGA side in common bitstream: while phpass salt would pass-by the count from the host, md5crypt salt would add-up 1,000 in 4 bytes during processing;
  • don't modify bitstream, send 1,000 as iteration count from the host and somehow keep it invisible for a user, exclude iteration_count from struct fmt_main.

Please confirm if this modification is required.

For the future development, I have thoughts on some "I/O processor" that would handle input/output to/from the rest of the FPGA, perform basic preparations, simplify further jtr-fpga development by a replacement of custom I/O curcuits with CPU programs.

Copy link
Member

@solardiz solardiz Oct 6, 2018

Choose a reason for hiding this comment

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

I'm surprised you find fixing this time-consuming. Of course, I meant the second option: "don't modify bitstream, send 1,000 as iteration count from the host and somehow keep it invisible for a user, exclude iteration_count from struct fmt_main." I think it should be do-able in under an hour of effort, no?

Copy link
Member

@solardiz solardiz Oct 6, 2018

Choose a reason for hiding this comment

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

This hack seems to work:

diff --git a/src/ztex/pkt_comm/cmp_config.c b/src/ztex/pkt_comm/cmp_config.c
index dda95ab..be2c1e9 100644
--- a/src/ztex/pkt_comm/cmp_config.c
+++ b/src/ztex/pkt_comm/cmp_config.c
@@ -102,7 +102,8 @@ void cmp_config_new(struct db_salt *salt, void *salt_ptr, int salt_len)
 
        int cost_num;
        for (cost_num = 0; cost_num < FMT_TUNABLE_COSTS; cost_num ++) {
-               if (jtr_fmt_params->tunable_cost_name[cost_num])
+               if (jtr_fmt_params->tunable_cost_name[cost_num] ||
+                   (cost_num == 0 && salt_len == 18) /* md5crypt hack */)
                        cmp_config.tunable_costs[cost_num] = salt->cost[cost_num];
                else
                        cmp_config.tunable_costs[cost_num] = 0;
diff --git a/src/ztex_md5crypt.c b/src/ztex_md5crypt.c
index 53a0017..ce806cd 100644
--- a/src/ztex_md5crypt.c
+++ b/src/ztex_md5crypt.c
@@ -140,6 +140,8 @@ static int crypt_all(int *pcount, struct db_salt *salt)
        salt_buf[1] = salt_len;
        memcpy(salt_buf + 2, salt->salt, salt_len);
 
+       salt->cost[0] = 1000;
+
        cmp_config_new(salt, salt_buf, 18);
 
        result = device_format_crypt_all(pcount, salt);
@@ -174,13 +176,6 @@ static int cmp_exact(char *source, int index)
        return !memcmp(result->binary, get_binary(source), 16);
 }
 
-
-static unsigned int iteration_count(void *salt)
-{
-       return 1000;
-}
-
-
 struct fmt_main fmt_ztex_md5crypt = {
        {
                FORMAT_LABEL,
@@ -197,9 +192,7 @@ struct fmt_main fmt_ztex_md5crypt = {
                1, // set by device_format_reset()
                1,
                FMT_CASE | FMT_8_BIT | FMT_MASK,
-               {
-                       "iteration count",
-               },
+               { NULL },
                {
                        FORMAT_TAG
                },
@@ -213,9 +206,7 @@ struct fmt_main fmt_ztex_md5crypt = {
                fmt_default_split,
                get_binary,
                get_salt,
-               {
-                       iteration_count,
-               },
+               { NULL },
                fmt_default_source,
                {
                        fmt_default_binary_hash_0,

Copy link
Member

@solardiz solardiz Oct 6, 2018

Choose a reason for hiding this comment

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

Maybe it'd be less of a hack to start supporting "nameless" tunable costs, which would thus be invisible to the user, but would work for this code anyway. Perhaps this will amount to removal of the name (set it to NULL like above), but not the function, and checking tunable_cost_value (function pointers) instead of tunable_cost_name in cmp_config_new.

Copy link
Collaborator Author

@Apingis Apingis Oct 6, 2018

Choose a reason for hiding this comment

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

I'm kind of surprised that you look into the code so in-depth.
I've got other solution that would not modify shared code for ztex formats,
limited to ztex_md5crypt.c source.



module thread_number #(
parameter N_CORES = -1,
Copy link
Member

@solardiz solardiz Oct 5, 2018

Choose a reason for hiding this comment

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

What is meant by N_CORES = -1? Maybe add a comment?

Copy link
Collaborator Author

@Apingis Apingis Oct 6, 2018

Choose a reason for hiding this comment

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

N_CORES = -1 would not pass synthesis (would raise an error where N_CORES actually used and expected to be a positive or non-negative number). I see/use such construct as a measure to enforce the requirement to pass valid N_CORES from upper-level module.


module cpu_flags #(
parameter N = `N_FLAGS,
parameter N_THREADS = 16,
Copy link
Member

@solardiz solardiz Oct 5, 2018

Choose a reason for hiding this comment

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

Should it possibly be 12 rather than 16 in N_THREADS = 16? This is probably harmless, and not worth fixing on its own.


- md5crypt ($1$) for ZTEX 1.15y board allows candidate passwords up to
64 bytes, equipped with on-board candidate generator and comparator.
It's also able to compute phpass-MD5 hashes.
Copy link
Member

@solardiz solardiz Oct 5, 2018

Choose a reason for hiding this comment

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

It'd be good to clarify whether the same limit of 64 characters applies to phpass as well.

Copy link
Collaborator Author

@Apingis Apingis Oct 6, 2018

Choose a reason for hiding this comment

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

Candidates of lengths up to and including 64 bytes can be passed to phpass program, there's enough space in the memory, handled w/o problems. However I decided to keep phpass(-cpu) limit on the length because I don't have example hashes with plaintexts up to & including 64 bytes long. I can set PLAINTEXT_LENGTH equal to 64 for phpass, that would perform but I can't verify correctness.
Could you provide example hashes (from plaintexts <= 64 bytes)? (could not find any on the Internet). Alternatively I can set-up the software/environment where such hashes are used, produce hashes, unfortunately I'm running out of the timeline previously agreed for (md5crypt|phpass)-ztex development.

Copy link
Member

@solardiz solardiz Oct 6, 2018

Choose a reason for hiding this comment

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

Oh, looking at the code I see that the limit of 39 is getting inherited from phpass_common.h. This isn't a terribly low limit, but 64 would be nicer - and we need to document the actual limit either way. Yes, I'll generate and provide a test hash for a 64 chars string. Thanks!

Copy link
Member

@solardiz solardiz Oct 6, 2018

Choose a reason for hiding this comment

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

test012345678901234567890123456789012345678901234567890123456789 $P$9IQRaTwmfexDe7M./J5j8/buI4F3sX0

@solardiz
Copy link
Member

solardiz commented Oct 5, 2018

@magnumripper This and another pull request appear to be stuck on travis-ci for 10 hours. Is our travis-ci integration possibly currently broken, or should we continue waiting?

@magnumripper
Copy link
Member

magnumripper commented Oct 5, 2018

I just saw that and fixed it. Merging now.

@magnumripper magnumripper merged commit f9bc00a into openwall:bleeding-jumbo Oct 5, 2018
2 checks passed

[ZTEX:phpass]
Frequency = 180
#TargetRounds = 2048
Copy link
Member

@solardiz solardiz Oct 6, 2018

Choose a reason for hiding this comment

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

Maybe the default TargetRounds for phpass should be 8192: it appears that over 50% of phpass hashes that people want to crack are $P$B, presumably due to this being the default for PHP 5+ in my upstream phpass code ($P$9 aka 2048 is only the default for PHP below 5), as well as the default in WordPress. I have yet to test the effect of cracking $P$B hashes with TargetRounds = 2048, and $P$9 with 8192 - which of these is impacted worse - and take this into consideration as well.

Copy link
Member

@solardiz solardiz Oct 6, 2018

Choose a reason for hiding this comment

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

I just did some testing with wrong TargetRounds. The performance impact of running with 8192 against 2048 rounds hashes is about 2%. Running with 2048 against 8192 rounds hashes results in timeouts - so that's what most(?) users will experience, and due to the warning will hopefully adjust the setting. Running with 4096 results in the performance impact for 2048 rounds hashes reduced to 1%, and the timeouts for 8192 rounds hashes are mostly gone - but not entirely gone. Also, I guess this might interfere with --test benchmark, where we use 2048 rounds hashes. So I guess let's leave things as-is for now.

Copy link
Collaborator Author

@Apingis Apingis Oct 6, 2018

Choose a reason for hiding this comment

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

OK. Performance depends on host system, its CPU and if it's busy with anything at moment of testing. USB communication is CPU-intensive.
What I think is worth consideration: extend no-warning range. Let's say there should be no warning if hashes are 2 times heavier than config's TargetRounds, and if hashes are 4-8 times weaker than config's TargetRounds (since there's not much performance loss anyway).
Currently it warns if hashes are different by more than 2X from TargetRounds in either direction.

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.

None yet

3 participants