Move SM2 to its own PMETH #6443

randombit · 2018-06-08T22:45:22Z

This moves SM2 to its own PMETH following the approach suggested by @mattcaswell in
#6402 fixing issue #5924

This is a contribution from Ribose Inc.

mattcaswell

Looks good! Mostly my comments are minor style issues. We will need some documentation for the new EVP_PKEY_set_alias_type() function.

mattcaswell · 2018-06-11T10:15:56Z

crypto/evp/p_lib.c

+        return 0;
+    }
+
+    if(pkey->type == type) {


Style nit: need a space between if and (, also with the other if above.

mattcaswell · 2018-06-11T10:16:50Z

crypto/evp/p_lib.c

+    }
+
+    if(pkey->type == type) {
+        return 1; // it already is that type


Style: We only allow /*...*/ comments. This is actually the cause of the travis failure.

mattcaswell · 2018-06-11T10:18:57Z

crypto/evp/p_lib.c

+    /*
+    The application is requesting to alias this to a different pkey type,
+    but not one that resolves to the base type.
+    */


Style: Multi line comments should be like this:

/* * The application is requesting to alias this to a different pkey type, * but not one that resolves to the base type. */

mattcaswell · 2018-06-11T10:55:29Z

crypto/evp/p_lib.c

+    The application is requesting to alias this to a different pkey type,
+    but not one that resolves to the base type.
+    */
+    if (ameth->pkey_id != pkey->type) {


I'm not sure this test is correct. The pkey->type is the type of the EVP_PKEY object (e.g. either EVP_PKEY_EC or EVP_PKEY_SM2) whereas ameth->pkey_id is the base type which will always be EVP_PKEY_EC in the SM2 case. With the test as it is, using this function to change an EC key into an SM2 key will work, but you can't use it to change an SM2 key back to an EC key.

Probably we need to compare the base type of the EVP_PKEY object, with the base type of the ameth which I think can be simplified to:

if (EVP_PKEY_type(type) != EVP_PKEY_base_id(pkey) {

mattcaswell · 2018-06-11T11:17:05Z

crypto/evp/p_lib.c

+        return 0;
+    }
+
+    pkey->save_type = pkey->type;


I think save_type is the original type of the object, so should probably be left unchanged.

mattcaswell · 2018-06-11T11:29:56Z

crypto/sm2/sm2_pmeth.c

+{
+    if (strcmp(type, "ec_paramgen_curve") == 0) {
+        int nid;
+        nid = EC_curve_nist2nid(value);


Blank line after declarations

mattcaswell · 2018-06-11T11:30:22Z

crypto/sm2/sm2_pmeth.c

+        return EVP_PKEY_CTX_set_ec_paramgen_curve_nid(ctx, nid);
+    } else if (strcmp(type, "ec_param_enc") == 0) {
+        int param_enc;
+        if (strcmp(value, "explicit") == 0)


blank line after declarations

mattcaswell · 2018-06-11T11:30:39Z

crypto/sm2/sm2_pmeth.c

+    EC_KEY *ec = NULL;
+    SM2_PKEY_CTX *dctx = ctx->data;
+    int ret = 0;
+    if (dctx->gen_group == NULL) {


Blank line after declarations

mattcaswell · 2018-06-11T11:31:12Z

crypto/sm2/sm2_pmeth.c

+    if (ec == NULL)
+        return 0;
+    ret = EC_KEY_set_group(ec, dctx->gen_group);
+    if (ret)


ret != NULL

ret is an int not pointer. Do you want ret != 0 here?

Oops. My mistake. Leave it as it is.

mattcaswell · 2018-06-11T11:31:32Z

crypto/sm2/sm2_pmeth.c

+{
+    EC_KEY *ec = NULL;
+    SM2_PKEY_CTX *dctx = ctx->data;
+    if (ctx->pkey == NULL && dctx->gen_group == NULL) {


Blank line after declarations

stevelinsell · 2018-06-11T15:02:48Z

crypto/sm2/sm2_pmeth.c

+        return 0;
+    }
+    ec = EC_KEY_new();
+    if (!ec)


Consistency. At line 256 you do the equivalent check of ec == NULL but in this function you do !ec. Change the !ec to ec == NULL as well.

randombit · 2018-06-11T16:30:40Z

@mattcaswell All review comments addressed. I also added a basic doc. I didn't find an existing pod that seemed to make sense, so I created doc/man3/EVP_PKEY_type.md to document all the various (currently undocumented) functions for setting/getting EVP_PKEY type. However beyond giving the declarations, only EVP_PKEY_set_alias_type is actually documented there.

mattcaswell · 2018-06-12T12:48:22Z

crypto/evp/p_lib.c

+    if (ameth == NULL) {
+        EVPerr(EVP_F_EVP_PKEY_SET_ALIAS_TYPE, EVP_R_UNSUPPORTED_ALGORITHM);
+        return 0;
+    }


I think this ameth stuff can probably be removed now that you amended the "if" condition below to not need it.

It stung my eye too. I mean it is redundant, because EVP_PKEY_type below calls EVP_PKEY_asn1_find and check for ameth being non-NULL.

And while you're at it, style guide suggests that block comments get stars aligned at opening star, i.e. and 4n+1 position, not 4n.

mattcaswell · 2018-06-12T12:50:49Z

crypto/sm2/sm2_pmeth.c

+        if (EVP_MD_type((const EVP_MD *)p2) != NID_sha256 &&
+            EVP_MD_type((const EVP_MD *)p2) != NID_sha384 &&
+            EVP_MD_type((const EVP_MD *)p2) != NID_sha512 &&
+            EVP_MD_type((const EVP_MD *)p2) != NID_sm3) {


Sorry missed this on the first read through. Please put the "&&" on the beginning of the next line.

mattcaswell · 2018-06-12T12:52:41Z

crypto/sm2/sm2_pmeth.c

+    if (ec == NULL)
+        return 0;
+    EVP_PKEY_assign_EC_KEY(pkey, ec);
+    if (ctx->pkey) {


ctx->pkey != NULL

mattcaswell · 2018-06-12T12:53:05Z

crypto/sm2/sm2_pmeth.c

+        if (!EVP_PKEY_copy_parameters(pkey, ctx->pkey))
+            return 0;
+    } else {
+        if (!EC_KEY_set_group(ec, dctx->gen_group))


We don't seem to then use ec for anything after this point.

I am not sure what I should change. This function is copied from same function in ec_pmeth.c fwiw

Forget it - my mistake. It's assigned to the pkey above here.

mattcaswell · 2018-06-12T12:56:16Z

doc/man3/EVP_PKEY_type.md

+EVP_PKEY_base_id,
+EVP_PKEY_set_type,
+EVP_PKEY_set_type_str,
+EVP_PKEY_set_alias_type,


I think we should either document all of these, or not include them in the pod at all (and remove them from the synopsis below). If you go with the latter of those two options then rename the file to match the one function that you are documenting. The problem is if we list these undocumented functions here then our scripts for finding undocumented functions will stop reporting them!

Turns out EVP_PKEY_type, EVP_PKEY_id and EVP_PKEY_base_id were already documented, just not where I expected (EVP_PKEY_set1_RSA). I will add the documentation of EVP_PKEY_set_alias_type to this same doc so at least they are all together.

mattcaswell · 2018-06-12T12:57:21Z

doc/man3/EVP_PKEY_type.md

+EVP_PKEY_set_type,
+EVP_PKEY_set_type_str,
+EVP_PKEY_set_alias_type,
+- examine and manipulate type of public key


Perhaps: "examine and manipulate the type of a key"? EVP_PKEY could be a private/public key pair or just a public key.

mattcaswell · 2018-06-12T13:00:40Z

doc/man3/EVP_PKEY_type.md

+
+EVP_PKEY_set_alias_type() allows modifying a EVP_PKEY to use a
+different set of algorithms than the default. This is currently used
+to support SM2 keys, which use an identical encoding to ECDSA.


I think adding a "=head1 EXAMPLE" section might be helpful.

I do also wonder whether it would be useful to have an "overview" type man page for SM2, e.g. see the other overviews in doc/man7. Or possibly a combined overview page covering SM2/3/4. Anyway - just a thought, and for a separate PR.

mattcaswell · 2018-06-12T13:01:21Z

doc/man3/EVP_PKEY_type.md

+
+=head1 COPYRIGHT
+
+Copyright 2006-2018 The OpenSSL Project Authors. All Rights Reserved.


mattcaswell · 2018-06-12T13:01:53Z

test/evp_test.c

+        /* Hack to detect SM2 keys */
+        if(strstr(key->name, "SM2")) {
+            EVP_PKEY_set_alias_type(pkey, EVP_PKEY_SM2);
+        }


No "{}" here.

randombit · 2018-06-12T14:45:44Z

Believe everything from round2 review is fixed in 7fd81fd. If all is ok let me know and I will rebase+squash the commits.

mattcaswell

LGTM

richsalz · 2018-06-12T16:57:38Z

crypto/ec/ec_ameth.c

+   EVP_PKEY_SM2,
+   EVP_PKEY_EC,
+   ASN1_PKEY_ALIAS
+   };


Nit: undent the close curly.

richsalz · 2018-06-12T16:59:09Z

crypto/evp/p_lib.c

+
+int EVP_PKEY_set_alias_type(EVP_PKEY *pkey, int type)
+{
+    if (pkey == NULL) {


We don't check for NULL parameters.

richsalz · 2018-06-12T17:00:42Z

crypto/sm2/sm2_pmeth.c

+    if (sig == NULL) {
+        *siglen = ECDSA_size(ec);
+        return 1;
+    } else if (*siglen < (size_t)ECDSA_size(ec)) {


I prefer not to see "else" here. Not a requirement since I know some poor misguided souls disagree. :)

richsalz · 2018-06-12T17:01:26Z

crypto/sm2/sm2_pmeth.c

+}
+
+static int pkey_sm2_verify(EVP_PKEY_CTX *ctx,
+                          const unsigned char *sig, size_t siglen,


Squinting nit: this and next line over one more space?

richsalz · 2018-06-12T17:01:41Z

crypto/sm2/sm2_pmeth.c

+        return 0;
+    }
+
+    if (dctx->md != NULL)


Optional: I prefer to see the ?: operator used here.

richsalz · 2018-06-12T17:04:15Z

crypto/sm2/sm2_pmeth.c

+        return 1;
+
+    case EVP_PKEY_CTRL_MD:
+        if (EVP_MD_type((const EVP_MD *)p2) != NID_sha256


Introduce a variable to hold EVP_MD_type value and compute once.

richsalz · 2018-06-12T17:04:39Z

crypto/sm2/sm2_pmeth.c

+                            const char *type, const char *value)
+{
+    if (strcmp(type, "ec_paramgen_curve") == 0) {
+        int nid;


can merge the decl and the init code into one line (but don't forget the blank line)

richsalz · 2018-06-12T17:06:10Z

crypto/sm2/sm2_pmeth.c

+        int nid;
+
+        nid = EC_curve_nist2nid(value);
+        if (nid == NID_undef)


I get it, avoiding walking-to-the-right. Perhaps this?

if ((nid = EV_curve_nist..) == NID_undef && (nid = OBJ_sn2nid...) == NID_undef && (nid = OBJ_ln2nid...) == NID_UNDEF) { SM2err ... }

richsalz · 2018-06-12T17:07:01Z

crypto/sm2/sm2_pmeth.c

+    SM2_PKEY_CTX *dctx = ctx->data;
+    int ret = 0;
+
+    if (dctx->gen_group == NULL) {


Is this really along the lines of a NUL check that we would typically avoid?

I'd suggest to remove this check too. As well as second another one...

Just instantiated object would have gen_group set to NULL. One can argue that calling paramgen without setting parameter is programming mistake, but it's probably not severe. So should this should probably stay? @richsalz?

richsalz · 2018-06-12T17:07:33Z

crypto/sm2/sm2_pmeth.c

+    EC_KEY *ec = NULL;
+    SM2_PKEY_CTX *dctx = ctx->data;
+
+    if (ctx->pkey == NULL && dctx->gen_group == NULL) {


similar, is this an assert-style error which we should coredump on?

As for check for ctx->pkey. Below we observe if (ctx->pkey != NULL) {} **else** {}. So which one is it?

As for dctx->gen_group, same as above...

Disregard last remark!!! I mean the if-else is legitimate, because the condition is if both are zero.

randombit · 2018-06-12T17:40:31Z

All review comments from @richsalz should be addressed by 19c2a45 except for the possibly undesired null pointer checks. I will point out that the EC pmeth has the same checks. Also, while (AIUI) the policy is to not check for NULL pointer arguments, ctx->pkey and dctx->gen_group are internal values set by the library itself. So crashing on a null pointer there is not likely to help an application developer understand that they should have made some previous call to set the parameter in the ctx.

Anyway I can remove them if needed, just let me know what the decision is.

richsalz · 2018-06-12T18:02:24Z

Keeping the NULL checks is fine. I approve this. Matt can reconfirm or merge as he sees fit. :)

richsalz

Thanks for all your work here!

randombit · 2018-06-12T18:25:45Z

Nice. Should I rebase+squash this PR?

richsalz · 2018-06-12T18:50:39Z

Rebase/push-f if you want, don't squash until @mattcaswell has looked at your latest commit.

dot-asm · 2018-06-12T21:08:00Z

crypto/sm2/sm2_pmeth.c

+    const int md_type = (dctx->md == NULL) ? NID_sm3 : EVP_MD_type(dctx->md);
+
+    if (out == NULL) {
+        if (!sm2_plaintext_size(ec, EVP_get_digestbynid(md_type), inlen,


I don't quite follow why does EVP_get_digestbynid have to be invovled here? As simplest case wouldn't dctx->md ? dctx->md : EVP_sm3() do the job? If one has to use EVP_get_digestbynid(NID_sm3), then `dctx->md ? dctx->md : EVP_get_digestbynid(NID_sm3). Either way this kind of calls for question if it would be appropriate to check for EVP_get_digestbynid return value. Well, no-sm3 implies no-sm2, so one can argue that lookup for NID_sm3 is bound to succeed [as well as availability of EVPsm3()].

dot-asm · 2018-06-12T21:08:51Z

crypto/sm2/sm2_pmeth.c

+
+static int pkey_sm2_encrypt(EVP_PKEY_CTX *ctx,
+                              unsigned char *out, size_t *outlen,
+                              const unsigned char *in, size_t inlen)


Formatting nit, two too many spaces in above two lines.

dot-asm · 2018-06-12T21:28:01Z

crypto/sm2/sm2_pmeth.c

+    if (strcmp(type, "ec_paramgen_curve") == 0) {
+       int nid = NID_undef;
+
+       if(((nid = EC_curve_nist2nid(value)) == NID_undef)


Space after if.

dot-asm · 2018-06-12T21:29:02Z

crypto/sm2/sm2_pmeth.c

+
+       if(((nid = EC_curve_nist2nid(value)) == NID_undef)
+          && ((nid = OBJ_sn2nid(value)) == NID_undef)
+          && ((nid == OBJ_ln2nid(value)) == NID_undef)) {


First == ~~aught~~ ought to be typo.

randombit · 2018-06-12T21:38:24Z

@dot-asm Thanks, your comments should be addressed by 8af1a2c

dot-asm · 2018-06-12T21:52:08Z

crypto/sm2/sm2_pmeth.c

+        return 0;
+    }
+
+    ret = sm2_sign(type, tbs, tbslen, sig, &sltmp, ec);


type makes no sense to me. I mean it's assigned to NID_sm3 or whatever nid dctx->md has. Then it's passed to sm2_sign where it's checked for being NID_sm3 and not used in any other way. If sm3 is the only one permitted, I'd say check should be performed earlier...

I think the final consensus (in discussion in the original SM2 PR) was that SM2 could be used with any "approved" hash. And while it so happens that currently the only approved hash is SM3, SM2 should not make this restriction eg in the event that the Chinese standard is later amended to also accept say SM2 with SHA-256. Also I see GmSSL has OIDs defined for SM2 signatures with SHA-2

So I think the correct action is to remove the check for SM3 and accept signing with any digest. @ronaldtse any thoughts here?

Checking for type in function that accepts already generated value is not right spot. Check should be done at attempt to generated the value. There are more inconsistencies in code. On one hand it's asserted that it can be used only with given hash algorithm and given curve, but at the same time ctrl accepts practically any curve, multiple digests,,,

I agree with @randombit that we already had that discussion in the original PR and the conclusion was that we should support any approved hash algorithm.

GB/T 33560 explicitly provides OIDs for SM2+SM3 and SM2+SHA1 combinations, so that strongly indicates there are possibly additional approved hash algorithms in the future.

OK I have pushed f481ccfe304645 which removes the restriction on using SM3 or a 256-bit digest with SM2 signatures. I generated a test signature using GmSSL with SHA-512 hash and added it to the evp tests.

dot-asm · 2018-06-12T22:11:43Z

crypto/sm2/sm2_pmeth.c

+            && md_nid != NID_sha512
+            && md_nid != NID_sm3) {
+            SM2err(SM2_F_PKEY_SM2_CTRL, SM2_R_INVALID_DIGEST_TYPE);
+            return 0;


Wouldn't it be appropriate to reject all but sm3 here? If not, then question would also be following, Check for nid in e.g. sm2_sign reads if (type != NID_sm3 || dgstlen != 32), so how does latter check for length align with say sha512 being permitted here? Then it's not like you enforce sm2 curve, so another curve would be able to handle it? But then check for length it not right... Kind of a mess...

Same here about SM3 but the remaining comment applies. Maybe we need a more generic approach here with [hash, length]?

dot-asm · 2018-06-13T08:28:22Z

crypto/evp/pmeth_lib.c

@@ -104,7 +107,8 @@ static EVP_PKEY_CTX *int_ctx_new(EVP_PKEY *pkey, ENGINE *e, int id)
    if (id == -1) {
        if (!pkey || !pkey->ameth)
            return NULL;
-        id = pkey->ameth->pkey_id;
+
+        id = pkey->type;


While you are at it, could you replace first logical ! with explicit NULL check and omit second one. If It would be also appropriate to add empty line prior outermost if... Actually one can wonder if one needs the NULL check at all... I mean if subroutine ends up being called with pkey being NULL and id being -1, then can we view it as programmer's mistake (or memory corruption)? Because if we can, then it would be appropriate to omit the check.

dot-asm · 2018-06-13T08:29:24Z

crypto/sm2/sm2_pmeth.c

+
+static int pkey_sm2_decrypt(EVP_PKEY_CTX *ctx,
+                              unsigned char *out, size_t *outlen,
+                              const unsigned char *in, size_t inlen)


Formatting nit, two too many spaces in above two lines.

dot-asm · 2018-06-13T08:30:09Z

crypto/sm2/sm2_pmeth.c

+    case EVP_PKEY_CTRL_MD: {
+        dctx->md = p2;
+        return 1;
+    }


dot-asm · 2018-06-13T08:30:36Z

crypto/sm2/sm2_pmeth.c

+}
+
+static int pkey_sm2_ctrl_str(EVP_PKEY_CTX *ctx,
+                            const char *type, const char *value)


Formatting nit, one space short in above line.

dot-asm · 2018-06-13T08:32:14Z

crypto/sm2/sm2_pmeth.c

+                            const char *type, const char *value)
+{
+    if (strcmp(type, "ec_paramgen_curve") == 0) {
+       int nid = NID_undef;


Formatting nit, one space short in above line. As well as in line below it. And following two lines are two spaces short...

dot-asm · 2018-06-13T08:37:34Z

I'd like to suggest to remaster this request into two commits. One that adds EVP_PKEY_set_alias_type (code and doc), and modifies int_ctx_new, and second commit that does the rest.

randombit · 2018-06-13T16:10:15Z

@dot-asm Splitting into 2 commits as you suggest seems good to me. I will do this once I have a final all-clear on the changes.

mattcaswell · 2018-06-18T17:25:07Z

Manually edit evperr.h and openssl.txt to resolve the clash. Just pick the next available number.

dot-asm · 2018-06-18T17:52:51Z

Looks like time to remaster as two commits, as suggested before.

randombit · 2018-06-18T18:36:09Z

Rebased against master and converted into two commits, one for EVP_PKEY_set_alias_type and other for SM2 specific changes.

dot-asm · 2018-06-18T19:25:33Z

Red cross from travis is relevant. mdebug errors from evp_extra_test. As for commits' remaster, I was hoping to see that first commit doesn't define new SM2 errors. Maybe to tricky to fix? I should work by copying just updated p_lib.c and running make update...

Use EVP_PKEY_set_alias_type to access

randombit · 2018-06-18T19:54:22Z

Memory leaks in new EVP tests fixed, and I also got the SM2 errors out of the first commit.

dot-asm · 2018-06-18T20:12:08Z

Cool, references to deleted functions are gone from sm2_err.c. :-)

dot-asm

Need re-approvals from others.

romen · 2018-06-18T22:56:46Z

So IIUC, once this PR gets merged into master, it will be possible from a program to explicitly use EVP_PKEY_set_alias() to force an existing EC EVP_PKEY to use the SM2 PKEY_METH, basically making it possible for a purposefully made program to perform SM2DSA (or encryption) over any EC curve, including the recommended SM2 curve. Is this correct?

But, how can a generic application reading a PEM file containing the private key decide to use SM2DSA over ECDSA?
For example: when using the apps/digest.c app to sign something, IIUC, after this PR upon reading an EC private key on the SM2 recommended curve from the specified PEM file will actually do ECDSA over that curve, rather than SM2DSA.
Other example: test/evp_test.c, lines 2534+ force keys over the SM2 recommended curve to use the SM2 pmeth using EVP_PKEY_set_alias(). As a result it is not possible to write evp_tests to test ECDSA over the SM2 recommended curve, nor to test SM2DSA signatures over NIST-P256 or any other curve (excluding the SM2 recommended curve).

Additionally, SM2DSA requires the userid/ZA/personalization field (e.g., "alice@yahoo.com") to generate a signature, but I don't see a way to provide such input through the EVP API, and this is further exemplified by the fact that none of the tests using the EVP API (not in test/recipes/30-test_evp_data/evppkey.txt nor in test/evp_extra_test.c) is feeding it as an input value.

randombit · 2018-06-18T23:35:57Z

But, how can a generic application reading a PEM file containing the private key decide to use SM2DSA over ECDSA?

It cannot, because SM2 uses the same key format as ECDSA and ECDH (and ECIES, ECGDSA, ECKCDSA, ...) and there is no way from looking at the bits to tell which algorithm is intended beyond "an EC algorithm of some kind".

IMO that's fine because a generic application probably should not be using off-brand EC signatures without a very conscious decision to support such things on the part of the developer.

As a result it is not possible to write evp_tests to test ECDSA over the SM2 recommended curve, nor to test SM2DSA signatures over NIST-P256 or any other curve (excluding the SM2 recommended curve).

This is not quite right - in the EVP tests if the key is given a name including substring "SM2" then SM2 is used. It has nothing to do with the key params. So there is no problem testing ECDSA with SM2 curve, or SM2 with P-256. (On current master it is true that SM2 the algo and SM2 the curve are tied together in away that prevents the usages you suggest.)

Additionally, SM2DSA requires the userid/ZA/personalization field

Indeed. I think this functionally got lost when the SM2 API functions were made private. However it seems GmSSL also allows creating signatures without the field (demonstrable by the fact that the EVP test data, which I generated with GmSSL command line util, does not use the personalization functionality). But, end users will likely want this. Probably this could be exposed via an EVP_PKEY_ctrl.

In the interests of making forward progress I would prefer having this PR (addressing #5924) merged and then come back with another PR adding a ctrl for setting the personalization string. But of course I defer to openssl team on that call.

mattcaswell · 2018-06-19T07:34:05Z

But, how can a generic application reading a PEM file containing the private key decide to use SM2DSA over ECDSA?

It cannot, because SM2 uses the same key format as ECDSA and ECDH (and ECIES, ECGDSA, ECKCDSA, ...) and there is no way from looking at the bits to tell which algorithm is intended beyond "an EC algorithm of some kind".

IMO that's fine because a generic application probably should not be using off-brand EC signatures without a very conscious decision to support such things on the part of the developer.

I agree. We could imagine some command line flag which signals that a different alias should be used ("-alias" perhaps?). But certainly not for this PR.

In the interests of making forward progress I would prefer having this PR (addressing #5924) merged and then come back with another PR adding a ctrl for setting the personalization string. But of course I defer to openssl team on that call.

I agree.

romen · 2018-06-19T09:57:04Z

Thanks @randombit for clarifying!

@mattcaswell yes, I too think it makes sense, I just wanted to make sure I correctly understood the current limits/caveats of the SM2 pmeth, and raise a flag in case some of them required to be addressed before merging!

mattcaswell

Sorry...I spotted one other issue! Hopefully this is the last one!

mattcaswell · 2018-06-19T09:57:56Z

crypto/sm2/sm2_pmeth.c

+    sctx = src->data;
+    dctx = dst->data;
+    if (sctx->gen_group != NULL) {
+        dctx->gen_group = EC_GROUP_dup(sctx->gen_group);


Shouldn't we free any pre-existing dctx->gen_group first?

dctx->gen_group is guaranteed to be NULL after pkey_sm2_init. This however ask for question if pkey_sm2_init should see if ~~there~~ ctx->data is not NULL. pkey_ec_init in ec_meth.c doesn't, so at least pkey_sm2_init [is] consistent with that.

Ah...I was thinking that dst may not be a newly allocated structure. But I now see that this function is only ever called as part of an EVP_PKEY_CTX_dup() call, which guarantees that dctx->data will be NULL, so this is a non-issue.

mattcaswell · 2018-06-19T10:26:49Z

I will merge shortly.

Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #6443)

Use EVP_PKEY_set_alias_type to access Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #6443)

mattcaswell · 2018-06-19T10:36:53Z

Pushed!! Thanks @randombit! Will you follow up with a PR to handle the personalisation string? We should also consider whether we want any changes to the command line apps

randombit · 2018-06-19T13:29:10Z

@mattcaswell No problem, probably have something by end of this week for setting personalization string.

ronaldtse · 2018-06-28T02:00:50Z

Thank you @randombit @mattcaswell ! 👍

randombit mentioned this pull request Jun 8, 2018

SM2: add ec_algo_set param to choose which EC algos to use #6402

Closed

mattcaswell requested changes Jun 11, 2018

View reviewed changes

stevelinsell reviewed Jun 11, 2018

View reviewed changes

mattcaswell requested changes Jun 12, 2018

View reviewed changes

mattcaswell approved these changes Jun 12, 2018

View reviewed changes

mattcaswell added the approval: review pending This pull request needs review by a committer label Jun 12, 2018

mattcaswell self-assigned this Jun 12, 2018

richsalz suggested changes Jun 12, 2018

View reviewed changes

richsalz approved these changes Jun 12, 2018

View reviewed changes

randombit force-pushed the sm2-pkey-alias branch from 19c2a45 to c87e933 Compare June 12, 2018 19:15

dot-asm reviewed Jun 12, 2018

View reviewed changes

dot-asm reviewed Jun 13, 2018

View reviewed changes

crypto/sm2/sm2_pmeth.c Outdated

case EVP_PKEY_CTRL_MD: {

dctx->md = p2;

return 1;

}

Copy link

Contributor

dot-asm Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omit {}.

dot-asm reviewed Jun 13, 2018

View reviewed changes

randombit force-pushed the sm2-pkey-alias branch from 55aa962 to 6eda39d Compare June 18, 2018 18:35

randombit force-pushed the sm2-pkey-alias branch from 6eda39d to 8382e2e Compare June 18, 2018 18:37

Jack Lloyd added 2 commits June 18, 2018 15:53

Add EVP_PKEY_set_alias_type

e3dbe15

Move SM2 algos to SM2 specific PKEY method

e9395b3

Use EVP_PKEY_set_alias_type to access

randombit force-pushed the sm2-pkey-alias branch from 96fb20b to e9395b3 Compare June 18, 2018 19:53

dot-asm approved these changes Jun 18, 2018

View reviewed changes

mattcaswell requested changes Jun 19, 2018

View reviewed changes

mattcaswell approved these changes Jun 19, 2018

View reviewed changes

mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jun 19, 2018

levitte pushed a commit that referenced this pull request Jun 19, 2018

Add EVP_PKEY_set_alias_type

2f2e6b6

Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #6443)

levitte pushed a commit that referenced this pull request Jun 19, 2018

Move SM2 algos to SM2 specific PKEY method

ddb634f

Use EVP_PKEY_set_alias_type to access Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #6443)

mattcaswell closed this Jun 19, 2018

This was referenced Jun 20, 2018

Refactor SM2 implementation to use its own PMETH #5924

Closed

Support personalization string in SM2 signatures #6534

Closed

mattcaswell mentioned this pull request Jul 31, 2018

Fix potential NULL pointer dereference in EVP "int_ctx_new". #6623

Closed


		=head1 COPYRIGHT

		Copyright 2006-2018 The OpenSSL Project Authors. All Rights Reserved.

Move SM2 to its own PMETH #6443

Move SM2 to its own PMETH #6443

Conversation

randombit commented Jun 8, 2018 • edited

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

randombit commented Jun 11, 2018

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

randombit commented Jun 12, 2018

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

randombit commented Jun 12, 2018

richsalz commented Jun 12, 2018

richsalz left a comment

Choose a reason for hiding this comment

randombit commented Jun 12, 2018

richsalz commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm Jun 12, 2018 • edited

Choose a reason for hiding this comment

randombit commented Jun 12, 2018

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 Jun 13, 2018 • 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

dot-asm commented Jun 13, 2018

randombit commented Jun 13, 2018

mattcaswell commented Jun 18, 2018

dot-asm commented Jun 18, 2018

randombit commented Jun 8, 2018 •

edited

dot-asm Jun 12, 2018 •

edited

dot-asm Jun 13, 2018 •

edited

randombit commented Jun 18, 2018 •

edited