Package: gnupg2 / 2.2.12-1+deb10u1

from-2.2.14/gpg-Avoid-importing-secret-keys-if-the-keyblock-is-not-va.patch Patch series | download
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
From: Werner Koch <wk@gnupg.org>
Date: Fri, 15 Mar 2019 19:50:37 +0100
Subject: gpg: Avoid importing secret keys if the keyblock is not valid.

* g10/keydb.h (struct kbnode_struct): Replace unused field RECNO by
new field TAG.
* g10/kbnode.c (alloc_node): Change accordingly.
* g10/import.c (import_one): Add arg r_valid.
(sec_to_pub_keyblock): Set tags.
(resync_sec_with_pub_keyblock): New.
(import_secret_one): Change return code to gpg_error_t.   Return an
error code if sec_to_pub_keyblock failed.  Resync secret keyblock.
--

When importing an invalid secret key ring for example without key
binding signatures or no UIDs, gpg used to let gpg-agent store the
secret keys anyway.  This is clearly a bug because the diagnostics
before claimed that for example the subkeys have been skipped.
Importing the secret key parameters then anyway is surprising in
particular because a gpg -k does not show the key.  After importing
the public key the secret keys suddenly showed up.

This changes the behaviour of
GnuPG-bug-id: 4392
to me more consistent but is not a solution to the actual bug.

Caution: The ecc.scm test now fails because two of the sample keys
         don't have binding signatures.

Signed-off-by: Werner Koch <wk@gnupg.org>
(cherry picked from commit f799e9728bcadb3d4148a47848c78c5647860ea4)
(cherry picked from commit 43b23aa82be7e02414398af506986b812e2b9349)
---
 g10/import.c                    | 122 ++++++++++++++++++++++++++++++++--------
 g10/kbnode.c                    |   2 +-
 g10/keydb.h                     |  13 +++--
 tests/openpgp/ecc.scm           |   2 +-
 tests/openpgp/samplekeys/README |   2 +
 5 files changed, 111 insertions(+), 30 deletions(-)

diff --git a/g10/import.c b/g10/import.c
index a5f4f38..2a01814 100644
--- a/g10/import.c
+++ b/g10/import.c
@@ -109,8 +109,8 @@ static gpg_error_t import_one (ctrl_t ctrl,
                        unsigned char **fpr, size_t *fpr_len,
                        unsigned int options, int from_sk, int silent,
                        import_screener_t screener, void *screener_arg,
-                       int origin, const char *url);
-static int import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
+                       int origin, const char *url, int *r_valid);
+static gpg_error_t import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
                               struct import_stats_s *stats, int batch,
                               unsigned int options, int for_migration,
                               import_screener_t screener, void *screener_arg);
@@ -584,7 +584,7 @@ import (ctrl_t ctrl, IOBUF inp, const char* fname,struct import_stats_s *stats,
       if (keyblock->pkt->pkttype == PKT_PUBLIC_KEY)
         rc = import_one (ctrl, keyblock,
                          stats, fpr, fpr_len, options, 0, 0,
-                         screener, screener_arg, origin, url);
+                         screener, screener_arg, origin, url, NULL);
       else if (keyblock->pkt->pkttype == PKT_SECRET_KEY)
         rc = import_secret_one (ctrl, keyblock, stats,
                                 opt.batch, options, 0,
@@ -1654,7 +1654,9 @@ update_key_origin (kbnode_t keyblock, u32 curtime, int origin, const char *url)
  * programs which called gpg.  If SILENT is no messages are printed -
  * even most error messages are suppressed.  ORIGIN is the origin of
  * the key (0 for unknown) and URL the corresponding URL.  FROM_SK
- * indicates that the key has been made from a secret key.
+ * indicates that the key has been made from a secret key.  If R_SAVED
+ * is not NULL a boolean will be stored indicating whether the keyblock
+ * has valid parts.
  */
 static gpg_error_t
 import_one (ctrl_t ctrl,
@@ -1662,7 +1664,7 @@ import_one (ctrl_t ctrl,
 	    unsigned char **fpr, size_t *fpr_len, unsigned int options,
 	    int from_sk, int silent,
             import_screener_t screener, void *screener_arg,
-            int origin, const char *url)
+            int origin, const char *url, int *r_valid)
 {
   gpg_error_t err = 0;
   PKT_public_key *pk;
@@ -1681,6 +1683,9 @@ import_one (ctrl_t ctrl,
   int any_filter = 0;
   KEYDB_HANDLE hd = NULL;
 
+  if (r_valid)
+    *r_valid = 0;
+
   /* If show-only is active we don't won't any extra output.  */
   if ((options & (IMPORT_SHOW | IMPORT_DRY_RUN)))
     silent = 1;
@@ -1701,7 +1706,7 @@ import_one (ctrl_t ctrl,
   if (opt.verbose && !opt.interactive && !silent && !from_sk)
     {
       /* Note that we do not print this info in FROM_SK mode
-       * because import_one already printed that.  */
+       * because import_secret_one already printed that.  */
       log_info ("pub  %s/%s %s  ",
                 pubkey_string (pk, pkstrbuf, sizeof pkstrbuf),
                 keystr_from_pk(pk), datestr_from_pk(pk) );
@@ -1827,6 +1832,10 @@ import_one (ctrl_t ctrl,
       return 0;
     }
 
+  /* The keyblock is valid and ready for real import.  */
+  if (r_valid)
+    *r_valid = 1;
+
   /* Show the key in the form it is merged or inserted.  We skip this
    * if "import-export" is also active without --armor or the output
    * file has explicily been given. */
@@ -2440,14 +2449,21 @@ transfer_secret_keys (ctrl_t ctrl, struct import_stats_s *stats,
 
 
 /* Walk a secret keyblock and produce a public keyblock out of it.
-   Returns a new node or NULL on error. */
+ * Returns a new node or NULL on error.  Modifies the tag field of the
+ * nodes.  */
 static kbnode_t
 sec_to_pub_keyblock (kbnode_t sec_keyblock)
 {
   kbnode_t pub_keyblock = NULL;
   kbnode_t ctx = NULL;
   kbnode_t secnode, pubnode;
+  unsigned int tag = 0;
+
+  /* Set a tag to all nodes.  */
+  for (secnode = sec_keyblock; secnode; secnode = secnode->next)
+    secnode->tag = ++tag;
 
+  /* Copy.  */
   while ((secnode = walk_kbnode (sec_keyblock, &ctx, 0)))
     {
       if (secnode->pkt->pkttype == PKT_SECRET_KEY
@@ -2477,6 +2493,7 @@ sec_to_pub_keyblock (kbnode_t sec_keyblock)
 	{
 	  pubnode = clone_kbnode (secnode);
 	}
+      pubnode->tag = secnode->tag;
 
       if (!pub_keyblock)
 	pub_keyblock = pubnode;
@@ -2487,23 +2504,67 @@ sec_to_pub_keyblock (kbnode_t sec_keyblock)
   return pub_keyblock;
 }
 
+
+/* Delete all notes in the keyblock at R_KEYBLOCK which are not in
+ * PUB_KEYBLOCK.  Modifies the tags of both keyblock's nodes.  */
+static gpg_error_t
+resync_sec_with_pub_keyblock (kbnode_t *r_keyblock, kbnode_t pub_keyblock)
+{
+  kbnode_t sec_keyblock = *r_keyblock;
+  kbnode_t node;
+  unsigned int *taglist;
+  unsigned int ntaglist, n;
+
+  /* Collect all tags in an array for faster searching.  */
+  for (ntaglist = 0, node = pub_keyblock; node; node = node->next)
+    ntaglist++;
+  taglist = xtrycalloc (ntaglist, sizeof *taglist);
+  if (!taglist)
+    return gpg_error_from_syserror ();
+  for (ntaglist = 0, node = pub_keyblock; node; node = node->next)
+    taglist[ntaglist++] = node->tag;
+
+  /* Walks over the secret keyblock and delete all nodes which are not
+   * in the tag list.  Those nodes have been delete in the
+   * pub_keyblock.  Sequential search is a bit lazt and could be
+   * optimized by sorting and bsearch; however secret key rings are
+   * short and there are easier weaus to DoS gpg.  */
+  for (node = sec_keyblock; node; node = node->next)
+    {
+      for (n=0; n < ntaglist; n++)
+        if (taglist[n] == node->tag)
+          break;
+      if (n == ntaglist)
+        delete_kbnode (node);
+    }
+
+  xfree (taglist);
+
+  /* Commit the as deleted marked nodes and return the possibly
+   * modified keyblock.  */
+  commit_kbnode (&sec_keyblock);
+  *r_keyblock = sec_keyblock;
+  return 0;
+}
+
+
 /****************
  * Ditto for secret keys.  Handling is simpler than for public keys.
  * We allow secret key importing only when allow is true, this is so
  * that a secret key can not be imported accidentally and thereby tampering
  * with the trust calculation.
  */
-static int
+static gpg_error_t
 import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
-                   struct import_stats_s *stats, int batch, unsigned int options,
-                   int for_migration,
+                   struct import_stats_s *stats, int batch,
+                   unsigned int options, int for_migration,
                    import_screener_t screener, void *screener_arg)
 {
   PKT_public_key *pk;
   struct seckey_info *ski;
   kbnode_t node, uidnode;
   u32 keyid[2];
-  int rc = 0;
+  gpg_error_t err = 0;
   int nr_prev;
   kbnode_t pub_keyblock;
   char pkstrbuf[PUBKEY_STRING_SIZE];
@@ -2527,7 +2588,7 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
 
   if (opt.verbose && !for_migration)
     {
-      log_info ("sec  %s/%s %s   ",
+      log_info ("sec  %s/%s %s  ",
                 pubkey_string (pk, pkstrbuf, sizeof pkstrbuf),
                 keystr_from_pk (pk), datestr_from_pk (pk));
       if (uidnode)
@@ -2587,20 +2648,35 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
   /* Make a public key out of the key. */
   pub_keyblock = sec_to_pub_keyblock (keyblock);
   if (!pub_keyblock)
-    log_error ("key %s: failed to create public key from secret key\n",
-                   keystr_from_pk (pk));
+    {
+      err = gpg_error_from_syserror ();
+      log_error ("key %s: failed to create public key from secret key\n",
+                 keystr_from_pk (pk));
+    }
   else
     {
+      int valid;
+
       /* Note that this outputs an IMPORT_OK status message for the
 	 public key block, and below we will output another one for
 	 the secret keys.  FIXME?  */
       import_one (ctrl, pub_keyblock, stats,
 		  NULL, NULL, options, 1, for_migration,
-                  screener, screener_arg, 0, NULL);
+                  screener, screener_arg, 0, NULL, &valid);
 
-      /* Fixme: We should check for an invalid keyblock and
-	 cancel the secret key import in this case.  */
-      release_kbnode (pub_keyblock);
+      /* 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
+       * surprising and clutters out private-keys-v1.d.  */
+      err = resync_sec_with_pub_keyblock (&keyblock, pub_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
@@ -2608,7 +2684,8 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
       if (!(opt.dry_run || (options & IMPORT_DRY_RUN))
           && stats->skipped_new_keys <= nr_prev)
 	{
-          /* Read the keyblock again to get the effects of a merge.  */
+          /* Read the keyblock again to get the effects of a merge for
+           * the public key.  */
           /* Fixme: we should do this based on the fingerprint or
              even better let import_one return the merged
              keyblock.  */
@@ -2618,8 +2695,6 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
                        keystr_from_pk (pk));
           else
             {
-              gpg_error_t err;
-
               /* transfer_secret_keys collects subkey stats.  */
               struct import_stats_s subkey_stats = {0};
 
@@ -2657,6 +2732,7 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
 
                   if (is_status_enabled ())
                     print_import_ok (pk, status);
+
                   check_prefs (ctrl, node);
                 }
               release_kbnode (node);
@@ -2664,7 +2740,9 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
         }
     }
 
-  return rc;
+ leave:
+  release_kbnode (pub_keyblock);
+  return err;
 }
 
 
diff --git a/g10/kbnode.c b/g10/kbnode.c
index c2aaacd..9ed6caf 100644
--- a/g10/kbnode.c
+++ b/g10/kbnode.c
@@ -68,8 +68,8 @@ alloc_node (void)
   n->next = NULL;
   n->pkt = NULL;
   n->flag = 0;
+  n->tag = 0;
   n->private_flag=0;
-  n->recno = 0;
   return n;
 }
 
diff --git a/g10/keydb.h b/g10/keydb.h
index 6fb4e5e..7aa2048 100644
--- a/g10/keydb.h
+++ b/g10/keydb.h
@@ -52,12 +52,13 @@ typedef struct getkey_ctx_s *getkey_ctx_t;
  * This structure is also used to bind arbitrary packets together.
  */
 
-struct kbnode_struct {
-    KBNODE next;
-    PACKET *pkt;
-    int flag;
-    int private_flag;
-    ulong recno;  /* used while updating the trustdb */
+struct kbnode_struct
+{
+  kbnode_t next;
+  PACKET *pkt;
+  int flag;          /* Local use during keyblock processing (not cloned).*/
+  unsigned int tag;  /* Ditto. */
+  int private_flag;
 };
 
 #define is_deleted_kbnode(a)  ((a)->private_flag & 1)
diff --git a/tests/openpgp/ecc.scm b/tests/openpgp/ecc.scm
index d7c02a5..a63ec45 100755
--- a/tests/openpgp/ecc.scm
+++ b/tests/openpgp/ecc.scm
@@ -175,7 +175,7 @@ Rg==
 	 (display "This is one line\n" (fdopen fd "wb")))
 
   (for-each-p
-   "Checking ECDSA decryption"
+   "Checking ECDH decryption"
    (lambda (test)
      (lettmp (x y)
        (call-with-output-file
diff --git a/tests/openpgp/samplekeys/README b/tests/openpgp/samplekeys/README
index 9f1648b..f8a7e9e 100644
--- a/tests/openpgp/samplekeys/README
+++ b/tests/openpgp/samplekeys/README
@@ -29,3 +29,5 @@ Notes:
   such a file is created which is then directly followed by a separate
   armored public key block.  To create such a sample concatenate
   pgp-desktop-skr.asc and E657FB607BB4F21C90BB6651BC067AF28BC90111.asc
+- ecc-sample-2-sec.asc and ecc-sample-3-sec.asc do not have and
+  binding signatures either.  ecc-sample-1-sec.asc has them, though.