Refactor PEM_read_bio_{PrivateKey,Parameters,DHparams} by levitte · Pull Request #2746 · openssl/openssl

levitte · 2017-02-25T23:39:48Z

They now go through internal STORE routines to do their job.

Just like #2696 and #2697, this PR will not build correctly until #3542, #2745 and #3483 have been approved and merged.

kaduk · 2017-02-27T01:42:28Z

This seems like it might make it harder for #1700 to achieve its goal of using memory from the secure heap for the temporary buffer used while reading private keys from disk.
(It looks like the latest version of that hasn't gotten any team review despite sitting there for a while ... probably overdue for a rebase, anyway, I suppose.)

levitte · 2017-02-27T05:17:34Z

Not really, but the proper place would be to introduce secure heap in crypto/store/loader_file.c in #2011.

levitte · 2017-02-27T13:32:29Z

I just added some code in #2011 that adds support for finer control of loaders. With that, the following diff should provide fairly good support for what you do in #1700, no?

diff --git a/crypto/store/loader_file.c b/crypto/store/loader_file.c
index 9d7e860dec..b59ecf6113 100644
--- a/crypto/store/loader_file.c
+++ b/crypto/store/loader_file.c
@@ -551,6 +551,8 @@ struct store_loader_ctx_st {
         is_pem,
         is_dir
     } type;
+#define FILE_FLAG_SECMEM         (1<<0)
+    unsigned int flags;
     union {
         struct {                 /* Used with is_raw and is_pem */
             BIO *file;
@@ -737,6 +739,36 @@ static STORE_LOADER_CTX *file_open(const char *scheme, const char *user,
     return NULL;
 }
 
+int file_ctrl(STORE_LOADER_CTX *ctx, int cmd, va_list args)
+{
+    int ret = 1;
+
+    switch (cmd) {
+    case STORE_C_USE_SECMEM:
+        {
+            int on = *(va_arg(args, int *));
+
+            switch (on) {
+            case 0:
+                ctx->flags &= ~FILE_FLAG_SECMEM;
+                break;
+            case 1:
+                ctx->flags |= FILE_FLAG_SECMEM;
+                break;
+            default:
+                STOREerr(STORE_F_FILE_CTRL, ERR_R_PASSED_INVALID_ARGUMENT);
+                ret = 0;
+                break;
+            }
+        }
+        break;
+    default:
+        break;
+    }
+
+    return ret;
+}
+
 static STORE_INFO *file_load_try_decode(STORE_LOADER_CTX *ctx,
                                         const char *pem_name,
                                         const char *pem_header,
@@ -862,12 +894,22 @@ static STORE_INFO *file_load_try_repeat(STORE_LOADER_CTX *ctx,
     return result;
 }
 
+static void pem_free_flag(char *pem_data, int secure)
+{
+    if (secure)
+        OPENSSL_secure_free(pem_data);
+    else
+        OPENSSL_free(pem_data);
+}
 static int file_read_pem(BIO *bp, char **pem_name, char **pem_header,
                          unsigned char **data, long *len,
                          const UI_METHOD *ui_method,
-                         void *ui_data)
+                         void *ui_data, int secure)
 {
-    int i = PEM_read_bio(bp, pem_name, pem_header, data, len);
+    int i = secure
+        ? PEM_read_bio_flags(bp, pem_name, pem_header, data, len,
+                             PEM_FLAG_SECURE);
+        : PEM_read_bio(bp, pem_name, pem_header, data, len);
 
     if (i <= 0)
         return 0;
@@ -881,9 +923,9 @@ static int file_read_pem(BIO *bp, char **pem_name, char **pem_header,
                                         ui_data)
             || !PEM_do_header(&cipher, *data, len, file_get_pem_pass,
                               &pass_data)) {
-            OPENSSL_free(*pem_name);
-            OPENSSL_free(*pem_header);
-            OPENSSL_free(*data);
+            pem_free_flag(*pem_name, secure);
+            pem_free_flag(*pem_header, secure);
+            pem_free_flag(*data, secure);
             *pem_name = NULL;
             *pem_header = NULL;
             *data = NULL;
@@ -1064,7 +1106,8 @@ static STORE_INFO *file_load(STORE_LOADER_CTX *ctx,
             matchcount = -1;
             if (ctx->type == is_pem) {
                 if (!file_read_pem(ctx->_.file.file, &pem_name, &pem_header,
-                                   &data, &len, ui_method, ui_data))
+                                   &data, &len, ui_method, ui_data,
+                                   (ctx->flags & FILE_FLAG_SECMEM) != 0))
                     goto err;
             } else {
                 if (!file_read_asn1(ctx->_.file.file, &data, &len))
@@ -1075,9 +1118,9 @@ static STORE_INFO *file_load(STORE_LOADER_CTX *ctx,
                                           ui_method, ui_data, &matchcount);
 
          err:
-            OPENSSL_free(pem_name);
-            OPENSSL_free(pem_header);
-            OPENSSL_free(data);
+            pem_free_flag(pem_name, (ctx->flags & FILE_FLAG_SECMEM) != 0);
+            pem_free_flag(pem_header, (ctx->flags & FILE_FLAG_SECMEM) != 0);
+            pem_free_flag(data, (ctx->flags & FILE_FLAG_SECMEM) != 0);
         } while (matchcount == 0 && !file_eof(ctx));
 
         /* We bail out on ambiguity */
diff --git a/include/openssl/store.h b/include/openssl/store.h
index e73b1df01b..f8ee6cd5be 100644
--- a/include/openssl/store.h
+++ b/include/openssl/store.h
@@ -70,6 +70,8 @@ int STORE_ctrl(STORE_CTX *ctx, int cmd, ... /* args */);
 /*
  * Common ctrl commands that different loaders may choose to support.
  */
+/* int on = 0 or 1; STORE_ctrl(ctx, STORE_C_USE_SECMEM, &on); */
+# define STORE_C_USE_SECMEM      1
 /* Where custom commands start */
 # define STORE_C_CUSTOM_START    100

levitte · 2017-02-27T13:36:50Z

To finalize support in this PR:

diff --git a/crypto/pem/pem_pkey.c b/crypto/pem/pem_pkey.c
index 01b9828534..8b9d014a8e 100644
--- a/crypto/pem/pem_pkey.c
+++ b/crypto/pem/pem_pkey.c
@@ -38,6 +38,13 @@ EVP_PKEY *PEM_read_bio_PrivateKey(BIO *bp, EVP_PKEY **x, pem_password_cb *cb,
 
     if ((ctx = store_attach_pem_bio(bp, ui_method, u)) == NULL)
         goto err;
+#ifndef OPENSSL_NO_SECURE_HEAP
+    {
+        int on = 1;
+        if (STORE_ctrl(ctx, STORE_C_USE_SECMEM, &on))
+            goto err;
+    }
+#endif
 
     while ((info = STORE_load(ctx)) != NULL
            && STORE_INFO_get_type(info) != STORE_INFO_UNSPECIFIED) {
@@ -88,6 +95,13 @@ EVP_PKEY *PEM_read_bio_Parameters(BIO *bp, EVP_PKEY **x)
 
     if ((ctx = store_attach_pem_bio(bp, UI_null(), NULL)) == NULL)
         goto err;
+#ifndef OPENSSL_NO_SECURE_HEAP
+    {
+        int on = 1;
+        if (STORE_ctrl(ctx, STORE_C_USE_SECMEM, &on))
+            goto err;
+    }
+#endif
 
     while ((info = STORE_load(ctx)) != NULL
            && STORE_INFO_get_type(info) != STORE_INFO_UNSPECIFIED) {
@@ -171,6 +185,13 @@ DH *PEM_read_bio_DHparams(BIO *bp, DH **x, pem_password_cb *cb, void *u)
 
     if ((ctx = store_attach_pem_bio(bp, ui_method, u)) == NULL)
         goto err;
+#ifndef OPENSSL_NO_SECURE_HEAP
+    {
+        int on = 1;
+        if (STORE_ctrl(ctx, STORE_C_USE_SECMEM, &on))
+            goto err;
+    }
+#endif
 
     while ((info = STORE_load(ctx)) != NULL
            && STORE_INFO_get_type(info) != STORE_INFO_UNSPECIFIED) {

levitte · 2017-02-27T13:37:30Z

Note: I haven't tested those diffs

kaduk · 2017-02-27T17:06:52Z

With that, the following diff should provide fairly good support for what you do in #1700, no?

Looks like it (and not very hairy, either).
I guess I should get cracking on the PEM-reader test corpus I've got sitting around, and then update #1700.

mattcaswell · 2019-10-31T13:37:42Z

Needs a rebase

crypto/pem/pem_pkey.c

They now go through internal STORE routines to do their job.

Domain parameters are public data, so secure heap is unnecessary for them

levitte · 2019-11-06T13:18:33Z

Rebased and fixups added

They now go through internal STORE routines to do their job. Reviewed-by: Matt Caswell <[email protected]> (Merged from #2746)

levitte · 2019-11-07T22:38:24Z

Merged.

1427d33 Refactor PEM_read_bio_{PrivateKey,Parameters,DHparams}

levitte added the branch: master Applies to master branch label Feb 25, 2017

levitte self-assigned this Feb 25, 2017

levitte mentioned this pull request Feb 27, 2017

PEM read-side refactor #1700

Closed

levitte force-pushed the pemreader-store2-integration branch from 649a277 to cd25258 Compare May 17, 2017 06:04

levitte force-pushed the pemreader-store2-integration branch 5 times, most recently from 0b038f1 to f1869df Compare May 26, 2017 21:14

levitte force-pushed the pemreader-store2-integration branch 2 times, most recently from 14e4413 to 3be3b04 Compare June 29, 2017 00:00

levitte mentioned this pull request Jun 29, 2017

Add the STORE module take 2, for loaded certs, keys and more given a URI (cont) #3542

Closed

2 tasks

levitte force-pushed the pemreader-store2-integration branch from 3be3b04 to c281833 Compare July 6, 2017 14:31

levitte force-pushed the pemreader-store2-integration branch from c281833 to 14c38e5 Compare July 20, 2017 15:08

levitte force-pushed the pemreader-store2-integration branch from 14c38e5 to fd96afe Compare September 5, 2017 18:15

levitte force-pushed the pemreader-store2-integration branch from fd96afe to 815a3be Compare October 7, 2017 08:36

mattcaswell added this to the Post 1.1.1 milestone Jan 24, 2018

levitte force-pushed the pemreader-store2-integration branch from 815a3be to 27bc94e Compare January 25, 2018 09:03

levitte force-pushed the pemreader-store2-integration branch 2 times, most recently from a022b4f to dc0b767 Compare February 23, 2018 21:23

richsalz added after 1.1.1 and removed after 1.1.1 labels May 6, 2018

levitte force-pushed the pemreader-store2-integration branch from dc0b767 to 318c5f5 Compare June 21, 2018 05:17

mspncp removed the after 1.1.1 label Oct 25, 2019

mattcaswell added the approval: review pending This pull request needs review by a committer label Oct 31, 2019

mattcaswell requested changes Nov 4, 2019

View reviewed changes

crypto/pem/pem_pkey.c Outdated Show resolved Hide resolved

crypto/pem/pem_pkey.c Outdated Show resolved Hide resolved

levitte added 2 commits November 6, 2019 14:15

Refactor PEM_read_bio_{PrivateKey,Parameters,DHparams}

9cd62b3

They now go through internal STORE routines to do their job.

fixup! Refactor PEM_read_bio_{PrivateKey,Parameters,DHparams}

da1015e

Domain parameters are public data, so secure heap is unnecessary for them

levitte force-pushed the pemreader-store2-integration branch from 318c5f5 to da1015e Compare November 6, 2019 13:18

mattcaswell approved these changes Nov 6, 2019

View reviewed changes

openssl-machine pushed a commit that referenced this pull request Nov 7, 2019

Refactor PEM_read_bio_{PrivateKey,Parameters,DHparams}

1427d33

They now go through internal STORE routines to do their job. Reviewed-by: Matt Caswell <[email protected]> (Merged from #2746)

levitte closed this Nov 7, 2019

matbech mentioned this pull request Nov 14, 2019

PEM_read_bio_PrivateKey regression #10444

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor PEM_read_bio_{PrivateKey,Parameters,DHparams}#2746

Refactor PEM_read_bio_{PrivateKey,Parameters,DHparams}#2746
levitte wants to merge 2 commits intoopenssl:masterfrom
levitte:pemreader-store2-integration

levitte commented Feb 25, 2017 •

edited

Loading

Uh oh!

kaduk commented Feb 27, 2017

Uh oh!

levitte commented Feb 27, 2017

Uh oh!

levitte commented Feb 27, 2017

Uh oh!

levitte commented Feb 27, 2017

Uh oh!

levitte commented Feb 27, 2017

Uh oh!

kaduk commented Feb 27, 2017

Uh oh!

mattcaswell commented Oct 31, 2019

Uh oh!

Uh oh!

Uh oh!

levitte commented Nov 6, 2019

Uh oh!

levitte commented Nov 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

levitte commented Feb 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaduk commented Feb 27, 2017

Uh oh!

levitte commented Feb 27, 2017

Uh oh!

levitte commented Feb 27, 2017

Uh oh!

levitte commented Feb 27, 2017

Uh oh!

levitte commented Feb 27, 2017

Uh oh!

kaduk commented Feb 27, 2017

Uh oh!

mattcaswell commented Oct 31, 2019

Uh oh!

Uh oh!

Uh oh!

levitte commented Nov 6, 2019

Uh oh!

levitte commented Nov 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

levitte commented Feb 25, 2017 •

edited

Loading