Fix for a segv in fix_dh_rfc5114 #16913

pmesnier · 2021-10-25T21:51:56Z

I found that when a call stack starting with EVP_PKEY_CTX_set_dhx_rfc5114 is invoked. The function in question, fix_dh_rfc5114() evaluates the value ctx->p2 using atoi(). However in this case p2 is null, triggering a segv interrupt, crashing the program. Further, fix_dh_rfc5114() is called by evp_pkey_ctx_ctrl_to_param() which explicitly sets the state parameter to PRE_CTRL_TO_PARAM but fix_dh_rfc5114() only evaluates the ctx values of p1 and p2 when the supplied state is PRE_CTRL_STR_TO_PARAM. This patch addresses these issues.

Checklist

tests are added or updated

t8m · 2021-10-26T07:53:18Z

crypto/evp/ctrl_params_translate.c

@@ -1026,9 +1026,9 @@ static int fix_dh_nid5114(enum state state,
    if (ctx->action_type != SET)
        return 0;

-    if (state == PRE_CTRL_STR_TO_PARAMS) {
+    if (state == PRE_CTRL_TO_PARAMS || state == PRE_CTRL_STR_TO_PARAMS) {


I would separate the ifs for PRE_CTRL_TO_PARAMS and PRE_CTRL_STR_TO_PARAMS so that the ctx->p1 and ctx->p2 is used only in the appropriate cases.

Thanks for the suggestion. I hadn't made a clear connection between the state values and the p* use.

t8m · 2021-10-26T07:56:47Z

test/dhtest.c

+static int dh_rfc5114_fix_nid_test(void)
+{
+    int ok = 0;
+    /**pretest initial */


Why double **

t8m · 2021-10-26T07:57:06Z

test/dhtest.c

+{
+    int ok = 0;
+    /**pretest initial */
+    EVP_PKEY* params;


A space should be before * and not after.

t8m · 2021-10-26T07:57:12Z

test/dhtest.c

+    int ok = 0;
+    /**pretest initial */
+    EVP_PKEY* params;
+    EVP_PKEY_CTX* paramgen_ctx;


t8m · 2021-10-26T07:59:13Z

test/dhtest.c

+    if (1 == EVP_PKEY_paramgen_init(paramgen_ctx)) {
+    /* erronious function is called here */
+      if (1 == EVP_PKEY_CTX_set_dhx_rfc5114(paramgen_ctx, 3)) {
+    /* if we're still running then the test passed. */


please do not use this reversed order of == arguments. First should be the function invocation, second should be the constant.

the comments should be indented as the block/statement under that

indentation is wrong - we use 4 spaces per level

crypto/evp/ctrl_params_translate.c

@@ -1026,10 +1026,22 @@ static int fix_dh_nid5114(enum state state,
    if (ctx->action_type != SET)
        return 0;

-    if (state == PRE_CTRL_STR_TO_PARAMS) {
+    switch (state) {


t8m · 2021-10-26T15:12:53Z

crypto/evp/ctrl_params_translate.c

        ctx->p2 = (char *)ossl_ffc_named_group_get_name
            (ossl_ffc_uid_to_dh_named_group(atoi(ctx->p2)));
        ctx->p1 = 0;
+        break;
+      }
+      default:;


should be

default: break;

t8m · 2021-10-26T15:13:21Z

crypto/evp/ctrl_params_translate.c

@@ -1026,10 +1026,22 @@ static int fix_dh_nid5114(enum state state,
    if (ctx->action_type != SET)
        return 0;

-    if (state == PRE_CTRL_STR_TO_PARAMS) {
+    switch (state) {
+      case PRE_CTRL_TO_PARAMS: {


Do not use the unnecessary extra block {} marks.

t8m · 2021-10-26T15:14:32Z

test/dhtest.c

+    EVP_PKEY *params;
+    EVP_PKEY_CTX *paramgen_ctx;
+
+    /* run the test. success is any time the test does not cause a SIGSEGV interrupt */


Nit: Please use uppercase at the beginning of sentences.

t8m · 2021-10-26T15:14:43Z

test/dhtest.c

+    params = EVP_PKEY_new();
+    paramgen_ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_DHX, 0);
+    if (EVP_PKEY_paramgen_init(paramgen_ctx) == 1) {
+        /* erronious function is called here */


s/erronious/tested/

t8m · 2021-10-26T15:16:23Z

test/dhtest.c

+    EVP_PKEY_CTX *paramgen_ctx;
+
+    /* run the test. success is any time the test does not cause a SIGSEGV interrupt */
+    params = EVP_PKEY_new();


Please look how the other test cases handle checking of function call results via the TEST_ptr() and other TEST_xyz calls. Please use that everywhere throughout the test case.

t8m · 2021-10-27T08:35:45Z

Oops, this is wrong. There must be no merge commits.

pmesnier · 2021-10-27T08:40:35Z

I am terribly sorry but I may need to abandon this branch and resubmit. I noticed that somehow I had picked up a change to the gost-engine submodule that was in master but not yet in openssl-3.0. in the process of addressing that, i lsao performed a rebase from the upstream to update both my forked openssl-3.0 branch and my opensssl-3.0-dhparam branch on which this pr is based. Now in my forked repository these two branches differ only by the 2 files I modified, and my forked openssl-3.0 branch is identical to yours, as you can see this pr now shows many changed files.

So I propose abandoning this PR and creating a new one off of a clean fork. Do you agree?

t8m · 2021-10-27T08:50:52Z

You can also just force push changes to this branch. For example - reset the openssl-3.0-dhparam branch via git reset to the branch point from openssl-3.0 and commit the changes at once with 1 commit. Then force push.

ctx->p2 being a null pointer.

pmesnier · 2021-10-27T09:33:40Z

Thanks for the suggestion, that seems to ahve done the trick.

pmesnier · 2021-11-03T18:42:24Z

Hi, I realize there is no obligation on your part to do anything with this PR. But is it possible for you to suggest when it might be reviewed for approval and ultimately accepted or rejected?

openssl-machine · 2021-11-04T23:00:13Z

This pull request is ready to merge

ctx->p2 being a null pointer. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #16913)

ctx->p2 being a null pointer. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #16913) (cherry picked from commit 07e6c85)

paulidale · 2021-11-05T00:55:42Z

Merged to 3.0 and master. Thanks for this fix.

t8m requested changes Oct 26, 2021

View reviewed changes

t8m added branch: 3.0 Merge to openssl-3.0 branch branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug labels Oct 26, 2021

t8m requested changes Oct 26, 2021

View reviewed changes

t8m approved these changes Oct 27, 2021

View reviewed changes

t8m added the approval: review pending This pull request needs review by a committer label Oct 27, 2021

Fix for a segv interrupt that occurs when fix_dh_rfc5114 is called with

cecf261

ctx->p2 being a null pointer.

pmesnier force-pushed the openssl-3.0-dhparam branch from c362a61 to cecf261 Compare October 27, 2021 09:28

t8m approved these changes Oct 27, 2021

View reviewed changes

paulidale approved these changes Nov 3, 2021

View reviewed changes

paulidale 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 Nov 3, 2021

openssl-machine removed the approval: done This pull request has the required number of approvals label Nov 4, 2021

openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Nov 4, 2021

paulidale closed this Nov 5, 2021

pmesnier deleted the openssl-3.0-dhparam branch November 5, 2021 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for a segv in fix_dh_rfc5114 #16913

Fix for a segv in fix_dh_rfc5114 #16913

pmesnier commented Oct 25, 2021

t8m Oct 26, 2021

pmesnier Oct 26, 2021

t8m Oct 26, 2021

t8m Oct 26, 2021

t8m Oct 26, 2021

t8m Oct 26, 2021

This comment was marked as resolved.

This comment was marked as resolved.

t8m Oct 26, 2021

t8m Oct 26, 2021

t8m Oct 26, 2021

t8m Oct 26, 2021

t8m Oct 26, 2021

t8m commented Oct 27, 2021

pmesnier commented Oct 27, 2021

t8m commented Oct 27, 2021

pmesnier commented Oct 27, 2021

pmesnier commented Nov 3, 2021

openssl-machine commented Nov 4, 2021

paulidale commented Nov 5, 2021

Fix for a segv in fix_dh_rfc5114 #16913

Fix for a segv in fix_dh_rfc5114 #16913

Conversation

pmesnier commented Oct 25, 2021

Checklist

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

This comment was marked as resolved.

This comment was marked as resolved.

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

t8m commented Oct 27, 2021

pmesnier commented Oct 27, 2021

t8m commented Oct 27, 2021

pmesnier commented Oct 27, 2021

pmesnier commented Nov 3, 2021

openssl-machine commented Nov 4, 2021

paulidale commented Nov 5, 2021