An implementation of the Chinese SM4 block cipher #4552

ronaldtse · 2017-10-18T15:46:25Z

This PR introduces the Chinese SM4 algorithm which is a 128-bit block cipher.

It was originally ported from Ribose's contribution to Botan, but has received further updates to make its performance on par with the SM4 implementation there.

Currently openssl speed -evp sm4-ecb reports:

~150 MB/s on @randombit 's machine
~110 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.

There might be some stuff missing but please do bring on the review process 😉 .

cc: @randombit @dewyatt @erikbor

This is a contribution from Ribose Inc..

Checklist

documentation is added or updated
tests are added or updated

mattcaswell · 2017-10-18T15:57:55Z

The CLA bot is complaining because the commit author in two of the commits is given as "Jack Lloyd" with an email address that is not in our CLA database.

ronaldtse · 2017-10-18T16:13:35Z

Thank you @mattcaswell for letting us know, we'll fix that.

ronaldtse · 2017-10-18T18:04:10Z

Some more things to fix:

crypto/sm4/sm4.c: In function 'SM4_T_slow':
crypto/sm4/sm4.c:117:5: error: C++ style comments are not allowed in ISO C90 [-Werror]
     // L linear transform
     ^
crypto/sm4/sm4.c:117:5: error: (this will be reported only once per input file) [-Werror]
crypto/sm4/sm4.c: In function 'SM4_set_key':
crypto/sm4/sm4.c:153:5: error: ISO C90 forbids mixed declarations and code [-Werror=pedantic]
     uint8_t i;
     ^

ronaldtse · 2017-10-18T18:28:24Z

One more:

�[0K$ if $make update; then echo -e '+\057 MAKE UPDATE OK'; else echo -e '+\057 MAKE UPDATE FAILED'; false; fi; git diff --quiet
LIBCRYPTO: 3 old symbols have updated info
LIBCRYPTO: No new symbols added
LIBSSL: No new symbols added
+/ MAKE UPDATE OK

travis_time:end:2e8b5d1f:start=1508350360675991669,finish=1508350365743647727,duration=5067656058
�[0K
�[31;1mThe command "if $make update; then echo -e '+/ MAKE UPDATE OK'; else echo -e '+/ MAKE UPDATE FAILED'; false; fi; git diff --quiet" exited with 1.�[0m

mattcaswell

This code looks very neat. I just have a few (mostly style) comments. I've not verified the implementation against the spec (where is the spec?).

mattcaswell · 2017-10-18T19:18:13Z

crypto/evp/e_sm4.c

+                            const SM4_KEY * key, const int enc)
+{
+    if (enc)
+


Remove this stray blank line

mattcaswell · 2017-10-18T19:26:14Z

crypto/include/internal/sm4.h

+
+void SM4_encrypt(const uint8_t *in, uint8_t *out, const SM4_KEY * ks);
+
+void SM4_decrypt(const uint8_t *in, uint8_t *out, const SM4_KEY * ks);


No space between '*' and ks, i.e. it should be const SM4_KEY *ks

mattcaswell · 2017-10-18T19:32:20Z

crypto/sm4/sm4.c

+
+static inline uint32_t SM4_T_slow(uint32_t X)
+{
+    uint32_t t = 0;


Blank line after declaration

mattcaswell · 2017-10-18T19:36:17Z

crypto/sm4/sm4.c

+    for (i = 0; i != 32; ++i) {
+        uint32_t X = K[(i + 1) % 4] ^ K[(i + 2) % 4] ^ K[(i + 3) % 4] ^ CK[i];
+
+        uint32_t t = 0;


No blank line above here - should be one below it.

mattcaswell · 2017-10-18T19:39:49Z

crypto/sm4/sm4.c

+    K[2] = load_u32_be(key, 2) ^ FK[2];
+    K[3] = load_u32_be(key, 3) ^ FK[3];
+
+    for (i = 0; i != 32; ++i) {


Probably should be SM4_KEY_SCHEDULE instead of 32?

mattcaswell · 2017-10-18T19:42:47Z

crypto/sm4/sm4.c

+    return 1;
+}
+
+#define SM4_RNDS(k0,k1,k2,k3,F) do {         \


Leave a space after each ,

mattcaswell · 2017-10-18T19:44:39Z

test/recipes/30-test_evp_data/evpciph.txt

@@ -2078,6 +2078,36 @@ Operation = ENCRYPT
 Plaintext = 000102030405060708090A0B0C0D0E0F101112131415161718191A1B1C1D1E1F20212223
 Ciphertext = A4DA23FCE6A5FFAA6D64AE9A0652A42CD161A34B65F9679F75C01F101F71276F15EF0D8D

+Title = SM4 test vectors


What is the origin of these test vectors? Can we include a reference?

Included a reference to IETF draft-ribose-cfrg-sm4.

mattcaswell · 2017-10-18T19:46:04Z

test/sm4test.c

+    SM4_KEY key;
+    SM4_set_key(k, &key);
+
+    uint8_t block[16];


Can't use SM4_BLOCK_SIZE here - but perhaps a define instead of a magic number here (and other instances below) would be better?

Actually, since internal/sm4.h is included, it's perfectly possible to use SM4_BLOCK_SIZE...

Using SM4_BLOCK_SIZE now, moved to test/sm4_internal_test.c.

levitte · 2017-10-18T20:14:28Z

util/libcrypto.num

+EVP_sm4_cbc                             4369	1_1_1	EXIST::FUNCTION:SM4
+EVP_sm4_ofb                             4370	1_1_1	EXIST::FUNCTION:SM4
+EVP_sm4_cfb128                          4371	1_1_1	EXIST::FUNCTION:SM4
+EVP_sm4_ctr                             4372	1_1_1	EXIST::FUNCTION:SM4


I suggest you remove these changes and do a make update again. You'll find that the SM4_* symbols will not appear... (that's the update of 3 symbols that's mentioned in Travis, make update wants to change their state to NOEXIST)

@levitte I've done this. Somehow make update on my machine adds a last line of license ... EXIST::FUNCTION to the end, which I've removed manually... Let me know if this behavior is correct.

I have no idea why it adds a license line... It shouldn't, and it doesn't for me...

Thanks @levitte for the confirmation 👍

paulidale · 2017-10-18T21:30:51Z

When I looked at these a while back, the best English reference I could locate for SM4 / SMS4 is: SMS4 Encryption Algorithm for Wireless Networks. There is a much better IETF draft now. There is of course a Chinese specification but I'm unable to read it.

The question in my mind is should the Chinese cryptographic primitives be wrapped in an engine like GOST is?

SM4 is but one of several defined algorithms, SM2 and SM3 are also IETF drafts, albeit expired ones. I also found a reference describing SM9. I've not looked for a ZUC definition. There are more algorithms than this.

ronaldtse · 2017-10-19T07:47:22Z

@paulidale the Internet-Draft for SM4 is intended to fully cover the official standard GB/T 32907-2016 (in Sections 1 to 7), as well as modes of operation and some considerations that were not covered by the standard.

SMS4 Encryption Algorithm for Wireless Networks is a translation based on an older standard published by OSCCA called GM/T 0002-2012, which is not currently available on the Internet.

levitte · 2017-10-19T09:02:26Z

test/sm4_internal_test.c

+       0x86, 0xb3, 0xe9, 0x4f, 0x53, 0x6e, 0x42, 0x46
+    };
+
+    // after 1,000,000 iterations


Change that to a classic C comment, please

levitte · 2017-10-19T09:11:48Z

test/recipes/05-test_sm4.t

+
+use OpenSSL::Test::Simple;
+
+simple_test("test_internal_sm4", "sm4_internal_test", "sm4");


Please rename this file to go along with the other internal tests. 03-test_internal_sm4.t would be a good one.

Also, since this is only built under certain condicions, you need to add the following as well, before the simple_test line:

plan skip_all => "This test is unsupported in a shared library build on Windows" if $^O eq 'MSWin32' && !disabled("shared");

Done, thanks @levitte !

InfoHunter · 2017-10-19T13:29:15Z

crypto/evp/e_sm4.c

@@ -0,0 +1,103 @@
+/*
+ * Copyright 2017 The OpenSSL Project Authors. All Rights Reserved.
+ * Copyright 2017 [Ribose Inc.](https://www.ribose.com). All Rights Reserved.


As I saw in other files, seems the copyright line should contain company name without the URL (correct me if I am wrong), the []() likes a markdown syntax...

It seems odd to me too. Better would be:

Copyright 2017 Ribose Inc. All Rights Reserved.

InfoHunter · 2017-10-19T13:31:01Z

crypto/objects/obj_mac.num

+sm4_cfb128		1137
+sm4_cfb8		1138
+sm4_ctr		1139
+ISO_CN		1140


Should the number be aligned to others? Or the 2-tabs is fixed there?

This file is generated, so...

I will feel challenged to make the generation script properly align 😉

InfoHunter · 2017-10-19T13:33:33Z

crypto/sm4/sm4.c

+};
+
+/*
+* SM4_SBOX_T[j] == L(SM4_SBOX[j]).


Indentation. And I think this could be placed in one line as /* */...

Indentation addressed.

InfoHunter · 2017-10-19T13:36:54Z

crypto/sm4/sm4.c

+* SM4_SBOX_T[j] == L(SM4_SBOX[j]).
+*/
+static const uint32_t SM4_SBOX_T[256] = {
+   0x8ED55B5B, 0xD0924242, 0x4DEAA7A7, 0x06FDFBFB, 0xFCCF3333, 0x65E28787,


4 spaces indent...

InfoHunter · 2017-10-19T13:39:38Z

test/recipes/03-test_internal_sm4.t

@@ -0,0 +1,19 @@
+#! /usr/bin/env perl
+# Copyright 2015-2016 The OpenSSL Project Authors. All Rights Reserved.


Copyright 2017 The OpenSSL...?

InfoHunter · 2017-10-19T13:41:06Z

test/sm4_internal_test.c

+static int test_sm4_ecb(void)
+{
+    static const uint8_t k[SM4_BLOCK_SIZE] = {
+       0x01, 0x23, 0x45, 0x67, 0x89, 0xab, 0xcd, 0xef,


Indent should be 4-spaces, so do the followings...

InfoHunter · 2017-10-19T13:49:57Z

crypto/sm4/sm4.c

+    return 1;
+}
+
+#define SM4_RNDS(k0, k1, k2, k3, F) do {     \


I guess it's preferred if the do statement is placed in next line as:

#define macrofun(a, b, c) \ do { \ if (a == 5) \ do_this(b, c); \ } while (0)

InfoHunter · 2017-10-19T13:59:53Z

And documentation is missing...

levitte · 2017-10-19T20:12:06Z

And documentation is missing...

@InfoHunter is right. There should be a new entry in the CIPHER LISTING section in doc/man3/EVP_EncryptInit.pod.

paulidale · 2017-10-19T21:21:41Z

@ronaldtse thanks for the better links.

ronaldtse · 2017-10-20T02:01:46Z

@InfoHunter @levitte added new section in CIPHER LISTING. I noticed that some of the ciphers are actually missing in doc/man3/EncryptInit.pod:

all the CTR modes: EVP_aes_{128,192,256}_ctr(), EVP_aria_{128,192,256}_ctr(), EVP_camellia_{128,192,256}_ctr()
all the aria ones: EVP_aria_{128,192,256}_{ecb,cbc,cfb,cfb1,cfb8,ctr,ofb,gcm,ccm}
all the camellia ones: EVP_camellia_{128,192,256}_{ecb,cbc,cfb,cfb1,cfb8,ctr,ofb,gcm,ccm}

Should these be added?

kroeckx · 2017-10-20T04:00:04Z

They should probably all get added, but I suggest you do that in a different pull request so that we can merge that to other branches.

dot-asm · 2017-10-20T21:13:33Z

crypto/sm4/sm4.c

+    0x794C3535, 0xA0208080, 0x9D78E5E5, 0x56EDBBBB, 0x235E7D7D, 0xC63EF8F8,
+    0x8BD45F5F, 0xE7C82F2F, 0xDD39E4E4, 0x68492121 };
+
+static inline uint32_t rotl(uint32_t a, uint8_t n)


inline is not portable, use ossl_inline.

Thanks @dot-asm ! Addressed.

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

One last rebase.

levitte

Yup

dot-asm · 2017-10-30T12:20:21Z

OMC is looking at it [enabled vs. disabled by default] now.

It's obvious that there probably can't be a solid common criteria for deciding either way. Especially since there are contradicting cases when things look the way they do rather for backward compatibility sake than anything else. But it doesn't mean that one shouldn't attempt to formulate multiple criteria and consider combination. One of such criteria can [or should?] be public record of independent cryptanalysis... Another is ability to withstand side channel attacks... Recognized [non-proprietary] application...

richsalz · 2017-10-30T12:22:09Z

Yes, @dot-asm, you're right. And also the ability of the project to maintain these things in the face of attacks. It's an interesting question.

dot-asm · 2017-10-30T13:08:08Z

BTW, siphash was previously mentioned. Note that it's not released, so one can still change that...

paulidale · 2017-10-30T21:46:07Z

A rebase is required before merge :(

paulidale · 2017-10-30T21:48:01Z

Another consideration is that enabling an algorithm wouldn't be considered API changing whereas disabling one would be. I.e. disabled by default is changeable in a minor release.

* remove for-loop initial declarations that are only allowed in C99 * update SM4 numbers in libcrypto.num via `make update` * fix C90 issues * style fixes as commented by mattcaswell * remaining style fixes * cleanup test vectors and add reference * use SM4_KEY_SCHEDULE instead of 32 * cleanup comments * include internal test, and use SM4_BLOCK_SIZE

* add documentation * use ossl_inline for portability * remove unnecessary includes * no spaces after cast * use (uint8_t) cast instead of `& 0xFF` * use int in for loop * use `or` in load_u32_be instead of `xor`

ronaldtse · 2017-10-30T23:52:53Z

@paulidale rebased again!

paulidale · 2017-10-31T00:29:47Z

Thanks, waiting for the Travis build to finish before merging.

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

paulidale · 2017-10-31T05:24:44Z

Merged, thanks.

ronaldtse · 2017-10-31T06:07:23Z

Thank you @paulidale and everyone here !

liuqun

Some trailing white spaces in crypto/objects/objects.txt were removed during this PR. Many text editors have such feature to remove all trailing white spaces, some of these white space changes are unrelated to the current PR.

Hope we can use some .c and .txt formatter to remove such unnecessary trailing white spaces before more new pull requests arrives.

util/openssl-format-source -v -c ignores crypto/objects/objects.txt, I guess.
See: https://github.com/openssl/openssl/blob/master/util/openssl-format-source

liuqun · 2018-04-08T07:31:07Z

crypto/objects/objects.txt

@@ -1368,7 +1368,7 @@ member-body 643 100 112	: issuerSignTool	: Signing Tool of Issuer
 # Definitions for Camellia cipher - ECB, CFB, OFB MODE

 !Alias ntt-ds 0 3 4401 5
-!Alias camellia ntt-ds 3 1 9 
+!Alias camellia ntt-ds 3 1 9


An unrelated white space change here.

liuqun · 2018-04-08T07:31:47Z

crypto/objects/objects.txt

@@ -1498,7 +1515,7 @@ ISO-US 10046 2 1	: dhpublicnumber		: X9.42 DH
 1 3 36 3 3 2 8 1 1 11 : brainpoolP384r1
 1 3 36 3 3 2 8 1 1 12 : brainpoolP384t1
 1 3 36 3 3 2 8 1 1 13 : brainpoolP512r1
-1 3 36 3 3 2 8 1 1 14 : brainpoolP512t1            
+1 3 36 3 3 2 8 1 1 14 : brainpoolP512t1


Another unrelated white space change.

InfoHunter · 2018-04-09T09:12:56Z

I don't think it was improper to remove these spaces here.

Long ago I learned the 'style' of this project which was: "if your changes are next to a nit, then you fix the nit incidentally' ;-)

levitte · 2018-04-09T09:15:54Z

Why these comments on a PR that was approved and merged a long time ago?

ronaldtse · 2018-04-09T15:29:29Z

@levitte seems that it was due to increased interest given news of OpenSSL 1.1.1 now supports these algorithms 😉👏

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Oct 18, 2017

ronaldtse mentioned this pull request Oct 18, 2017

Add SM4 block cipher riboseinc/openssl#2

Merged

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Oct 18, 2017

mattcaswell previously requested changes Oct 18, 2017

View reviewed changes

levitte requested changes Oct 18, 2017

View reviewed changes

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Oct 19, 2017

levitte requested changes Oct 19, 2017

View reviewed changes

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Oct 19, 2017

InfoHunter reviewed Oct 19, 2017

View reviewed changes

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Oct 20, 2017

dot-asm reviewed Oct 20, 2017

View reviewed changes

levitte approved these changes Oct 30, 2017

View reviewed changes

Jack Lloyd and others added 9 commits October 31, 2017 07:51

SM4: Add SM4 block cipher to EVP

5c9af7a

SM4: Fix code styling and indentation issues.

ecc6b5e

SM4: follow up on comments

5facb75

* add documentation * use ossl_inline for portability * remove unnecessary includes * no spaces after cast * use (uint8_t) cast instead of `& 0xFF` * use int in for loop * use `or` in load_u32_be instead of `xor`

SM4: add CHANGES

f8907c7

SM4: split out documentation to EVP_sm4.pod

3bed209

SM4: fix copyright messages

9ad7485

SM4: fix copyright message

4bb43dd

SM4: update libcrypto.num

75294ac

ronaldtse force-pushed the add-sm4 branch from c99d922 to 75294ac Compare October 30, 2017 23:52

levitte pushed a commit that referenced this pull request Oct 31, 2017

SM4: Add SM4 block cipher to EVP

f19a5ff

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

paulidale closed this Oct 31, 2017

liuqun reviewed Apr 8, 2018

View reviewed changes

liuqun mentioned this pull request Apr 8, 2018

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

Closed

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


		void SM4_encrypt(const uint8_t in, uint8_t out, const SM4_KEY * ks);

		void SM4_decrypt(const uint8_t in, uint8_t out, const SM4_KEY * ks);


		use OpenSSL::Test::Simple;

		simple_test("test_internal_sm4", "sm4_internal_test", "sm4");

		@@ -0,0 +1,19 @@
		#! /usr/bin/env perl
		# Copyright 2015-2016 The OpenSSL Project Authors. All Rights Reserved.

An implementation of the Chinese SM4 block cipher #4552

An implementation of the Chinese SM4 block cipher #4552

Conversation

ronaldtse commented Oct 18, 2017 • edited

Checklist

mattcaswell commented Oct 18, 2017

ronaldtse commented Oct 18, 2017

ronaldtse commented Oct 18, 2017

ronaldtse commented Oct 18, 2017

mattcaswell 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

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

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

Choose a reason for hiding this comment

paulidale commented Oct 18, 2017

ronaldtse commented Oct 19, 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

InfoHunter Oct 19, 2017 • edited

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

InfoHunter Oct 19, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InfoHunter commented Oct 19, 2017

levitte commented Oct 19, 2017

paulidale commented Oct 19, 2017

ronaldtse commented Oct 20, 2017

kroeckx commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronaldtse commented Oct 30, 2017

levitte left a comment

Choose a reason for hiding this comment

dot-asm commented Oct 30, 2017 • edited

richsalz commented Oct 30, 2017

dot-asm commented Oct 30, 2017

paulidale commented Oct 30, 2017

paulidale commented Oct 30, 2017

ronaldtse commented Oct 30, 2017

paulidale commented Oct 31, 2017

paulidale commented Oct 31, 2017

ronaldtse commented Oct 31, 2017

liuqun left a comment • edited

Choose a reason for hiding this comment

liuqun Apr 8, 2018 • edited

ronaldtse commented Oct 18, 2017 •

edited

InfoHunter Oct 19, 2017 •

edited

InfoHunter Oct 19, 2017 •

edited

dot-asm commented Oct 30, 2017 •

edited

liuqun left a comment •

edited

liuqun Apr 8, 2018 •

edited