An alternative to address the SM2 ID issues #7113

InfoHunter · 2018-09-04T13:14:31Z

This is an alternative to #6757 , to fix some SM2 ID relevant issues. Comments are very appreciated.

The idea in this PR is:

the ID in SM2 is a "Distinguishing Identifier" defined as "OCTET STRING" instead of ASCII style string. So this PR implements the ID as a pointer-length style instead of assuming it as a null-terminated one.
Rename some variables to match the only official English SM2 standard - "ISO/IEC-14888".
Don't bother with the default ID '1234567812345678' anymore, that should be the responsibility of the consumer of SM2, not the SM2 low level algorithm.
Make it more flexible for the null-length ID, since the base SM2 algorithm makes this thing very unclear. Although GM/T 0009-2012 defines the default ID, but for the algorithm itself, no specification says null-length ID in invalid.
Avoid the 'two-calls' to EVP_DigestSign/VerifyInit. Introduce a method to pass an EVP_PKEY_CTX to MD and make it configurable before calling EVP_DigestSign/VerifyInit, so the current sequence of calls is something as follows:

/* obtain an EVP_PKEY from whatever methods... */
EVP_PKEY_set_alias_type(pkey, EVP_PKEY_SM2);
mctx = EVP_MD_CTX_new();
pctx = EVP_PKEY_CTX_new(pkey, NULL);
EVP_PKEY_CTX_set_sm2_id(pctx, id, id_len);
EVP_MD_CTX_set_pkey_ctx(mctx, pctx);;
EVP_DigestVerifyInit(mctx, NULL, EVP_sm3(), NULL, pkey);
EVP_DigestVerifyUpdate(mctx, msg, msg_len);
EVP_DigestVerifyFinal(mctx, sig, sig_len)

Document is not updated and test cases are not complete, if this concept is accepted then it should be very easy to complete them.

I borrowed some test code from #6757 to test the correctness of the ID involved signature verification process.

Actually there is another much simpler way of doing this, by using the EVP_ctrl stuffs to set the ID and compute the 'Z' digest after the calls to EVP_DigestSign/VerifyInit. But that will cut the whole algorithm into separated parts so I finally abandoned that idea.

Checklist

documentation is added or updated
tests are added or updated

InfoHunter · 2018-09-04T13:44:24Z

Resolving the travis failures

mattcaswell

Looks good. My initial review comments below. Also if I understood you correctly some of the test stuff was lifted from the other PR. Can that bit be in a separate commit with appropriate git author attribution?

mattcaswell · 2018-09-04T13:31:37Z

crypto/sm2/sm2_pmeth.c

@@ -22,28 +22,35 @@ typedef struct {
    EC_GROUP *gen_group;
    /* message digest */
    const EVP_MD *md;
+    /* Distinguishing Identifier, ISO/IEC 15946-3 */
+    uint8_t *id;
+    int id_len;


Probably this should be size_t

mattcaswell · 2018-09-04T13:34:21Z

crypto/sm2/sm2_pmeth.c

        SM2err(SM2_F_PKEY_SM2_INIT, ERR_R_MALLOC_FAILURE);
        return 0;
    }

-    ctx->data = dctx;
+    /* -1 indicates there is no ID set yet */
+    smctx->id_len = -1;


...but this is problematic if it were to be size_t. I still think it should be though...I had too much pain sorting out the "it's a size_t, no it's a long, no it's an int" mess that we used to have in libssl (casts everywhere).

Actually I used size_t initially and changed that to int in 7659f41. Because without the -1 value, it can distinguish if the ID is set or not, any ideas on this?

Perhaps just add an additional flag into the ctx?

Perhaps just add an additional flag into the ctx?

That is workable

mattcaswell · 2018-09-04T13:37:43Z

crypto/sm2/sm2_pmeth.c

+            return 0;
+        }
+        memcpy(dctx->id, sctx->id, sctx->id_len);
+        dctx->id_len = sctx->id_len;


Probably you need to copy id_len unconditionally to pick up the -1 initialisation

mattcaswell · 2018-09-04T13:41:01Z

crypto/sm2/sm2_pmeth.c

+        *(const EVP_MD **)p2 = smctx->md;
+        return 1;
+
+    case EVP_PKEY_CTRL_SM2_SET_ID:


Probably this should be ...SET1_ID?

I am a little confused with the number after get/set, so 1 here stands for the caller could free the passed-in parameter at his will?

Yes. The number is supposed to give you a hint about the memory management:

get0 == returns a pointer to the internal data. The caller should therefore not free it
get1 == creates a copy of the data or ups a ref count. The caller should free.
set0 == the caller passes ownership of the pointer to the library, so the caller should not subsequently free it
set1 == the caller does not pass ownership of the pointer to the library. The library will take a copy or up a ref count. The caller should free the original pointer.

Understood, thanks for the clear explanation.

mattcaswell · 2018-09-04T13:45:44Z

crypto/sm2/sm2_pmeth.c

+        /*
+         * If no ID is set, reject to compute the 'Z' value. This allows to
+         * set a null-ID to SM2, although which is not clear in the relevant
+         * specifications. We implement as this to stay flexibility.


Perhaps reword as: "An ID value must be set. The specifications are not clear whether a NULL is allowed. We only allow it if set explicitly for maximum flexibility"

mattcaswell · 2018-09-04T13:50:39Z

include/openssl/ec.h

@@ -1429,6 +1429,19 @@ void EC_KEY_METHOD_get_verify(const EC_KEY_METHOD *meth,
                                EVP_PKEY_OP_DERIVE, \
                                EVP_PKEY_CTRL_GET_EC_KDF_UKM, 0, (void *)(p))

+/* SM2 will skip the operation check so no need to pass operation here */
+# define EVP_PKEY_CTX_set_sm2_id(ctx, id, id_len) \


...set1_sm2_id?

Actually, I wonder whether we should lose the sm2 bit altogether. This seems like a fairly general concept that might be needed for other algs in the future. Similarly for the macros below.

There should be no sm2 in the name ...

But how to deal with the EVP_PKEY_SM2 parameter in EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_SM2, ...?

Perhaps set that to -1?

Yes, that is what is done elsewhere.

mattcaswell · 2018-09-04T13:51:20Z

include/openssl/evp.h

@@ -181,6 +181,9 @@ int (*EVP_MD_meth_get_ctrl(const EVP_MD *md))(EVP_MD_CTX *ctx, int cmd,
 */
 # define EVP_MD_CTX_FLAG_FINALISE        0x0200

+/* Don't free up ctx->pctx in EVP_MD_CTX_reset */
+# define EVP_MD_CTX_FLAG_NEGLECT_PCTX    0x0400


This feels misplaced in the public header - although I understand why you put it there.

It should also have a different name - it is do-not-free and neglect isn't the right name to use.
Perhaps EVP_MD_CTX_FLAG_KEEP_PKEY_CTX ... I'd also not abbreviate to PCTX ...

mattcaswell · 2018-09-04T13:51:55Z

include/openssl/evp.h

@@ -1321,6 +1325,8 @@ void EVP_PKEY_asn1_set_security_bits(EVP_PKEY_ASN1_METHOD *ameth,
 * Method handles all operations: don't assume any digest related defaults.
 */
 # define EVP_PKEY_FLAG_SIGCTX_CUSTOM     4
+/* Do a customized hashing process */
+# define EVP_PKEY_FLAG_DIGEST_CUSTOM     8


Similarly this seems misplaced.

Hmm, if this is placed in an internal header file, then the value of the flag should be chosen safe...

I'd suggest a comment in the public header stating that the value 8 is reserved for internal use. In the internal header file where we put the definition we add a reference to the public header values.

Why is this flag even needed? If there's a non-NULL digest_custom in a EVP_PKEY_METHOD, then it has a digest custom function... isn't that enough?
(I know that we have sometimes used flags to tell if certain pointers are defined or not, but that's from the time when all structures were public, so flagging was done to allow for a certain level of ABI backward compatibility, to tell if added fields could be relied upon. Since EVP_PKEY_METHOD is opaque, this methodology is no longer needed)

mattcaswell · 2018-09-04T13:56:04Z

test/sm2_internal_test.c

@@ -294,7 +294,8 @@ static int test_sm2_sign(const EC_GROUP *group,
        goto done;

    start_fake_rand(k_hex);
-    sig = sm2_do_sign(key, EVP_sm3(), userid, (const uint8_t *)message, msg_len);
+    sig = sm2_do_sign(key, EVP_sm3(), (const uint8_t *)userid, (int)strlen(userid),


If the type were size_t the int cast would not be needed here.

mattcaswell · 2018-09-04T13:56:20Z

test/sm2_internal_test.c

-    ok = sm2_do_verify(key, EVP_sm3(), sig, userid, (const uint8_t *)message,
-                       msg_len);
+    ok = sm2_do_verify(key, EVP_sm3(), sig, (const uint8_t *)userid,
+                       (int)strlen(userid), (const uint8_t *)message, msg_len);


...and here

InfoHunter · 2018-09-04T14:17:10Z

Also if I understood you correctly some of the test stuff was lifted from the other PR. Can that bit be in a separate commit with appropriate git author attribution?

Makes sense, will do that.

levitte · 2018-09-04T16:22:32Z

include/openssl/evp.h

@@ -1321,6 +1325,8 @@ void EVP_PKEY_asn1_set_security_bits(EVP_PKEY_ASN1_METHOD *ameth,
 * Method handles all operations: don't assume any digest related defaults.
 */
 # define EVP_PKEY_FLAG_SIGCTX_CUSTOM     4
+/* Do a customized hashing process */
+# define EVP_PKEY_FLAG_DIGEST_CUSTOM     8


Why is this flag even needed? If there's a non-NULL digest_custom in a EVP_PKEY_METHOD, then it has a digest custom function... isn't that enough?
(I know that we have sometimes used flags to tell if certain pointers are defined or not, but that's from the time when all structures were public, so flagging was done to allow for a certain level of ABI backward compatibility, to tell if added fields could be relied upon. Since EVP_PKEY_METHOD is opaque, this methodology is no longer needed)

levitte · 2018-09-04T16:23:56Z

crypto/evp/digest.c

-    EVP_PKEY_CTX_free(ctx->pctx);
+    /*
+     * pctx should be freed by the user of EVP_MD_CTX
+     * if EVP_MD_CTX_FLAG_NEGLECT_PCTX is set


This comment needs editing, since that flag doesn't exist.

levitte · 2018-09-04T16:32:52Z

crypto/evp/m_sigver.c

@@ -75,6 +75,14 @@ static int do_sigver_init(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
        return 1;
    if (!EVP_DigestInit_ex(ctx, type, e))
        return 0;
+    if (ctx->pctx->pmeth->flags & EVP_PKEY_FLAG_DIGEST_CUSTOM) {


EVP_PKEY_FLAG_DIGEST_CUSTOM is unnecessary, it should be enough to check that digest_custom is non-NULL

Ahh, makes sense

levitte · 2018-09-04T16:33:21Z

crypto/evp/pmeth_lib.c

    if (!ctx || !ctx->pmeth || !ctx->pmeth->ctrl) {
        EVPerr(EVP_F_EVP_PKEY_CTX_CTRL, EVP_R_COMMAND_NOT_SUPPORTED);
        return -2;
    }
    if ((keytype != -1) && (ctx->pmeth->pkey_id != keytype))
        return -1;

+    /* Skip the operation checks since this is called in a very early stage */
+    if (ctx->pmeth->flags & EVP_PKEY_FLAG_DIGEST_CUSTOM)


Check that digest_custom is non-NULL instead

levitte · 2018-09-04T16:34:13Z

crypto/include/internal/evp_int.h

@@ -10,6 +10,14 @@
 #include <openssl/evp.h>
 #include "internal/refcount.h"

+/* Do a customized hashing process, use the reserved flag value in evp.h */
+#define EVP_PKEY_FLAG_DIGEST_CUSTOM     8


As I argued before, not necessary, just throw it away

levitte · 2018-09-04T16:35:03Z

include/openssl/evp.h

@@ -1321,6 +1323,7 @@ void EVP_PKEY_asn1_set_security_bits(EVP_PKEY_ASN1_METHOD *ameth,
 * Method handles all operations: don't assume any digest related defaults.
 */
 # define EVP_PKEY_FLAG_SIGCTX_CUSTOM     4
+/* NOTE: 8 is reserved for internal usage in evp_int.h */


Remove this line

InfoHunter · 2018-09-05T07:20:29Z

Document is updated to reflect the modification in this PR

dot-asm · 2018-09-05T08:52:10Z

crypto/evp/pmeth_lib.c

+                                     int (**pdigest_custom) (EVP_PKEY_CTX *ctx,
+                                                             EVP_MD_CTX *mctx))
+{
+    if (*pdigest_custom)


So that void *foo(EVP_PKEY_METHOD *pmeth) { void *p; EVP_PKEY_meth_get_digest_custom(pmeth, &p); return p; } might return sensible or garbage value. But even if corrected, the check is arguably inappropriate. In sense that it's more appropriate to punish caller with crash if programmer didn't care to arrange proper location that would receive the value, than to let caller believe that everything is fine. "Is fine" means that there is no way of telling it's not fine.

The remark applies to multiple cases. [Which in turn effectively means review process failure.]

These checks exist in other (not all???) method function getters, but are implemented differently in some. I don't think it's a fix to do in this PR, but it would be good to clean this up.

In my opinion, the check should be this:

if (pdigest_custom)

I agree to the form that Richard suggested

I don't think it's a fix to do in this PR,

So suggestion is to perpetuate bugs?

but it would be good to clean this up.

??? "Good" to "clean up"? It's not something that would be "good to do". They are bugs that has to be fixed.

the check should be this:

It's equivalent to demanding NULL-check in memcpy(NULL, p, 1).

I opened an issue (#7130) to keep track of this.

levitte · 2018-09-05T09:05:45Z

doc/man3/EVP_PKEY_meth_new.pod

@@ -334,6 +342,15 @@ key-pair, the public component and parameters respectively for a given B<pkey>.
 They could be called by L<EVP_PKEY_check(3)>, L<EVP_PKEY_public_check(3)> and
 L<EVP_PKEY_param_check(3)> respectively.

+ int (*digest_custom) (EVP_PKEY_CTX *ctx, EVP_MD_CTX *mctx);
+
+The digest_custom() method is used to generate customzied degest content before


s/degest/digest/

levitte · 2018-09-05T09:09:01Z

Looking at this had me stumble on stuff I haven't looked at before, the custom signctx functions... and the similarity with what digest_custom is used for is striking! That has me wonder if digest_custom reimplements something that's already there, just in sliiiiightly different form. @InfoHunter, is this something you have explored?

mattcaswell · 2018-09-05T08:48:02Z

crypto/evp/evp_lib.c

@@ -460,6 +460,13 @@ EVP_PKEY_CTX *EVP_MD_CTX_pkey_ctx(const EVP_MD_CTX *ctx)
    return ctx->pctx;
 }

+void EVP_MD_CTX_set_pkey_ctx(EVP_MD_CTX *ctx, EVP_PKEY_CTX *pctx)


Can this function be called with a NULL pctx (e.g. say if you want to clear one that was previously set)?
What should happen if ctx->pctx is not NULL when this function is called?

Good question. TBH I didn't consider the 'set to NULL' or the 'clear' scenario. But after a quick thinking, I think it would be useful to allow users to set ctx->pctx to NULL (for instance someone wants to reuse the MD_CTX). So the current implementation supports that already.

If ctx->pctx is not NULL when this function is called, then I think it should be the user's responsibility to free the old pctx attached in MD_CTX, makes sense?

If ctx->pctx is not NULL when this function is called, then I think it should be the user's responsibility to free the old pctx attached in MD_CTX, makes sense?

Yes, if EVP_MD_CTX_FLAG_KEEP_PKEY_CTX is set - but not otherwise.

Ahh, nah, the flag should also be cleared if the ctx->pctx is set to NULL if someone wants to reuse the MD_CTX. So I think there should be a separated EVP_MD_CTX_clr_pkey_ctx` function to do this?

Or we could mix the set/clr behavior into one function by doing something like:

if (pctx == NULL) { /* clear the flag... */ }

Hmm..I'm not sure that is necessary. Surely all we need is something like this:

if (!EVP_MD_CTX_test_flags(ctx, EVP_MD_CTX_FLAG_KEEP_PKEY_CTX)) EVP_PKEY_CTX_free(ctx->pctx); ctx->pctx = pctx; if (pctx != NULL) EVP_MD_CTX_set_flags(ctx, EVP_MD_CTX_FLAG_KEEP_PKEY_CTX); else EVP_MD_CTX_clear_flags(out, EVP_MD_CTX_FLAG_KEEP_PKEY_CTX);

Okay, I get the point - It's complicated to deal with such stuffs: one pointer holds two different memory management strategy objects...

In my experience, another way is to have two pointers containing system-created and user-created objects respectively - if the user-created on is used, the the system one will always be NULL. Thus it would make it simpler. But this time let's just mix them together

mattcaswell · 2018-09-05T08:48:37Z

crypto/evp/m_sigver.c

+     * This indicates the current algorithm requires
+     * special treatment before hashing the tbs-message.
+     */
+    if (ctx->pctx->pmeth->digest_custom)


Add != NULL

mattcaswell · 2018-09-05T08:50:22Z

crypto/evp/pmeth_lib.c

+                                     int (**pdigest_custom) (EVP_PKEY_CTX *ctx,
+                                                             EVP_MD_CTX *mctx))
+{
+    if (*pdigest_custom)


mattcaswell · 2018-09-05T08:52:07Z

crypto/sm2/sm2_pmeth.c

+    case EVP_PKEY_CTRL_SET1_ID:
+        OPENSSL_free(smctx->id);
+        if (p1 > 0) {
+            smctx->id = OPENSSL_malloc(p1);


Should we free any pre-existing id?

Ahh, you mean if malloc fails, then we should not touch the original previous set smctx->id?

No I mean if there was a pre-existing id then we just introduced a mem leak because we didn't free the old one. So I think you need to add OPENSSL_free(smctx->id); before the malloc.

It seems that is already there. But I improved the logical to defer OPNESSL_free to a more suitable position in the new version of this function.

Duh...why did I not see that?

mattcaswell · 2018-09-05T08:58:10Z

doc/man3/EVP_DigestSignInit.pod

@@ -31,6 +31,10 @@ B<*pctx> is overwritten. The EVP_PKEY_CTX value returned must not be freed
 directly by the application (it will be freed automatically when the EVP_MD_CTX
 is freed). The digest B<type> may be NULL if the signing algorithm supports it.

+There is no B<EVP_PKEY_CTX> will be created by EVP_DigestSignInit() if the passed
+B<ctx> is already assigned with an B<EVP_PKEY_CTX>, take L<EVP_MD_CTX_set_pkey_ctx(3)>
+and L<SM2(7)> as references.


"No B<EVP_PKEY_CTX> will be created by EVP_DigsetSignInit() if the passed B<ctx> has already been assigned one via L<EVP_MD_CTX_set_ctx(3)>. See also L<SM2(7)>."

Oh...but wait...there is no SM2(7)!? There probably needs to be something (maybe not in this PR) - otherwise how will people know about the special hoops they need to jump through to be able to use SM2?

In fact I forgot to include that SM2(7)... I used 'git add -u' so the new SM2.pod file is left on my disk...now added

mattcaswell · 2018-09-05T09:04:24Z

doc/man3/EVP_PKEY_CTX_ctrl.pod

+are used to manipulate special identifier field for specific signature algorithm
+such as SM2. The EVP_PKEY_CTX_get1_id_len() returns the length of the ID set in
+algorihtm related context. The length is usually used to allocate adequate memory
+for further calls to EVP_PKEY_CTX_get1_id().


There is no description here of how EVP_PKEY_CTX_set1_id() and EVP_PKEY_get1_id() actually work and what the parameters mean. There should at least be a sentence about the memory management aspects I think.

mattcaswell · 2018-09-05T09:04:57Z

doc/man3/EVP_PKEY_CTX_ctrl.pod

@@ -179,7 +191,8 @@ L<EVP_PKEY_keygen(3)>

 =head1 HISTORY

-These functions were first added to OpenSSL 1.0.0.
+EVP_PKEY_CTX_set1_id(), EVP_PKEY_CTX_get1_id() and EVP_PKEY_CTX_get1_id_len() are
+added in 1.1.1, other functions were first added to OpenSSL 1.0.0.


s/are added/were added/

mattcaswell · 2018-09-05T09:05:18Z

doc/man3/EVP_PKEY_meth_new.pod

@@ -334,6 +342,15 @@ key-pair, the public component and parameters respectively for a given B<pkey>.
 They could be called by L<EVP_PKEY_check(3)>, L<EVP_PKEY_public_check(3)> and
 L<EVP_PKEY_param_check(3)> respectively.

+ int (*digest_custom) (EVP_PKEY_CTX *ctx, EVP_MD_CTX *mctx);
+
+The digest_custom() method is used to generate customzied degest content before


typo: customized

mattcaswell · 2018-09-05T09:05:56Z

doc/man3/EVP_PKEY_meth_new.pod

+the real message is passed to functions like L<EVP_DigestSignUpdate(3)> or
+L<EVP_DigestVerifyInit(3)>. This is usually required by some public key
+signature algorithms like SM2 which requires a hashed prefix to the message to
+be signed. The digest_custom() could be called by L<EVP_DigestSignInit(3)> and


s/could/will/ ?

mattcaswell · 2018-09-05T09:10:46Z

crypto/sm2/sm2_pmeth.c

+         * NULL is allowed. We only allow it if set explicitly for maximum
+         * flexibility.
+         */
+        return 0;


Probably there needs to be a call to SM2err here.

InfoHunter · 2018-09-05T11:51:53Z

That has me wonder if digest_custom reimplements something that's already there, just in sliiiiightly different form. @InfoHunter, is this something you have explored?

The real difference is the position of triggering for signctx (or other function pointers) and digest_custom. Actually we need a calling point right after the 'digest' is initialized and before the 'DigestSign/Verify' is initialized. There is no such thing yet, which makes it's very hard to be fit in some scenarios...

InfoHunter · 2018-09-05T12:21:28Z

I forgot to include a man(7) page of SM2, now updated...

levitte · 2018-09-05T12:24:17Z

The real difference is the position of triggering for signctx (or other function pointers) and digest_custom. Actually we need a calling point right after the 'digest' is initialized and before the 'DigestSign/Verify' is initialized. There is no such thing yet, which makes it's very hard to be fit in some scenarios...

Good, so you looked at least. I gotta admit, signctx is new to me, and seems to have multiple applications (depending on the flag EVP_PKEY_FLAG_SIGCTX_CUSTOM); I haven't quite wrapped my head around the differences.

It does worry me, though, that we seems to add more hooks for almost, but not quite the same the same things. I guess that's a future clean-up job 😉

InfoHunter · 2018-09-05T13:14:53Z

It does worry me, though, that we seems to add more hooks for almost, but not quite the same the same things. I guess that's a future clean-up job

Right. The concept behind this is the 'calling points' for these hooks should be considered and defined carefully and precisely. A very challenging job to design a decent framework as current EVP...

t-j-h · 2018-09-06T00:41:54Z

crypto/sm2/sm2_pmeth.c

+    if (sctx->id != NULL) {
+        dctx->id = OPENSSL_malloc(sctx->id_len);
+        if (dctx->id == NULL) {
+            pkey_sm2_cleanup(dst);


This looks like it is missing an SM2err(SM2_F_PKEY_SM2_COPY, ERR_R_MALLOC_FAILURE) call ... for consistency with the other error handling.

t-j-h · 2018-09-06T00:43:12Z

crypto/sm2/sm2_pmeth.c

+        if (p1 > 0) {
+            tmp_id = OPENSSL_malloc(p1);
+            if (tmp_id == NULL)
+                return 0;


This looks like it is missing an SM2err(SM2_F_PKEY_SM2_CTRL, ERR_R_MALLOC_FAILURE) call ... for consistency with the other error handling.

t-j-h

Once you have added the SM2err calls for malloc failures and fixed the typo in the SM2.pod file this is good to go.

t-j-h · 2018-09-06T00:52:44Z

doc/man7/SM2.pod

+
+B<SM2> algorithm is first defined by the Chinese national standard GM/T 0003-2012
+and is standardized by ISO as ISO/IEC 14888. B<SM2> is actually an elliptic curve
+based algorithm. Currnet implementation in OpenSSL supports both signature and


Currnet -> The current

t-j-h

LGTM

InfoHunter · 2018-09-06T03:12:35Z

Oops, a nit is fixed

InfoHunter · 2018-09-06T03:37:38Z

Travis failures are not related. Force a push to re-trigger it...

InfoHunter · 2018-09-06T04:38:06Z

There is adequate approvals for merging this PR. I would squash and cleanup the commits soon.

InfoHunter · 2018-09-06T05:41:53Z

I squashed the commits into proper styles - no actual code modified after the 'ready' label is assigned so I don't think this needs to re-approve for the approval by Tim

mattcaswell

Lots of comments but I think they're all minor nits in the documentation which should be easy to fix. This looks almost ready.

mattcaswell · 2018-09-06T09:16:53Z

doc/man3/EVP_PKEY_CTX_ctrl.pod

@@ -160,6 +166,17 @@ For maximum compatibility the named curve form should be used. Note: the
 B<OPENSSL_EC_NAMED_CURVE> value was only added to OpenSSL 1.1.0; previous
 versions should use 0 instead.

+The EVP_PKEY_CTX_set1_id(), EVP_PKEY_CTX_get1_id() and EVP_PKEY_CTX_get1_id_len()
+are used to manipulate special identifier field for specific signature algorithm


Nit: manipulate the special identifier field
Nit: for specific signature algorithms

mattcaswell · 2018-09-06T09:17:17Z

doc/man3/EVP_DigestInit.pod

+Assigns an B<EVP_PKEY_CTX> to B<EVP_MD_CTX>. This is usually used to provide
+a customzied B<EVP_PKEY_CTX> to L<EVP_DigestSignInit(3)> or
+L<EVP_DigestVerifyInit(3)>. The B<pctx> passed to this function should be freed
+by the caller. A null B<pctx> pointer is also allowed to clear the B<EVP_PKEY_CTX>


Nit: s/null/NULL/

mattcaswell · 2018-09-06T09:23:25Z

doc/man3/EVP_PKEY_CTX_ctrl.pod

+such as SM2. The EVP_PKEY_set1_id() sets an ID pointed by B<id> with the length
+B<id_len> to the library. The library maintains the memory management stuffs so
+the caller can safely free the original memory pointed by B<id>. The
+EVP_PKEY_CTX_get1_id_len() returns the length of the ID set via a previous call


insert the word "macro" before "returns"

mattcaswell · 2018-09-06T09:23:37Z

doc/man3/EVP_PKEY_CTX_ctrl.pod

+B<id_len> to the library. The library maintains the memory management stuffs so
+the caller can safely free the original memory pointed by B<id>. The
+EVP_PKEY_CTX_get1_id_len() returns the length of the ID set via a previous call
+to EVP_PKEY_set1_id(). The length is usually used to allocate adequate memory for


Typo: EVP_PKEY_CTX_set1_id()

mattcaswell · 2018-09-06T09:24:04Z

doc/man3/EVP_PKEY_CTX_ctrl.pod

+the caller can safely free the original memory pointed by B<id>. The
+EVP_PKEY_CTX_get1_id_len() returns the length of the ID set via a previous call
+to EVP_PKEY_set1_id(). The length is usually used to allocate adequate memory for
+further calls to EVP_PKEY_CTX_get1_id(). The EVP_PKEY_CTX_get1_id() returns the


Insert the word "macro" before "returns"

mattcaswell · 2018-09-06T09:32:23Z

doc/man7/SM2.pod

+
+ EVP_MD_CTX_set_pkey_ctx(mctx, pctx);
+
+And normally there is no need to pass a B<pctx> parameter to EVP_DeigestSignInit()


Typo: EVP_DigestSignInit()

mattcaswell · 2018-09-06T09:32:36Z

doc/man7/SM2.pod

+ EVP_MD_CTX_set_pkey_ctx(mctx, pctx);
+
+And normally there is no need to pass a B<pctx> parameter to EVP_DeigestSignInit()
+or EVP_DigestVerifyInit() in such scenario.


such a scenario

mattcaswell · 2018-09-06T09:33:17Z

doc/man7/SM2.pod

+
+=head1 EXAMPLE
+
+This example demonstrates the calling sequence on how to use an B<EVP_PKEY> to


calling sequence for using an B<EVP_PKEY>...

mattcaswell · 2018-09-06T09:34:02Z

doc/man7/SM2.pod

+=head1 EXAMPLE
+
+This example demonstrates the calling sequence on how to use an B<EVP_PKEY> to
+sign a message with SM2 signature algorithm and SM3 hash algorithm:


with the SM2....and the SM3

Perhaps add this sentence. "The example below omits error checking code for clarity. Application code should check the return codes of function calls for errors."

The text above says this is an example for signing. But actually its an example for verifying a signature.

mattcaswell · 2018-09-06T09:34:29Z

doc/man7/SM2.pod

+
+ #include <openssl/evp.h>
+
+ /* obtain an EVP_PKEY from whatever methods... */


using whatever method

ronaldtse · 2018-09-06T09:39:32Z

Sounds like things here moved much, much faster here compared to at #6757 . Our unfamiliarity with OpenSSL internals is apparent. Happy to see this merged with appropriate attribution. Thanks @InfoHunter !

t-j-h · 2018-09-06T09:42:31Z

Approval still stands after @mattcaswell suggested doc changes applied.

InfoHunter · 2018-09-07T05:02:02Z

Doc nits are all fixed

InfoHunter · 2018-09-07T09:35:16Z

@mattcaswell , it seems I could merge this now, right?

mattcaswell

Sorry a few more nits. Approved assuming you fix those.

mattcaswell · 2018-09-07T09:38:49Z

doc/man3/EVP_PKEY_CTX_ctrl.pod

@@ -160,6 +166,17 @@ For maximum compatibility the named curve form should be used. Note: the
 B<OPENSSL_EC_NAMED_CURVE> value was only added to OpenSSL 1.1.0; previous
 versions should use 0 instead.

+The EVP_PKEY_CTX_set1_id(), EVP_PKEY_CTX_get1_id() and EVP_PKEY_CTX_get1_id_len()
+are used to manipulate the special identifier field for specific signature


Insert the word "macros" before "are used"

mattcaswell · 2018-09-07T09:39:06Z

doc/man3/EVP_PKEY_CTX_ctrl.pod

@@ -160,6 +166,17 @@ For maximum compatibility the named curve form should be used. Note: the
 B<OPENSSL_EC_NAMED_CURVE> value was only added to OpenSSL 1.1.0; previous
 versions should use 0 instead.

+The EVP_PKEY_CTX_set1_id(), EVP_PKEY_CTX_get1_id() and EVP_PKEY_CTX_get1_id_len()
+are used to manipulate the special identifier field for specific signature
+algorithms such as SM2. The EVP_PKEY_set1_id() sets an ID pointed by B<id> with


Typo: EVP_PKEY_CTX_set1_id()

mattcaswell · 2018-09-07T09:47:34Z

doc/man3/EVP_PKEY_CTX_ctrl.pod

+are used to manipulate the special identifier field for specific signature
+algorithms such as SM2. The EVP_PKEY_set1_id() sets an ID pointed by B<id> with
+the length B<id_len> to the library. The library maintains the memory management
+stuffs so the caller can safely free the original memory pointed by B<id>. The


Change this to "The library takes a copy of the id so that the caller can safely free the original memory pointed to by B<id>"

mattcaswell · 2018-09-07T09:48:15Z

doc/man3/EVP_PKEY_CTX_ctrl.pod

+EVP_PKEY_CTX_get1_id_len() macro returns the length of the ID set via a previous
+call to EVP_PKEY_CTX_set1_id(). The length is usually used to allocate adequate
+memory for further calls to EVP_PKEY_CTX_get1_id(). The EVP_PKEY_CTX_get1_id()
+macro returns the previously set ID value to caller into B<id>, caller should


to the caller in B<id>. The caller should...

mattcaswell · 2018-09-07T09:49:48Z

doc/man7/SM2.pod

+signature.
+
+The B<EVP_PKEY> structure will default to using ECDSA for signatures when it is
+created. Itshould be set to B<EVP_PKEY_SM2> by calling:


typo: missing space "It should"

mattcaswell · 2018-09-07T09:50:05Z

doc/man7/SM2.pod

+
+ EVP_PKEY_CTX_set1_id(pctx, id, id_len);
+
+When calling the EVP_DeigestSignInit() or EVP_DigestVerifyInit() functions, a


Typo: EVP_DigestSignInit(()

mattcaswell · 2018-09-07T09:50:38Z

doc/man7/SM2.pod

+
+=head1 EXAMPLE
+
+This example demonstrates the calling sequence for using an B<EVP_PKEY> to sign


s/sign/verify/

Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #7113)

Thus users can use this function to set customized EVP_PKEY_CTX to EVP_MD_CTX structure. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #7113)

Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #7113)

zero-length ID is allowed, but it's not allowed to skip the ID. Fixes: #6534 Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #7113)

This test case is originally submitted in #6757, by Jack Lloyd. The test case has been modified to use the a different method to set the ID when computing the Z hash of SM2 signature. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Yang <yang.yang@baishancloud.com> (Merged from #7113)

Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #7113)

InfoHunter · 2018-09-07T10:21:28Z

New nits are fixed, Merged to master from 5bd0abe to f922dac. Closing...

Remove unnecessary sm2_za.c

1edfb1d

mattcaswell requested changes Sep 4, 2018

View reviewed changes

mattcaswell added this to the 1.1.1 milestone Sep 4, 2018

InfoHunter self-assigned this Sep 4, 2018

levitte requested changes Sep 4, 2018

View reviewed changes

dot-asm reviewed Sep 5, 2018

View reviewed changes

levitte requested changes Sep 5, 2018

View reviewed changes

mattcaswell requested changes Sep 5, 2018

View reviewed changes

t-j-h reviewed Sep 6, 2018

View reviewed changes

t-j-h approved these changes Sep 6, 2018

View reviewed changes

t-j-h reviewed Sep 6, 2018

View reviewed changes

t-j-h approved these changes Sep 6, 2018

View reviewed changes

InfoHunter force-pushed the sm2 branch from d3c6845 to 059ad51 Compare September 6, 2018 03:45

t-j-h added approval: done This pull request has the required number of approvals branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch labels Sep 6, 2018

InfoHunter mentioned this pull request Sep 6, 2018

Remove the NULL checks of function parameters #7130

Closed

InfoHunter added 2 commits September 6, 2018 13:39

Add a SM2(7) man page

bf67dbb

Allow EVP_MD_CTX_set_pkey_ctx to accept NULL pctx

261a272

InfoHunter force-pushed the sm2 branch from 059ad51 to d166a5d Compare September 6, 2018 05:40

mattcaswell requested changes Sep 6, 2018

View reviewed changes

t-j-h mentioned this pull request Sep 6, 2018

Add support for setting SM2 id field #6757

Closed

2 tasks

InfoHunter force-pushed the sm2 branch from d166a5d to ed38b2f Compare September 7, 2018 04:44

t-j-h approved these changes Sep 7, 2018

View reviewed changes

mattcaswell approved these changes Sep 7, 2018

View reviewed changes

Add missing SM2err and fix doc nits

670511d

InfoHunter force-pushed the sm2 branch from ed38b2f to 670511d Compare September 7, 2018 10:10

levitte pushed a commit that referenced this pull request Sep 7, 2018

Remove unnecessary sm2_za.c

5bd0abe

Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #7113)

levitte pushed a commit that referenced this pull request Sep 7, 2018

Support pmeth->digest_custom

0a8fdef

Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #7113)

levitte pushed a commit that referenced this pull request Sep 7, 2018

Make SM2 ID stick to specification

00433ba

Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #7113)

levitte pushed a commit that referenced this pull request Sep 7, 2018

Support setting SM2 ID

4803717

zero-length ID is allowed, but it's not allowed to skip the ID. Fixes: #6534 Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #7113)

levitte pushed a commit that referenced this pull request Sep 7, 2018

Update document for SM2 stuffs

675f4ce

Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #7113)

levitte pushed a commit that referenced this pull request Sep 7, 2018

Add a SM2(7) man page

571286b

Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #7113)

levitte pushed a commit that referenced this pull request Sep 7, 2018

Allow EVP_MD_CTX_set_pkey_ctx to accept NULL pctx

81c7945

Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #7113)

levitte pushed a commit that referenced this pull request Sep 7, 2018

Add missing SM2err and fix doc nits

f922dac

Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #7113)

InfoHunter closed this Sep 7, 2018

InfoHunter deleted the sm2 branch September 7, 2018 11:31

InfoHunter mentioned this pull request Jun 28, 2019

Support SM2 certificate signing #9085

Closed

2 tasks

InfoHunter mentioned this pull request Dec 29, 2023

Fix SM2 signature, should set default user id #23162

Closed

2 tasks


		EVP_MD_CTX_set_pkey_ctx(mctx, pctx);

		And normally there is no need to pass a B<pctx> parameter to EVP_DeigestSignInit()


		=head1 EXAMPLE

		This example demonstrates the calling sequence on how to use an B<EVP_PKEY> to


		#include <openssl/evp.h>

		/* obtain an EVP_PKEY from whatever methods... */


		EVP_PKEY_CTX_set1_id(pctx, id, id_len);

		When calling the EVP_DeigestSignInit() or EVP_DigestVerifyInit() functions, a

An alternative to address the SM2 ID issues #7113

An alternative to address the SM2 ID issues #7113

Conversation

InfoHunter commented Sep 4, 2018 • edited Loading

Checklist

InfoHunter commented Sep 4, 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

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

InfoHunter commented Sep 4, 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

InfoHunter commented Sep 5, 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

levitte commented Sep 5, 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

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

InfoHunter commented Sep 5, 2018

InfoHunter commented Sep 5, 2018

levitte commented Sep 5, 2018

InfoHunter commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InfoHunter commented Sep 4, 2018 •

edited

Loading