mirror of
git://git.gnupg.org/gnupg.git
synced 2024-12-22 10:19:57 +01:00
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)
This commit is contained in:
parent
61fc831885
commit
43b23aa82b
116
g10/import.c
116
g10/import.c
@ -109,8 +109,8 @@ static gpg_error_t import_one (ctrl_t ctrl,
|
|||||||
unsigned char **fpr, size_t *fpr_len,
|
unsigned char **fpr, size_t *fpr_len,
|
||||||
unsigned int options, int from_sk, int silent,
|
unsigned int options, int from_sk, int silent,
|
||||||
import_screener_t screener, void *screener_arg,
|
import_screener_t screener, void *screener_arg,
|
||||||
int origin, const char *url);
|
int origin, const char *url, int *r_valid);
|
||||||
static int import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
|
static gpg_error_t import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
|
||||||
struct import_stats_s *stats, int batch,
|
struct import_stats_s *stats, int batch,
|
||||||
unsigned int options, int for_migration,
|
unsigned int options, int for_migration,
|
||||||
import_screener_t screener, void *screener_arg);
|
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)
|
if (keyblock->pkt->pkttype == PKT_PUBLIC_KEY)
|
||||||
rc = import_one (ctrl, keyblock,
|
rc = import_one (ctrl, keyblock,
|
||||||
stats, fpr, fpr_len, options, 0, 0,
|
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)
|
else if (keyblock->pkt->pkttype == PKT_SECRET_KEY)
|
||||||
rc = import_secret_one (ctrl, keyblock, stats,
|
rc = import_secret_one (ctrl, keyblock, stats,
|
||||||
opt.batch, options, 0,
|
opt.batch, options, 0,
|
||||||
@ -1645,7 +1645,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 -
|
* programs which called gpg. If SILENT is no messages are printed -
|
||||||
* even most error messages are suppressed. ORIGIN is the origin of
|
* even most error messages are suppressed. ORIGIN is the origin of
|
||||||
* the key (0 for unknown) and URL the corresponding URL. FROM_SK
|
* 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
|
static gpg_error_t
|
||||||
import_one (ctrl_t ctrl,
|
import_one (ctrl_t ctrl,
|
||||||
@ -1653,7 +1655,7 @@ import_one (ctrl_t ctrl,
|
|||||||
unsigned char **fpr, size_t *fpr_len, unsigned int options,
|
unsigned char **fpr, size_t *fpr_len, unsigned int options,
|
||||||
int from_sk, int silent,
|
int from_sk, int silent,
|
||||||
import_screener_t screener, void *screener_arg,
|
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;
|
gpg_error_t err = 0;
|
||||||
PKT_public_key *pk;
|
PKT_public_key *pk;
|
||||||
@ -1672,6 +1674,9 @@ import_one (ctrl_t ctrl,
|
|||||||
int any_filter = 0;
|
int any_filter = 0;
|
||||||
KEYDB_HANDLE hd = NULL;
|
KEYDB_HANDLE hd = NULL;
|
||||||
|
|
||||||
|
if (r_valid)
|
||||||
|
*r_valid = 0;
|
||||||
|
|
||||||
/* If show-only is active we don't won't any extra output. */
|
/* If show-only is active we don't won't any extra output. */
|
||||||
if ((options & (IMPORT_SHOW | IMPORT_DRY_RUN)))
|
if ((options & (IMPORT_SHOW | IMPORT_DRY_RUN)))
|
||||||
silent = 1;
|
silent = 1;
|
||||||
@ -1692,7 +1697,7 @@ import_one (ctrl_t ctrl,
|
|||||||
if (opt.verbose && !opt.interactive && !silent && !from_sk)
|
if (opt.verbose && !opt.interactive && !silent && !from_sk)
|
||||||
{
|
{
|
||||||
/* Note that we do not print this info in FROM_SK mode
|
/* 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 ",
|
log_info ("pub %s/%s %s ",
|
||||||
pubkey_string (pk, pkstrbuf, sizeof pkstrbuf),
|
pubkey_string (pk, pkstrbuf, sizeof pkstrbuf),
|
||||||
keystr_from_pk(pk), datestr_from_pk(pk) );
|
keystr_from_pk(pk), datestr_from_pk(pk) );
|
||||||
@ -1818,6 +1823,10 @@ import_one (ctrl_t ctrl,
|
|||||||
return 0;
|
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
|
/* 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
|
* if "import-export" is also active without --armor or the output
|
||||||
* file has explicily been given. */
|
* file has explicily been given. */
|
||||||
@ -2431,14 +2440,21 @@ transfer_secret_keys (ctrl_t ctrl, struct import_stats_s *stats,
|
|||||||
|
|
||||||
|
|
||||||
/* Walk a secret keyblock and produce a public keyblock out of it.
|
/* 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
|
static kbnode_t
|
||||||
sec_to_pub_keyblock (kbnode_t sec_keyblock)
|
sec_to_pub_keyblock (kbnode_t sec_keyblock)
|
||||||
{
|
{
|
||||||
kbnode_t pub_keyblock = NULL;
|
kbnode_t pub_keyblock = NULL;
|
||||||
kbnode_t ctx = NULL;
|
kbnode_t ctx = NULL;
|
||||||
kbnode_t secnode, pubnode;
|
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)))
|
while ((secnode = walk_kbnode (sec_keyblock, &ctx, 0)))
|
||||||
{
|
{
|
||||||
if (secnode->pkt->pkttype == PKT_SECRET_KEY
|
if (secnode->pkt->pkttype == PKT_SECRET_KEY
|
||||||
@ -2468,6 +2484,7 @@ sec_to_pub_keyblock (kbnode_t sec_keyblock)
|
|||||||
{
|
{
|
||||||
pubnode = clone_kbnode (secnode);
|
pubnode = clone_kbnode (secnode);
|
||||||
}
|
}
|
||||||
|
pubnode->tag = secnode->tag;
|
||||||
|
|
||||||
if (!pub_keyblock)
|
if (!pub_keyblock)
|
||||||
pub_keyblock = pubnode;
|
pub_keyblock = pubnode;
|
||||||
@ -2478,23 +2495,67 @@ sec_to_pub_keyblock (kbnode_t sec_keyblock)
|
|||||||
return pub_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.
|
* Ditto for secret keys. Handling is simpler than for public keys.
|
||||||
* We allow secret key importing only when allow is true, this is so
|
* 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
|
* that a secret key can not be imported accidentally and thereby tampering
|
||||||
* with the trust calculation.
|
* with the trust calculation.
|
||||||
*/
|
*/
|
||||||
static int
|
static gpg_error_t
|
||||||
import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
|
import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
|
||||||
struct import_stats_s *stats, int batch, unsigned int options,
|
struct import_stats_s *stats, int batch,
|
||||||
int for_migration,
|
unsigned int options, int for_migration,
|
||||||
import_screener_t screener, void *screener_arg)
|
import_screener_t screener, void *screener_arg)
|
||||||
{
|
{
|
||||||
PKT_public_key *pk;
|
PKT_public_key *pk;
|
||||||
struct seckey_info *ski;
|
struct seckey_info *ski;
|
||||||
kbnode_t node, uidnode;
|
kbnode_t node, uidnode;
|
||||||
u32 keyid[2];
|
u32 keyid[2];
|
||||||
int rc = 0;
|
gpg_error_t err = 0;
|
||||||
int nr_prev;
|
int nr_prev;
|
||||||
kbnode_t pub_keyblock;
|
kbnode_t pub_keyblock;
|
||||||
char pkstrbuf[PUBKEY_STRING_SIZE];
|
char pkstrbuf[PUBKEY_STRING_SIZE];
|
||||||
@ -2578,20 +2639,35 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
|
|||||||
/* Make a public key out of the key. */
|
/* Make a public key out of the key. */
|
||||||
pub_keyblock = sec_to_pub_keyblock (keyblock);
|
pub_keyblock = sec_to_pub_keyblock (keyblock);
|
||||||
if (!pub_keyblock)
|
if (!pub_keyblock)
|
||||||
|
{
|
||||||
|
err = gpg_error_from_syserror ();
|
||||||
log_error ("key %s: failed to create public key from secret key\n",
|
log_error ("key %s: failed to create public key from secret key\n",
|
||||||
keystr_from_pk (pk));
|
keystr_from_pk (pk));
|
||||||
|
}
|
||||||
else
|
else
|
||||||
{
|
{
|
||||||
|
int valid;
|
||||||
|
|
||||||
/* Note that this outputs an IMPORT_OK status message for the
|
/* Note that this outputs an IMPORT_OK status message for the
|
||||||
public key block, and below we will output another one for
|
public key block, and below we will output another one for
|
||||||
the secret keys. FIXME? */
|
the secret keys. FIXME? */
|
||||||
import_one (ctrl, pub_keyblock, stats,
|
import_one (ctrl, pub_keyblock, stats,
|
||||||
NULL, NULL, options, 1, for_migration,
|
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
|
/* The secret keyblock may not have nodes which are deleted in
|
||||||
cancel the secret key import in this case. */
|
* the public keyblock. Otherwise we would import just the
|
||||||
release_kbnode (pub_keyblock);
|
* 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
|
/* At least we cancel the secret key import when the public key
|
||||||
import was skipped due to MERGE_ONLY option and a new
|
import was skipped due to MERGE_ONLY option and a new
|
||||||
@ -2599,7 +2675,8 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
|
|||||||
if (!(opt.dry_run || (options & IMPORT_DRY_RUN))
|
if (!(opt.dry_run || (options & IMPORT_DRY_RUN))
|
||||||
&& stats->skipped_new_keys <= nr_prev)
|
&& 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
|
/* Fixme: we should do this based on the fingerprint or
|
||||||
even better let import_one return the merged
|
even better let import_one return the merged
|
||||||
keyblock. */
|
keyblock. */
|
||||||
@ -2609,8 +2686,6 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
|
|||||||
keystr_from_pk (pk));
|
keystr_from_pk (pk));
|
||||||
else
|
else
|
||||||
{
|
{
|
||||||
gpg_error_t err;
|
|
||||||
|
|
||||||
/* transfer_secret_keys collects subkey stats. */
|
/* transfer_secret_keys collects subkey stats. */
|
||||||
struct import_stats_s subkey_stats = {0};
|
struct import_stats_s subkey_stats = {0};
|
||||||
|
|
||||||
@ -2648,6 +2723,7 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
|
|||||||
|
|
||||||
if (is_status_enabled ())
|
if (is_status_enabled ())
|
||||||
print_import_ok (pk, status);
|
print_import_ok (pk, status);
|
||||||
|
|
||||||
check_prefs (ctrl, node);
|
check_prefs (ctrl, node);
|
||||||
}
|
}
|
||||||
release_kbnode (node);
|
release_kbnode (node);
|
||||||
@ -2655,7 +2731,9 @@ import_secret_one (ctrl_t ctrl, kbnode_t keyblock,
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
return rc;
|
leave:
|
||||||
|
release_kbnode (pub_keyblock);
|
||||||
|
return err;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
@ -68,8 +68,8 @@ alloc_node (void)
|
|||||||
n->next = NULL;
|
n->next = NULL;
|
||||||
n->pkt = NULL;
|
n->pkt = NULL;
|
||||||
n->flag = 0;
|
n->flag = 0;
|
||||||
|
n->tag = 0;
|
||||||
n->private_flag=0;
|
n->private_flag=0;
|
||||||
n->recno = 0;
|
|
||||||
return n;
|
return n;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -52,12 +52,13 @@ typedef struct getkey_ctx_s *getkey_ctx_t;
|
|||||||
* This structure is also used to bind arbitrary packets together.
|
* This structure is also used to bind arbitrary packets together.
|
||||||
*/
|
*/
|
||||||
|
|
||||||
struct kbnode_struct {
|
struct kbnode_struct
|
||||||
KBNODE next;
|
{
|
||||||
|
kbnode_t next;
|
||||||
PACKET *pkt;
|
PACKET *pkt;
|
||||||
int flag;
|
int flag; /* Local use during keyblock processing (not cloned).*/
|
||||||
|
unsigned int tag; /* Ditto. */
|
||||||
int private_flag;
|
int private_flag;
|
||||||
ulong recno; /* used while updating the trustdb */
|
|
||||||
};
|
};
|
||||||
|
|
||||||
#define is_deleted_kbnode(a) ((a)->private_flag & 1)
|
#define is_deleted_kbnode(a) ((a)->private_flag & 1)
|
||||||
|
@ -175,7 +175,7 @@ Rg==
|
|||||||
(display "This is one line\n" (fdopen fd "wb")))
|
(display "This is one line\n" (fdopen fd "wb")))
|
||||||
|
|
||||||
(for-each-p
|
(for-each-p
|
||||||
"Checking ECDSA decryption"
|
"Checking ECDH decryption"
|
||||||
(lambda (test)
|
(lambda (test)
|
||||||
(lettmp (x y)
|
(lettmp (x y)
|
||||||
(call-with-output-file
|
(call-with-output-file
|
||||||
|
@ -29,3 +29,5 @@ Notes:
|
|||||||
such a file is created which is then directly followed by a separate
|
such a file is created which is then directly followed by a separate
|
||||||
armored public key block. To create such a sample concatenate
|
armored public key block. To create such a sample concatenate
|
||||||
pgp-desktop-skr.asc and E657FB607BB4F21C90BB6651BC067AF28BC90111.asc
|
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.
|
||||||
|
Loading…
x
Reference in New Issue
Block a user