Solve issue Update hashcat mode with new rules #5905

[Serax4]

Added the options requested
#5905
I did not add option S because we had it already and I wasn't sure about option v so i skipped it as well.

[magnumripper]

Thank you! I tested each mnemonic and they work fine. However, it seems to me you messed up the indentation. In particular, the old code for '1' and '2' now has an additional tab of indentation. You shouldn't touch existing code. Also, your case lines (only that one line for each case) for 'H' and 'B' have one tab too much.

You would be able to implement the 'v' rule if you like (would be much appreciated) if you use the hc_logic variable (see the code for 'p' rule, and doc/RULES-hashcat).

[magnumripper]

You can't touch old/unrelated code (some formatting was still changed). I know the old code doesn't comply with current coding standards so what you did was right in a way but things like that should be in some other commit and PR.

Worse, case 'v' doesn't even build. It clashes with the existing one. Could you build and run it?

rules.c: In function 'rules_apply':
rules.c:1479:17: error: duplicate case value
 1479 |                 case 'v':
      |                 ^~~~
rules.c:1426:17: note: previously used here
 1426 |                 case 'v': /* assign value to numeric variable */
      |                 ^~~~
make[1]: *** [rules.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [default] Error 2

To make it work, this new code needs to be inside the existing 'v' case (with if (hc_logic) .. else). See case 'p' for an example.

[Serax4]

@magnumripper Sorry this is the first time i work with a public repository I will try to be more caferul and follow the coding guidelines for the next issues (FYI the other issue I am working on is #5817).
I forgot to replace the v case with the new one that's why it didn't compile.
I did implement the if (hc_logic)... else but i see in case 'p':
if (hc_logic || (*rule >= '1' && *rule <= '9')) {
I don't really understand the OR statement here, is the idea here that if the user forgets to enable hc_logic but provides the additional arguments (required for Hashcat mode but not normally) then the case should still be interpreted in hc_logic? Case v (hc_logic) is in the format vNX. Should i check if a number (N position) and a character (X) are passed?
I just want to make sure that I understood, you said ' what you did was right in a way but things like that should be in some other commit and PR' do you want me to delete this pull request, and make a new one with a nice and tidy commit?

[magnumripper]

Sorry this is the first time i work with a public repository I will try to be more caferul and follow the coding guidelines for the next issues

No problem, you'll get the hang of it. Thanks for contributing!

I forgot to replace the v case with the new one that's why it didn't compile. I did implement the if (hc_logic)... else but i see in case 'p': if (hc_logic || (*rule >= '1' && *rule <= '9')) { I don't really understand the OR statement here, is the idea here that if the user forgets to enable hc_logic but provides the additional arguments (required for Hashcat mode but not normally) then the case should still be interpreted in hc_logic?

Almost: In the 'pN' case the non-hashcat 'p' is only possible in certain contexts so in other situations we assume the hashcat 'p' even without being in hashcat mode. That hack is not applicable to 'v' so you only need if (hc_logic)... else. I'm sorry for giving a poor example, maybe 'x' is a better one.

Case v (hc_logic) is in the format vNX. Should i check if a number (N position) and a character (X) are passed?

You should, and this is similar to 'o' I think. The POSITION macro ensures it's a number while VALUE can be a character. Those macros can be confusing and are poorly documented, I always have a hard time writing new rules.

I just want to make sure that I understood, you said ' what you did was right in a way but things like that should be in some other commit and PR' do you want me to delete this pull request, and make a new one with a nice and tidy commit?

No need for a new PR, just revert the modifications of the unrelated code and force push to this one. If/when we want to reformat the unrelated code it will be a completely separate issue & PR.

Thanks!
magnum

[magnumripper]

Case v (hc_logic) is in the format vNX. Should i check if a number (N position) and a character (X) are passed?

You should, and this is similar to 'o' I think. The POSITION macro ensures it's a number while VALUE can be a character. Those macros can be confusing and are poorly documented, I always have a hard time writing new rules.

To clarify, in this regard your current version of 'v' looks correct already. You do use the POSITION and VALUE macros correctly as far as I understand and then you ensure the position isn't 0. The for takes care of if (pos < length) so the latter is not needed.

[Serax4]

I think it should work now. It was very satisfying to see only green in the commit XD

[magnumripper]

The first line here (and same for 'h') aborts without converting a single letter to hex. I'm not saying it's wrong (we don't have a spec that detailed) and I don't care - I think RULE_WORD_SIZE is pretty large. Just mentioning it.

[Serax4]

I also don't think it's a problem, if it's acceptable now should it be merged?