- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
(WIP) An implementation of the Chinese SM2 ECC #4793
Conversation
ronaldtse
commented
Nov 25, 2017
- documentation is added or updated
- tests are added or updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First quick look.
Key = tmppss | ||
Ctrl = rsa_mgf1_md:sha1 | ||
Result = PKEY_CTRL_ERROR | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhmmm, why are you taking these away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge error... will fix this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not fixed, it seems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed now
crypto/sm2/sm2_crypt.c
Outdated
ASN1_SIMPLE(SM2_Ciphertext, C1y, BIGNUM), | ||
ASN1_SIMPLE(SM2_Ciphertext, C3, ASN1_OCTET_STRING), | ||
ASN1_SIMPLE(SM2_Ciphertext, C2, | ||
ASN1_OCTET_STRING),} ASN1_SEQUENCE_END(SM2_Ciphertext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ASN1_SEQUENCE_END should be on his own line.
crypto/sm2/sm2_pmeth.c
Outdated
{ | ||
SM2_PKEY_CTX *dctx = ctx->data; | ||
EC_GROUP *group; | ||
switch (type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank line after declaration
include/openssl/sm2.h
Outdated
const char *user_id, const EC_KEY *key); | ||
|
||
/* | ||
* SM2 signatures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/*
* Multi-line comment
*/
test/sm2crypttest.c
Outdated
if (fake_rand_bytes == NULL) | ||
return saved_rand->bytes(buf, num); | ||
|
||
for(int i = 0; i != num; ++i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for(int i ...
not compatible with the old C standard .
Build will break.
Declaration should be done outside the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About error code macro generation...
Actually, there might be a choice to be made here... if SM2 is such a close relative to EC, why not put it all in crypto/ec/
? That would certainly make things less confusing error macro wise. But frankly, I'm unsure what's the right choice here, and I think that might be worth a discussion.
crypto/sm2/sm2_pmeth.c
Outdated
*siglen = ECDSA_size(ec); | ||
return 1; | ||
} else if (*siglen < (size_t)ECDSA_size(ec)) { | ||
ECerr(EC_F_PKEY_SM2_SIGN, EC_R_BUFFER_TOO_SMALL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be:
SM2err(SM2_F_PKEY_SM2_SIGN, SM2_R_BUFFER_TOO_SMALL);
include/openssl/ecerr.h
Outdated
# define EC_F_PKEY_SM2_CTRL_STR 275 | ||
# define EC_F_PKEY_SM2_KEYGEN 276 | ||
# define EC_F_PKEY_SM2_PARAMGEN 277 | ||
# define EC_F_PKEY_SM2_SIGN 278 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you add these manually? Don't, run make update
instead. As it is now, 274 is already taken by EC_F_EC_PKEY_PARAM_CHECK
crypto/err/openssl.txt
Outdated
EC_F_PKEY_SM2_CTRL_STR:275:pkey_sm2_ctrl_str | ||
EC_F_PKEY_SM2_KEYGEN:276:pkey_sm2_keygen | ||
EC_F_PKEY_SM2_PARAMGEN:277:pkey_sm2_paramgen | ||
EC_F_PKEY_SM2_SIGN:278:pkey_sm2_sign |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look manually inserted. Please take them away and let make update
do the job instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides, there's something off with these names, more on that in the code that defines them
crypto/sm2/sm2_pmeth.c
Outdated
case EVP_PKEY_CTRL_EC_PARAMGEN_CURVE_NID: | ||
group = EC_GROUP_new_by_curve_name(p1); | ||
if (group == NULL) { | ||
ECerr(EC_F_PKEY_SM2_CTRL, EC_R_INVALID_CURVE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be:
SM2err(SM2_F_PKEY_SM2_CTRL, SM2_R_INVALID_CURVE);
crypto/sm2/sm2_pmeth.c
Outdated
if (md_type != NID_sm3 && | ||
md_type != NID_sha256 && | ||
md_type != NID_sha512_256) { | ||
ECerr(EC_F_PKEY_SM2_CTRL, EC_R_INVALID_DIGEST_TYPE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be:
SM2err(SM2_F_PKEY_SM2_CTRL, SM2_R_INVALID_DIGEST_TYPE);
crypto/sm2/sm2_pmeth.c
Outdated
if (nid == NID_undef) | ||
nid = OBJ_ln2nid(value); | ||
if (nid == NID_undef) { | ||
ECerr(EC_F_PKEY_SM2_CTRL_STR, EC_R_INVALID_CURVE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be:
SM2err(SM2_F_PKEY_SM2_CTRL_STR, SM2_R_INVALID_CURVE);
crypto/sm2/sm2_pmeth.c
Outdated
SM2_PKEY_CTX *dctx = ctx->data; | ||
int ret = 0; | ||
if (dctx->gen_group == NULL) { | ||
ECerr(EC_F_PKEY_SM2_PARAMGEN, EC_R_NO_PARAMETERS_SET); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be:
SM2err(SM2_F_PKEY_SM2_PARAMGEN, SM2_R_NO_PARAMETERS_SET);
crypto/sm2/sm2_pmeth.c
Outdated
EC_KEY *ec = NULL; | ||
SM2_PKEY_CTX *dctx = ctx->data; | ||
if (ctx->pkey == NULL && dctx->gen_group == NULL) { | ||
ECerr(EC_F_PKEY_SM2_KEYGEN, EC_R_NO_PARAMETERS_SET); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be:
SM2err(SM2_F_PKEY_SM2_KEYGEN, SM2_R_NO_PARAMETERS_SET);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax issues. Basically, we've set the bar at ANSI C (that's C89 or C90 depending on who you ask... the difference is quite small between the two) for syntax. If you look att the Appveyor build, you will see it have quite a lot of issues when you try a more modern syntax, for example...
Note: I haven't picked at everything, my day is severely split up today, so expect me to get back.
... and thank you for your patience, I know this can be frustrating.
crypto/sm2/sm2_crypt.c
Outdated
int SM2_ciphertext_size(const EC_KEY *key, const EVP_MD *digest, size_t msg_len) | ||
{ | ||
return 10 + 2 * EC_field_size(EC_KEY_get0_group(key)) + | ||
EVP_MD_size(digest) + msg_len; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This calculates a size_t
, so SM2_ciphertext_size
should return size_t
.
crypto/sm2/sm2_crypt.c
Outdated
if (EVP_DigestFinal(hash, C3, NULL) == 0) | ||
goto done; | ||
|
||
struct SM2_Ciphertext_st ctext_struct; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We require ANSI C syntax, so this definition must move up to the top of the function.
crypto/sm2/sm2_crypt.c
Outdated
const int msg_len = sm2_ctext->C2->length; | ||
uint8_t msg_mask[msg_len]; | ||
|
||
const uint8_t *C3 = sm2_ctext->C3->data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ANSI C syntax. These definitions must move above the memset
line.
crypto/sm2/sm2_crypt.c
Outdated
if (sm2_ctext->C3->length != hash_size) | ||
goto done; | ||
|
||
EVP_MD_CTX *hash = EVP_MD_CTX_new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ANSI C syntax. This must move above the memset
line. Besides, this becomes a memory leak, as it isn't freed in the done
section...
crypto/sm2/sm2_crypt.c
Outdated
EC_POINT *kG = EC_POINT_new(group); | ||
EC_POINT *kP = EC_POINT_new(group); | ||
const EC_POINT *P = EC_KEY_get0_public_key(key); | ||
uint8_t msg_mask[msg_len]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ANSI C requires a constant expression for the size. You may need to allocate msg_mask
on the heap
crypto/sm2/sm2_crypt.c
Outdated
const size_t field_size = EC_field_size(group); | ||
uint8_t x2y2[2 * field_size]; | ||
uint8_t x2_bits[field_size]; | ||
uint8_t y2_bits[field_size]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ANSI C needs constant expressions...
crypto/sm2/sm2_crypt.c
Outdated
uint8_t y2_bits[field_size]; | ||
|
||
const size_t C3_size = EVP_MD_size(digest); | ||
uint8_t C3[C3_size]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constant expression...
crypto/sm2/sm2_crypt.c
Outdated
const size_t field_size = EC_field_size(group); | ||
uint8_t x2y2[2 * field_size]; | ||
uint8_t x2_bits[field_size]; | ||
uint8_t y2_bits[field_size]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constant expressions
crypto/sm2/sm2_crypt.c
Outdated
BIGNUM *x2 = BN_new(); | ||
BIGNUM *y2 = BN_new(); | ||
const size_t hash_size = EVP_MD_size(digest); | ||
uint8_t computed_C3[hash_size]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constant expression
crypto/sm2/sm2_crypt.c
Outdated
|
||
const uint8_t *C2 = sm2_ctext->C2->data; | ||
const int msg_len = sm2_ctext->C2->length; | ||
uint8_t msg_mask[msg_len]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constant expression...
crypto/sm2/sm2_crypt.c
Outdated
size_t msg_len, uint8_t *ciphertext_buf, size_t *ciphertext_len) | ||
{ | ||
int rc = 0; | ||
BIGNUM *k = BN_new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for not creating the ctx first then using BN_CTX_start() and then allocating k, x1, y1, x2, y2 using BN_CTX_get() to make the cleanup simpler. At the moment you seem to just be creating the ctx to pass down to other functions but don't use it in this function. You'd have to declare the variables first in that case, before assigning to them after starting the ctx.
crypto/sm2/sm2_crypt.c
Outdated
BIGNUM *x2 = BN_new(); | ||
BIGNUM *y2 = BN_new(); | ||
|
||
BN_CTX *ctx = BN_CTX_new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code needs to be a little more defensive, no checking for BN_CTX_new() returning NULL
crypto/sm2/sm2_crypt.c
Outdated
BIGNUM *x1 = BN_new(); | ||
BIGNUM *y1 = BN_new(); | ||
BIGNUM *x2 = BN_new(); | ||
BIGNUM *y2 = BN_new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently these also all need checking for NULL, if you move to allocate from the ctx then you only need to check the last allocation
crypto/sm2/sm2_crypt.c
Outdated
const EC_GROUP *group = EC_KEY_get0_group(key); | ||
const BIGNUM *order = EC_GROUP_get0_order(group); | ||
EC_POINT *kG = EC_POINT_new(group); | ||
EC_POINT *kP = EC_POINT_new(group); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again need to be a little more defensive these EC_POINT_new() calls could return NULL but there is no checking.
crypto/sm2/sm2_crypt.c
Outdated
BIGNUM *y2 = BN_new(); | ||
|
||
BN_CTX *ctx = BN_CTX_new(); | ||
EVP_MD_CTX *hash = EVP_MD_CTX_new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this one also needs a check for NULL too.
crypto/sm2/sm2_crypt.c
Outdated
memcpy(x2y2, x2_bits, field_size); | ||
memcpy(x2y2 + field_size, y2_bits, field_size); | ||
|
||
// This happens to match the KDF used in SM2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C++ style comment violates OpenSSL coding style document
crypto/sm2/sm2_crypt.c
Outdated
uint8_t x2_bits[field_size]; | ||
uint8_t y2_bits[field_size]; | ||
BIGNUM *x2 = BN_new(); | ||
BIGNUM *y2 = BN_new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here about starting and allocating from the ctx for these bignums.
Again same comments in this function about checking for NULL's from allocations.
crypto/sm2/sm2_pmeth.c
Outdated
if(der_sig == NULL) | ||
return 0; | ||
|
||
// Now ASN.1 encode ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong style of comment
crypto/sm2/sm2_pmeth.c
Outdated
} else if(strcmp(type, "user_id") == 0) { | ||
SM2_PKEY_CTX *dctx = ctx->data; | ||
free(dctx->user_id); | ||
dctx->user_id = OPENSSL_strdup(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional to drop out of this else after assigning the user_id and return -2. Surely you only want to return -2 on a failure case?
crypto/sm2/sm2_pmeth.c
Outdated
return 0; | ||
} | ||
ec = EC_KEY_new(); | ||
if (!ec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency, at line 202 you check (ec == NULL), here you check (!ec) after the same type of allocation. Better to be consistent and stick to (ec == NULL).
const char *user_id, | ||
const uint8_t *msg, size_t msg_len) | ||
{ | ||
EVP_MD_CTX *hash = EVP_MD_CTX_new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check for NULL if allocation fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
crypto/sm2/sm2_sign.c
Outdated
if (EVP_DigestUpdate(hash, msg, msg_len) == 0) | ||
goto done; | ||
|
||
// reuse za buffer to hold H(ZA || M) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again change comment style
crypto/sm2/sm2_sign.c
Outdated
{ | ||
EVP_MD_CTX *hash = EVP_MD_CTX_new(); | ||
const int md_size = EVP_MD_size(digest); | ||
uint8_t za[md_size]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another variable that probably needs allocating on the heap to avoid ANSI C violation.
crypto/sm2/sm2_sign.c
Outdated
BIGNUM *s = BN_new(); | ||
BIGNUM *x1 = BN_new(); | ||
BIGNUM *tmp = BN_new(); | ||
BN_CTX *ctx = BN_CTX_new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again why not move the ctx declaration higher and use the ctx for some of these BIGNUM allocations, the ones that are only in scope within this function, ie. k, x1 and tmp
crypto/sm2/sm2_sign.c
Outdated
const BIGNUM *dA = EC_KEY_get0_private_key(key); | ||
const EC_GROUP *group = EC_KEY_get0_group(key); | ||
const BIGNUM *order = EC_GROUP_get0_order(group); | ||
EC_POINT *kG = EC_POINT_new(group); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again a bit more checking of allocation failures required.
crypto/sm2/sm2_sign.c
Outdated
if (BN_mod_add(r, e, x1, order, ctx) == 0) | ||
goto done; | ||
|
||
// try again if r == 0 or r+k == n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong comment style again
|
||
BN_mod_mul(s, s, tmp, order, ctx); | ||
|
||
sig = ECDSA_SIG_new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allocation could fail and needs to be checked for NULL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
crypto/sm2/sm2_crypt.c
Outdated
== 0) | ||
goto done; | ||
|
||
for (size_t i = 0; i != msg_len; ++i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declare variable at top outside of loop.
crypto/sm2/sm2_crypt.c
Outdated
== 0) | ||
goto done; | ||
|
||
for (size_t i = 0; i != msg_len; ++i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again declare variable at top outside of loop
/* Is there some simpler way to do this? */ | ||
BIGNUM *p = BN_new(); | ||
BIGNUM *a = BN_new(); | ||
BIGNUM *b = BN_new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should check all of these allocations for NULL, or use a BN_CTX and check the last allocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
crypto/sm2/sm2_za.c
Outdated
|
||
const EC_GROUP *group = EC_KEY_get0_group(key); | ||
|
||
BN_CTX *ctx = NULL; // ??? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems pointless at the moment, you're just using it to pass a NULL parameter to EC_GROUP_get_curve_GFp() and EC_POINT_get_affine_coordinates_GFp(). You might as well either pass NULL directly to the functions without a variable if you want them to handle creating the BN_CTX or you should call BN_CTX_new() and BN_CTX_free() within this function and pass a valid pointer to the functions. The later will be more efficient than the current implementation as you'll only be doing a single BN_CTX_new()/BN_CTX_free() rather than 3.
crypto/sm2/sm2_za.c
Outdated
ZA=H256(ENTLA || IDA || a || b || xG || yG || xA || yA) | ||
*/ | ||
|
||
size_t uid_len = strlen(user_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More declarations of variables in the middle of a code block, several below as well, the declarations need to move to the top.
test/sm2crypttest.c
Outdated
{ | ||
BIGNUM* p = NULL; | ||
BN_hex2bn(&p, p_hex); | ||
BIGNUM* a = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the test code suffers from the same issues as already noted in the main files and needs sorting:
Variables declared in the middle of code blocks.
Variables declared based on sizes unknown at compile time (allocate dynamically)
Incorrect style of some comments.
I won't flag them individually.
Hey @ronaldtse, is something happening here? It's been very silent lately... |
Sorry guys got caught up in finishing up work before the holidays -- will try to finish this by the end of year 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just read through this PR as it currently stands, and it looks fine as it currently stands. The only thing that still stings the eye a bit is the set of public SM2 functions. However, I'm also concerned about the beta cycle starting tomorrow, so everything considered, I think it's better this gets into 1.1.1-dev now, and the public SM2 functions can be removed within the beta cycle, rather than not having this in 1.1.1 at all.
FYI, the conflicts with |
Sure. I already have the merge lined up, so ;-) |
Please raise an issue if there is something about this that needs to be fixed post-merge. |
Will do |
Also, apologies to @ronaldtse, @randombit and @InfoHunter for letting this one hanging for so long... |
Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #4793)
Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #4793)
Without actually using EVP_PKEY_FLAG_AUTOARGLEN Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #4793)
SM2_compute_userid_digest 4452 1_1_1 EXIST::FUNCTION: | ||
SM2_verify 4453 1_1_1 EXIST::FUNCTION: | ||
SM2_sign 4454 1_1_1 EXIST::FUNCTION: | ||
ERR_load_SM2_strings 4455 1_1_1 EXIST::FUNCTION: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these actually have to be exported? And if actually so, does it mean that no-sm2 wouldn't work? And it seems that it doesn't. I mean attempt to configure with no-sm2 fails. Is it actually intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #5670.
The behavior of
|
It is very nature that the same |
When signing message M, the SM2 sign the digest of Is the current implementation of EVP support this behavior (Z value is digest updated before message M) ? |
Hmmm... when I initially started reviewing this, I was under the distinct impression that the SM2 algo was tightly coupled with the SM2 specific curves, and the responses I got didn't give me any other impression. |
Personally I was unaware of any additional curve support in SM2. Re splitting out the PKEY, this would probably make more sense given the above comments. I initially did create the PR with a distinct SM2 PKEY but it seemed a needless complication (see comments in early January). |
According to GB/T 32918.5-2017 (the fifth part of the SM2 Standard), the implemented curve is no longer a "recommended curve" (which it was previously called), but it is "the" official curve. Therefore nothing further needs to be done for GB/T 32918 (SM2 Standard) compliance. That said, it is true that in GB/T 32915.{2,3,4} some examples provided are calculated over different curves, so for generality and flexibility reasons it would be beneficial to support such change. But I would consider it optional. |
I've not studied this in detail but from the comments I've seen here it seems to make sense to have a different pkey type. I'd rather make sure we have the correct solution rather than support something a bit quirky and not quite right for ever more. If we're going to do do it, we'd better get on with it. |
Could someone please create an issue instead of discussing in a closed PR? (the discussion thread is too big anyway, causes github not to want to load it at times) |
Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl/openssl#4793)