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

Add SM3 #5495

Merged
merged 1 commit into from
Jun 29, 2024
Merged

Add SM3 #5495

merged 1 commit into from
Jun 29, 2024

Conversation

SamuraiOcto
Copy link
Contributor

This adds the SM3 hash function that is fairly widespread, mainly in China, but which was not yet present in john. For now this is just a basic but slow-ish implementation. Perhaps I might add avx2 later. Let me know if things should be changed.

@claudioandre-br
Copy link
Member

Thank you for your contribution! Your PR has passed all tests, including 32-bit, ARM, ASAN, no-OMP, ...

We will give feedback soon.

@solardiz
Copy link
Member

Thank you for the contribution @SamuraiOcto!

The added sm3* files lack copyright and license statements. Can you please add those, preferably using our common 0-clause BSD license:

 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted.
 *
 * There's ABSOLUTELY NO WARRANTY, express or implied.

although where you use third-party SM3 code, you need to use their original license (or maybe public domain statement).

Also, at least as seen in GitHub, these files appear to use 4-space indentation? We normally use tabs, and assume tab width 8.

Finally, are the current settings for MAX_KEYS_PER_CRYPT and OMP_SCALE arbitrary (e.g., inherited from another file we had) or did you re-tune (--tune=auto -v=5)? There's a likely stale comment about tuning on "Core i7" (which always looked too vague to me, but another contributor put this comment in, and it propagated between files) - please either update or drop it.

When you revise this PR, please amend the one commit and force-push. Please do not add more commits.

@solardiz
Copy link
Member

Oh, please also add an entry to doc/NEWS, near the end of the section for post-1.9.0 changes, so currently on line 329, and maintain a two empty line gap before the next section.

@SamuraiOcto
Copy link
Contributor Author

  • The indentation should be fixed. Used the indent/astyle commands from your CONTRIBUTING.md.
  • Most of the SM3 code is taken from github.com/guanzhi/GmSSL so their Apache license is added where applicable. The format plugin file has the 0-clause BSD license.
  • OMP_SCALE was indeed copied from another file. I re-tuned with --tune=auto -v=5 which gives 4 for me, but won't the outcome depend on the number of CPU cores?
  • Honestly I'm not sure about MAX_KEYS_PER_CRYPT, it was indeed copied from another file.
  • Added a NEWS entry.

@solardiz
Copy link
Member

Thanks, this looks almost ready to merge.

The format plugin file has the 0-clause BSD license.

Great. You also need to add your copyright statement to there, if you wrote that file.

OMP_SCALE was indeed copied from another file. I re-tuned with --tune=auto -v=5 which gives 4 for me, but won't the outcome depend on the number of CPU cores?

It is per-thread, so we multiply it by the actual number of threads.

Honestly I'm not sure about MAX_KEYS_PER_CRYPT, it was indeed copied from another file.

Normally, it's to be tuned manually at 1 thread or in a non-OpenMP build, then once you have it at optimal value for that, you tune OMP_SCALE for multiple threads. That said, the value of 32 looks sane for a fast hash.

@solardiz
Copy link
Member

I think I'll just merge this and then add a copyright line using your GitHub username on it...

@solardiz solardiz merged commit a87862b into openwall:bleeding-jumbo Jun 29, 2024
31 of 32 checks passed
@solardiz
Copy link
Member

OK, merged and I'm testing and preparing further changes. Added copyright statement:

+++ b/src/sm3_fmt_plug.c
@@ -1,9 +1,12 @@
-/*  SM3 format plugin
+/*
+ * SM3 format plugin
  *
- *  Redistribution and use in source and binary forms, with or without
- *  modification, are permitted.
+ * Copyright (c) 2024 SamuraiOcto
  *
- *  There's ABSOLUTELY NO WARRANTY, express or implied.
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted.
+ *
+ * There's ABSOLUTELY NO WARRANTY, express or implied.
  */
 
 #if FMT_EXTERNS_H

Somehow performance with OpenMP is awful - way lower than at 1 thread. Running with -tune=auto gets me a many orders of magnitude higher OMP_SCALE than the 4 put in the code, and then performance becomes sane. But I'll look into tuning MAX_KEYS_PER_CRYPT first.

Also confirmed that the auto-generated dynamic_big_crypt_generated.c matches what I'm getting (sans the timestamp).

Reviewing the code, I see it somehow limits hash comparisons to 16 bytes - why is that? Perhaps copied from another format, and is a bug here?

@solardiz
Copy link
Member

OK, I've made several commits on top of yours @SamuraiOcto and pushed them into bleeding-jumbo.

You're adding several dynamic preload formats. Can you please document those in doc/DYNAMIC?

@solardiz
Copy link
Member

solardiz commented Jul 6, 2024

You're adding several dynamic preload formats. Can you please document those in doc/DYNAMIC?

@SamuraiOcto Reminding you that this is still missing the above documentation, which I'd appreciate you adding via a separate PR. Thanks!

@SamuraiOcto
Copy link
Contributor Author

Thanks for merging this and sorry for the delay. #5512 adds the missing documentation

@SamuraiOcto SamuraiOcto deleted the sm3 branch July 11, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants