Skip to content

Commit

Permalink
Fix access violation in hashconfig_destroy if hashcat_ctx_t is only p…
Browse files Browse the repository at this point in the history 
…artially initialized.

Fix hashcat_ctx leak and refactor module and kernel existence checks.
  • Loading branch information
@jtojanen
jtojanen committed Jun 21, 2021
1 parent 2c48bba commit 6967e70
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 46 deletions.
21 changes: 11 additions & 10 deletions src/interface.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -495,23 +495,24 @@ void hashconfig_destroy (hashcat_ctx_t *hashcat_ctx)
}
}

if (hashconfig->hook_extra_param_size)
if (module_ctx->hook_extra_params)
{
const int hook_threads = (int) user_options->hook_threads;
if (hashconfig->hook_extra_param_size)
{
const int hook_threads = (int) user_options->hook_threads;

for (int i = 0; i < hook_threads; i++)
for (int i = 0; i < hook_threads; i++)
{
hcfree (module_ctx->hook_extra_params[i]);
}
}
else
{
hcfree (module_ctx->hook_extra_params[i]);
hcfree (module_ctx->hook_extra_params[0]);
}

hcfree (module_ctx->hook_extra_params);
}
else
{
hcfree (module_ctx->hook_extra_params[0]);

hcfree (module_ctx->hook_extra_params);
}

module_unload (module_ctx);

Expand Down
67 changes: 31 additions & 36 deletions src/user_options.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2845,27 +2845,6 @@ int user_options_check_files (hashcat_ctx_t *hashcat_ctx)
}
}

// single kernel and module existence check to detect "7z e" errors

char *modulefile = (char *) hcmalloc (HCBUFSIZ_TINY);

module_filename (folder_config, 0, modulefile, HCBUFSIZ_TINY);

if (hc_path_exist (modulefile) == false)
{
event_log_error (hashcat_ctx, "%s: %s", modulefile, strerror (errno));

event_log_warning (hashcat_ctx, "If you are using the hashcat binary package, this may be an extraction issue.");
event_log_warning (hashcat_ctx, "For example, using \"7z e\" instead of using \"7z x\".");
event_log_warning (hashcat_ctx, NULL);

hcfree (modulefile);

return -1;
}

hcfree (modulefile);

const bool quiet_save = user_options->quiet;

user_options->quiet = true;
Expand All @@ -2874,31 +2853,47 @@ int user_options_check_files (hashcat_ctx_t *hashcat_ctx)

user_options->quiet = quiet_save;

if (rc == -1) return -1;
if (rc == -1)
{
// module existence check to detect "7z e" errors

hashconfig_destroy (hashcat_ctx);
const module_ctx_t* module_ctx = hashcat_ctx->module_ctx;

// same check but for an backend kernel
if (module_ctx->module_handle == NULL)
{
event_log_warning (hashcat_ctx, "If you are using the hashcat binary package, this may be an extraction issue.");
event_log_warning (hashcat_ctx, "For example, using \"7z e\" instead of using \"7z x\".");
event_log_warning (hashcat_ctx, NULL);
}

hashconfig_destroy (hashcat_ctx);

char *kernelfile = (char *) hcmalloc (HCBUFSIZ_TINY);
return -1;
}
else
{
// same check but for an backend kernel

generate_source_kernel_filename (false, ATTACK_EXEC_OUTSIDE_KERNEL, ATTACK_KERN_STRAIGHT, 400, 0, folder_config->shared_dir, kernelfile);
const hashconfig_t* hashconfig = hashcat_ctx->hashconfig;

if (hc_path_read (kernelfile) == false)
{
event_log_error (hashcat_ctx, "%s: %s", kernelfile, strerror (errno));
char kernelfile[HCBUFSIZ_TINY] = { 0 };

event_log_warning (hashcat_ctx, "If you are using the hashcat binary package, this may be an extraction issue.");
event_log_warning (hashcat_ctx, "For example, using \"7z e\" instead of using \"7z x\".");
event_log_warning (hashcat_ctx, NULL);
generate_source_kernel_filename (user_options->slow_candidates, hashconfig->attack_exec, user_options_extra->attack_kern, hashconfig->kern_type, hashconfig->opti_type & OPTI_TYPE_OPTIMIZED_KERNEL, folder_config->shared_dir, kernelfile);

hcfree (kernelfile);
hashconfig_destroy (hashcat_ctx);

return -1;
}
if (hc_path_read (kernelfile) == false)
{
event_log_error (hashcat_ctx, "%s: %s", kernelfile, strerror(errno));

hcfree (kernelfile);
event_log_warning (hashcat_ctx, "If you are using the hashcat binary package, this may be an extraction issue.");
event_log_warning (hashcat_ctx, "For example, using \"7z e\" instead of using \"7z x\".");
event_log_warning (hashcat_ctx, NULL);

return -1;
}
}

// loopback - can't check at this point

// tuning file check already done
Expand Down

4 comments on commit 6967e70

@jsteube
Copy link
Member

Choose a reason for hiding this comment

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

@jtojanen I think I did not check this PR with enough care. The changes done to user_options.c break --hash-info. See #2859 as a reference.

The fixed values in module_filename () and especially generate_source_kernel_filename () were done on purpose. I think I need to revert the changes in user_options.c, but before I do I want to clarify that the only reason why you changed it was that you can do the test for the -m mode that the user actually selected. Is that right? If yes, that's not what we want to do, because we only want to test for this particular case in which the user did not unpack hashcat correctly. For that purpose using a fixed value fits perfectly.

@jtojanen
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsteube I did study the detection algorithm, but it seems that I didn't test all cases. I do understand the reasoning behind fixed values, although 0 and 400 seemed like random magic numbers.

As we already call "hashconfig_init" in user_options.c, which loads and initializes the needed module, tries to read both pure and optimized kernel and sets flags "hashconfig->has_pure_kernel" and "hashconfig->has_optimized_kernel", this is all we need. I only forgot to remove test "if (user_options->hash_info == false)" in interface.c as that blocked the final decision making between pure and optimized kernels. I have committed a fix containing this.

Actually now we can also simplify logics in user_options.c to use "hashconfig->has_pure_kernel == false && hashconfig->has_optimized_kernel == false" instead of retesting kernel in lines 2875 - 2885.

@jsteube
Copy link
Member

@jsteube jsteube commented on 6967e70 Jul 1, 2021

Choose a reason for hiding this comment

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

I'm afraid that's not entirely correct. The original code called hashconfig_init() but the idea is to use a fixed -m value for that, too. That saves us to check if the user-specified -m value is a valid one. But since the -m value will be hardcoded to zero, we do not know about flags "hashconfig->has_pure_kernel" and "hashconfig->has_optimized_kernel", because they will be related to -m 0, not the one the user specified. That causes problem in --hash-info. All this is not necessary if we switch back to the original code base. The question is (and I hope you can answer that) what is it that we lose and can we redo it afterwards in a different way?

@jsteube
Copy link
Member

@jsteube jsteube commented on 6967e70 Jul 2, 2021 •
edited

Choose a reason for hiding this comment

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

I have partially reverted this PR (src/user_options.c part) in order to fix #2859. We can redo what's lost after the changes in #2858 are split and described in details, which should also cover the reverted sections from this PR.

Please sign in to comment.