An implementation of the Chinese SM3 hash algorithm #4616

ronaldtse · 2017-10-30T04:16:22Z

This PR introduces the Chinese SM3 hash algorithm, which produces a 256-bit digest.

This was ported from Ribose's contribution to Botan, and contains some minor changes to make its performance on par with Botan's SM3 implementation.

Currently openssl speed -evp sm3 reports:

~270 MB/s on @randombit 's machine
~250 MB/s on @ronaldtse 's macOS Macbook Pro 3.1 GHz
This is a contribution by Ribose Inc. and was done with knowledge of @richsalz, @levitte, @InfoHunter.

Questions:

Currently there is a doc/man3/SM3.pod, but the non-EVP interface is discouraged. Should it be moved to doc/man3/EVP_sm3.pod (and hence the breakup of the digests pod files)?
Most hash algorithms don't seem to have a test/recipe/ except HMAC. Do we need one?

cc: @randombit @dewyatt @erikbor

This is a contribution from Ribose Inc..

Checklist

documentation is added or updated
tests are added or updated

paulidale · 2017-10-30T04:31:19Z

For the first question, I suspect the consensus will be that the low level calls SM3_Init, SM3_Update, SM3_Final and SM3 shouldn't be exposed. This means no need for the SM3.pod file, the sm3.h should be internal and the functions renamed sm3_init, sm3_update and sm3_final and the SM3 function removed.

For the second question, there isn't a need for a standalone test for SM3. The tests in evpdigest.txt should cover it properly (& look like they do).

levitte · 2017-10-30T06:34:48Z

crypto/evp/m_sm3.c

@@ -0,0 +1,55 @@
+/*


This file could be placed in crypto/sm3 just as well. As a matter of fact, I'd like to encourage that. Have a look at crypto/blake2/ for inspiration.

levitte · 2017-10-30T06:40:02Z

I agree with @paulidale, making an EVP only interface has been the move for some time now.

levitte · 2017-10-30T06:42:40Z

Regarding lowercasing the function names, I'm not sure that's an absolute necessity. We haven't been picky about that before (see SM4... or Blake2, for that matter)...

paulidale · 2017-10-30T08:02:33Z

I'm for lower casing them moving forwards. Upper case lead ins to names are generally exported.

We possibly should rework the non-exported upper case lead in names to be lowercase. I might get some time for this later in the week.

I've also a scheme to move to an EVP only interface -- this means replacing the historic direct calls with wrappers that use the EVP interface. It also renaming internal uses of these functions and (perhaps) the structures that are used. I.e. somewhat invasive :(

paulidale · 2017-10-30T08:08:31Z

Out of interest, are there going to be submissions for SM2 and SM9 as well? The rest of the SM family are unpublished/untranslated as far as I'm aware.

levitte · 2017-10-30T08:18:50Z

I've also a scheme to move to an EVP only interface -- this means replacing the historic direct calls with wrappers that use the EVP interface. It also renaming internal uses of these functions and (perhaps) the structures that are used. I.e. somewhat invasive :(

I argued the exact same thing years ago, and personally encourage it. You need to know, though, that some are concerned with speed and how that will be affected by this move (that's the reason I never went ahead with this)

ronaldtse · 2017-10-30T10:00:44Z

@paulidale @levitte restructured to internal EVP now, using lowercase function names.

Yes @paulidale we will be contributing SM2 as well, as @richsalz , @levitte and @InfoHunter already know. We haven't done SM9 but will see what happens!

dot-asm · 2017-10-30T10:31:56Z

crypto/sm3/sm3_locl.h

+       D = TT1;                                                   \
+       F = ROTATE(F, 19);                                         \
+       H = P0(TT2);                                               \
+     } while(0);


Whole point behind do { } while(0) in macro is to not end it with ;.

Oops, introduced while converting this from an inline to a macro

dot-asm · 2017-10-30T10:36:28Z

crypto/sm3/sm3_locl.h

+       const SM3_WORD A12_SM = A12 + E + TJ;                      \
+       const SM3_WORD SS1 = ROTATE(A12_SM, 7);                    \
+       const SM3_WORD TT1 = FF(A, B, C) + D + (SS1 ^ A12) + (Wj); \
+       const SM3_WORD TT2 = GG(E, F, G) + H + SS1 + Wi;           \


What's with const qualifiers? With variables one would customarily use it with assignment that can be evaluated at compile time, but here it makes no sense and asks for [unnecessary] question "why" ;-)

This is the idiom used in the originating C++ code in Botan (any variable that is not mutated is marked const so it is clear to the reader no later modifications occur, without having to scan all later uses to verify this.)

In the spirit of moving ahead, @dot-asm would you want us to remove the const?

I use const in contexts like these as an aid to prevent programmer error -- the compiler catches attempts to assign to const variables by mistake. Not so much for when I first write the code but for the developer who gets to maintain it ten years later.

dot-asm · 2017-10-30T10:47:10Z

Regarding lowercasing the function names, I'm not sure that's an absolute necessity. We haven't been picky about that before (see SM4... or Blake2, for that matter)...

It has lesser to do export-ability, but rather with how algorithm is referred to in general. With all capitals customarily denoting abbreviations.

dot-asm · 2017-10-30T11:02:40Z

I've also a scheme to move to an EVP only interface -- this means replacing the historic direct calls with wrappers that use the EVP interface.

I'd say that this effectively encourages application developers to not rewrite their code, and I'd view it as negative thing. Or in other words direct calls should be simply privatized in next major release without any "convenience" patches. I for one would even take it step further and discourage application developers from using even things like EVP_sm3() (and by extension EVP_sha1()). Instead application should look it up by nid or symbolic name at run time and act accordingly. [Well, one might find it appropriate to compromise and say that those EVP_algo() that can't be disabled could be exported, but those which can be disabled shouldn't really.] This has everything to do with idea of "universal" headers. And just in case it wasn't clear, this is something that could be done only in next major release.

kaduk · 2017-10-30T14:57:08Z

this is something that could be done only in next major release.

Do we have a wiki page or something where we are keeping track of major changes we want to make in the next major release?

mattcaswell · 2017-10-30T15:00:11Z

Do we have a wiki page or something where we are keeping track of major changes we want to make in the next major release?

The closest thing we have AFAIK is the "1.2.0" label. There aren't any issues using that label, but there are a number of PRs that have that against them. We should probably create issues when we identifying this like this and assign the "1.2.0" label.

levitte · 2017-10-30T17:49:56Z

doc/man3/EVP_sm3.pod

+
+ #include <openssl/evp.h>
+
+ const EVP_MD *EVP_digestname(void)


doc-nits complains about this missing in the NAME section. In this case, it's just one name, so I think it would be better to simply have EVP_sm3 instead of EVP_digestname here...

levitte · 2017-10-30T17:50:20Z

doc/man3/SM3.pod

@@ -0,0 +1,76 @@
+=pod


Weren't you going to remove this file?

Indeed, it's removed now. Just a reminder that with this change, now each EVP hash algorithm is going to need its own man file 😉

paulidale · 2017-10-30T22:55:39Z

Were folks aware of http://gmssl.org/ ?
Some assembly versions are provided for i64.

richsalz · 2017-10-30T23:03:31Z

Yeah, I took a look at it. Based on some advice I got during our recent China trip, it didn't get more than a cursory look.

ronaldtse · 2017-10-31T07:58:48Z

@levitte I've removed all references of SM3 from EVP_DigestInit.pod for doc-nits to stop complaining.

levitte · 2017-10-31T08:24:28Z

INSTALL

-                   synonymous with rmd160.
+				   rc2, rc4, rmd160, scrypt, seed, siphash, sm3, sm4 or
+				   whirlpool.  The "ripemd" algorithm is deprecated and if used
+				   is synonymous with rmd160.


Why was the indentation changed? We prefer all spaces indentation as tabs can be set different. You obviously use 4-column tabs ;-)

Argh it was from a merge conflict between sm3 and sm4 😉 Thank you for this.

levitte · 2017-10-31T08:27:26Z

Also, I'm curious, how is GM/T 0004-2012 related to this?

ronaldtse · 2017-10-31T08:33:46Z

The SM3 hash algorithm has been specified in multiple documents of differing importance.

The first public document of SM3 was published by OSCCA (State Cryptography Office) as a "Cryptographic Standard" GM/T 0004-2012. It was "upgraded" to a "China National Standard" by the SAC and TC 260 (China Standards Body) in GB/T 32905-2016 with some minor changes (to documentation, not to the algorithm).

levitte · 2017-10-31T08:39:42Z

Ok. It's curious in my view, but then I'm used to only refering to the last RFC we conform to, and that RFC having back references to older RFCs on the same matter. Either way, I'm not asking you to change your references, only to satisfy my curiousity... which you have now done, thank you :-)

ronaldtse · 2017-10-31T09:18:03Z

It would be sad if no one was curious on something I found interesting, so thank you 😉

levitte · 2017-10-31T09:44:44Z

If there is an english version of GB/T 32905-2016, I'd love to read it. All I've been able to find is old IETF drafts, and I'd like to understand a bit better what corresponds to what

ronaldtse · 2017-11-02T04:33:06Z

@levitte Is there anything further we can do to move this ahead? Thanks!

paulidale

A few minor nits only.

paulidale · 2017-11-02T04:41:25Z

crypto/include/internal/sm3.h

+#  error SM3 is disabled.
+# endif
+
+# ifdef  __cplusplus


I do wonder if this is needed for an internal header file.

paulidale · 2017-11-02T04:42:27Z

crypto/sm3/m_sm3.c

+
+#include <openssl/evp.h>
+#include "internal/evp_int.h"
+#include "internal/sm3.h"


A space between the # and include for these.

paulidale · 2017-11-02T04:43:17Z

crypto/sm3/sm3.c

+
+#ifndef OPENSSL_NO_SM3
+
+#include <openssl/e_os2.h>


Add spaces to these two too.

levitte · 2017-11-02T05:03:00Z

crypto/include/internal/sm3.h

+# endif
+
+#define SM3_DIGEST_LENGTH 32
+#define SM3_WORD unsigned int


While we're picking nits, there two need a space between # and defined

ronaldtse · 2017-11-02T07:57:34Z

Will get the nits ironed out soon!

ronaldtse · 2017-11-02T13:20:39Z

@levitte @paulidale the nits have been addressed. Please feel free to let me know if there’s anything further to be done. Thanks!

paulidale

I'm happy.

levitte

It turns out that you need to do another make update. You will see fuzz/oids.txt get an update.

levitte · 2017-11-02T22:52:37Z

.travis.yml

@@ -168,7 +168,7 @@ script:
      else
          echo -e '+\057 MAKE UPDATE FAILED'; false;
      fi;
-      git diff --quiet
+      git diff --name-only


You can remove this now.

Thanks for the tip! Done.

levitte

Looks good to me.

Are you fine with us squashing (and editing your commit message) or do you prefer to do it yourself?

ronaldtse · 2017-11-03T11:06:18Z

@levitte do you want to squash into one commit? I'm happy do so.

ronaldtse · 2017-11-04T00:15:49Z

@levitte squashed to 2 commits now and rebased to latest master. Good to merge?

levitte · 2017-11-04T08:31:50Z

Looks good enough for me.

paulidale · 2017-11-05T21:52:10Z

Need a rebase to master before merging.

SM3 is a secure hash function which is part of the Chinese "Commercial Cryptography" suite of algorithms which use is required for certain commercial applications in China.

ronaldtse · 2017-11-05T23:21:39Z

@paulidale all done!

SM3 is a secure hash function which is part of the Chinese "Commercial Cryptography" suite of algorithms which use is required for certain commercial applications in China. Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #4616)

Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #4616)

paulidale · 2017-11-06T01:10:11Z

Merged, thanks.

ronaldtse · 2017-11-06T03:26:05Z

Thank you @paulidale @levitte @mattcaswell @dot-asm and @richsalz !

paulidale · 2017-11-06T03:59:57Z

Something the reviewers missed which I've added for SM3 and SM4 in #4678 but should go in for SM2 when you make that PR.

Things that can be disabled should be added to the openssl list -disabled command.

ronaldtse · 2017-11-06T04:44:43Z

Indeed we will keep this in mind. Thanks!

Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl/openssl#4616)

ronaldtse changed the title ~~Add sm3 hash~~ An implementation of the Chinese SM3 hash algorithm Oct 30, 2017

levitte requested changes Oct 30, 2017

View reviewed changes

dot-asm reviewed Oct 30, 2017

View reviewed changes

levitte requested changes Oct 30, 2017

View reviewed changes

paulidale mentioned this pull request Oct 30, 2017

Only expose EVP interface for crypto primitives #4619

Closed

ronaldtse force-pushed the add-sm3-hash branch from 7ba3bc9 to cb26fba Compare October 31, 2017 06:31

ronaldtse mentioned this pull request Oct 31, 2017

EVP digest list: one hash algorithm per file, synchronize EVP list, o… #4628

Closed

1 task

levitte requested changes Oct 31, 2017

View reviewed changes

paulidale reviewed Nov 2, 2017

View reviewed changes

levitte requested changes Nov 2, 2017

View reviewed changes

ronaldtse force-pushed the add-sm3-hash branch from 7e75b19 to 7159bde Compare November 2, 2017 09:50

paulidale approved these changes Nov 2, 2017

View reviewed changes

levitte requested changes Nov 2, 2017

View reviewed changes

ronaldtse force-pushed the add-sm3-hash branch from e30dc79 to 00c6b38 Compare November 3, 2017 00:20

levitte approved these changes Nov 3, 2017

View reviewed changes

ronaldtse force-pushed the add-sm3-hash branch from 00c6b38 to b44957c Compare November 3, 2017 11:05

ronaldtse force-pushed the add-sm3-hash branch from b44957c to 3b3d3d0 Compare November 4, 2017 00:15

Jack Lloyd and others added 2 commits November 6, 2017 07:21

SM3: Add SM3 hash function

88194dd

SM3 is a secure hash function which is part of the Chinese "Commercial Cryptography" suite of algorithms which use is required for certain commercial applications in China.

SM3: restructure to EVP internal and update doc to right location

0ca2733

ronaldtse force-pushed the add-sm3-hash branch from 3b3d3d0 to 0ca2733 Compare November 5, 2017 23:21

levitte pushed a commit that referenced this pull request Nov 6, 2017

SM3: restructure to EVP internal and update doc to right location

67e247f

Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #4616)

paulidale closed this Nov 6, 2017

liuqun mentioned this pull request Apr 8, 2018

OpenSSL 1.1.1预览版已经开始支持SM2/SM3/SM4算法 guanzhi/GmSSL#468

Closed

busterb pushed a commit to libressl/fuzz that referenced this pull request Jan 23, 2019

SM3: restructure to EVP internal and update doc to right location

ad6c8c8

Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl/openssl#4616)

Yikun mentioned this pull request Jul 17, 2020

SM2/SM3/SM4 algorithm support for ssl protocol #12473

Open

thedreamwork mentioned this pull request Jul 25, 2022

Add SM2 Digital Signature Algorithm Support w3c/webauthn#1783

Closed

azasypkin mentioned this pull request Jan 1, 2024

Add support for SM2, SM3 and SM4 secutils-dev/secutils#40

Open

An implementation of the Chinese SM3 hash algorithm #4616

An implementation of the Chinese SM3 hash algorithm #4616

Conversation

ronaldtse commented Oct 30, 2017 • edited Loading

Checklist

paulidale commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levitte commented Oct 30, 2017

levitte commented Oct 30, 2017

paulidale commented Oct 30, 2017 • edited Loading

paulidale commented Oct 30, 2017

levitte commented Oct 30, 2017 • edited Loading

ronaldtse commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm commented Oct 30, 2017

dot-asm commented Oct 30, 2017

kaduk commented Oct 30, 2017

mattcaswell commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale commented Oct 30, 2017

richsalz commented Oct 30, 2017

ronaldtse commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levitte commented Oct 31, 2017

ronaldtse commented Oct 31, 2017

levitte commented Oct 31, 2017

ronaldtse commented Oct 31, 2017

levitte commented Oct 31, 2017

ronaldtse commented Nov 2, 2017

paulidale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronaldtse commented Nov 2, 2017

ronaldtse commented Nov 2, 2017

paulidale left a comment

Choose a reason for hiding this comment

levitte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levitte left a comment

Choose a reason for hiding this comment

ronaldtse commented Nov 3, 2017

ronaldtse commented Nov 4, 2017

levitte commented Nov 4, 2017

paulidale commented Nov 5, 2017

ronaldtse commented Nov 5, 2017

paulidale commented Nov 6, 2017

ronaldtse commented Nov 6, 2017

paulidale commented Nov 6, 2017

ronaldtse commented Nov 6, 2017

ronaldtse commented Oct 30, 2017 •

edited

Loading

paulidale commented Oct 30, 2017 •

edited

Loading

levitte commented Oct 30, 2017 •

edited

Loading