Page MenuHome GnuPG
Create Task
Maniphest T4410

Memory leak in import recently introduced
Closed, ResolvedPublic
Actions

Description

Found by oss-fuzz
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13756

Commit f799e9728bcadb3d4148a47848c78c5647860ea4 introduced a memory leak in import.c
pkt = xmalloc (sizeof *pkt); gets allocated in import

Bug can be reproduced with {F630817}: gpg --import clusterfuzz-testcase-minimized-fuzz_import-5751600352591872.dms

Bug was introduced in import_secret_one
Patch should be checking valid value from import_one before calling resync_sec_with_pub_keyblock:

diff --git a/g10/import.c b/g10/import.c
index 155792d5a..6a15cf565 100644
--- a/g10/import.c
+++ b/g10/import.c
@@ -2687,6 +2687,12 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
                  NULL, NULL, options, 1, for_migration,
                   screener, screener_arg, 0, NULL, &valid);
 
+        if (!valid)
+        {
+            err = gpg_error (GPG_ERR_NO_SECKEY);
+            goto leave;
+        }
+
       /* The secret keyblock may not have nodes which are deleted in
        * the public keyblock.  Otherwise we would import just the
        * secret key without having the public key.  That would be
@@ -2695,12 +2701,6 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
       if (err)
         goto leave;
 
-      if (!valid)
-        {
-          err = gpg_error (GPG_ERR_NO_SECKEY);
-          goto leave;
-        }
-
       /* At least we cancel the secret key import when the public key
         import was skipped due to MERGE_ONLY option and a new
         key.  */

Related Objects

Event Timeline

catenacyber created this task.Mar 18 2019, 11:56 AM
• werner updated the task description. (Show Details)Mar 18 2019, 6:03 PM
• werner added a subscriber: • werner.

That was an intermediate commit on master - it is likely that there are memory leaks.
Moving the test around is not a solution. BTW {F630817} is not accessible.

catenacyber added a comment.Mar 18 2019, 8:24 PM

Ok, I will wait longer next time.
How do I make the file accessible ? (I can download it)

clusterfuzz-testcase-minimized-fuzz_import-5751600352591872.dms186 BDownload

• werner added a comment.Mar 19 2019, 7:56 AM

This file is readable. You must have changed the former one's visibility so that only you can view it.

Valgrind doesn't show a problem with that file and my current code, though.

catenacyber added a comment.Mar 19 2019, 9:07 AM

Running
valgrind --leak-check=full ./g10/gpg --import clusterfuzz-testcase-minimized-fuzz_import-5751600352591872.dms
gave me at commit f799e9728bcadb3d4148a47848c78c5647860ea4

==11882== 232 (16 direct, 216 indirect) bytes in 1 blocks are definitely lost in loss record 290 of 333
==11882==    at 0x1001C32C5: malloc (vg_replace_malloc.c:302)
==11882==    by 0x100B211B9: do_malloc (in /usr/local/Cellar/libgcrypt/1.8.3/lib/libgcrypt.20.dylib)
==11882==    by 0x100B214D5: _gcry_xmalloc (in /usr/local/Cellar/libgcrypt/1.8.3/lib/libgcrypt.20.dylib)
==11882==    by 0x100058A1D: read_block (import.c:929)
==11882==    by 0x10005B772: import (import.c:584)
==11882==    by 0x1000597FF: import_keys_internal (import.c:486)
==11882==    by 0x1000596FE: import_keys (import.c:526)
==11882==    by 0x10000727B: main (gpg.c:4675)

This looks fixed at commit b98799ce964df1743478dc3d0cc503f51c4b6733

• werner closed this task as Resolved.Mar 19 2019, 1:43 PM
• werner claimed this task.