From ec81c437e71b4c630a799ed29447cc5e3db162cd Mon Sep 17 00:00:00 2001 From: Werner Koch Date: Mon, 30 Sep 2019 14:08:13 +0200 Subject: [PATCH] gpg: Fix expand GPG groups when resolving a key * g10/expand-group.c (expand_group): Add arg prepend_input. * g10/pkclist.c (build_pk_list): Adjust for it. * g10/getkey.c (key_byname): Keep the expanded names in the CTX and don't premature free them. (get_pubkey_byname): Append the namelist to the extra_list. -- The original patch didn't kept the expanded list in the context and also would duplicate names which are not group names. The latter does not really harm but the former lead to a use after free. Original patch was applied just a few weeks ago. Fixes-commit: e825aea2ba3529c333d7ec2c76e63998cb83d999 Signed-off-by: Werner Koch --- g10/expand-group.c | 33 ++++++++++++++++++++++-------- g10/getkey.c | 51 ++++++++++++++++++++++++++-------------------- g10/keydb.h | 2 +- g10/pkclist.c | 2 +- 4 files changed, 55 insertions(+), 33 deletions(-) diff --git a/g10/expand-group.c b/g10/expand-group.c index e09a4fff6..ec9c8a2ef 100644 --- a/g10/expand-group.c +++ b/g10/expand-group.c @@ -53,21 +53,36 @@ expand_id (const char *id, strlist_t *into, unsigned int flags) } /* For simplicity, and to avoid potential loops, we only expand once - - * you can't make an alias that points to an alias. */ + * you can't make an alias that points to an alias. If PREPEND_INPUT + * is true each item from INPUT is prepended to the new list; if it is + * false the original item from INPUT is only added if no group + * existed for it. */ strlist_t -expand_group (strlist_t input) +expand_group (strlist_t input, int prepend_input) { strlist_t output = NULL; strlist_t sl, rover; for (rover = input; rover; rover = rover->next) - if (!(rover->flags & PK_LIST_FROM_FILE) - && !expand_id (rover->d, &output, rover->flags)) - { - /* Didn't find any groups, so use the existing string */ - sl = add_to_strlist (&output, rover->d); - sl->flags = rover->flags; - } + { + if ((rover->flags & PK_LIST_FROM_FILE)) + continue; + if (!expand_id (rover->d, &output, rover->flags)) + { + /* Didn't find any groups, so use the existing string unless + * we will anyway add it due to the prepend flag. */ + if (!prepend_input) + { + sl = add_to_strlist (&output, rover->d); + sl->flags = rover->flags; + } + } + if (prepend_input) + { + sl = add_to_strlist (&output, rover->d); + sl->flags = rover->flags; + } + } return output; } diff --git a/g10/getkey.c b/g10/getkey.c index de5024198..57079fa4d 100644 --- a/g10/getkey.c +++ b/g10/getkey.c @@ -729,7 +729,8 @@ key_byname (ctrl_t ctrl, GETKEY_CTX *retctx, strlist_t namelist, { int rc = 0; int n; - strlist_t r, namelist_expanded = NULL, link = NULL; + strlist_t r; + strlist_t namelist_expanded = NULL; GETKEY_CTX ctx; KBNODE help_kb = NULL; KBNODE found_key = NULL; @@ -758,18 +759,8 @@ key_byname (ctrl_t ctrl, GETKEY_CTX *retctx, strlist_t namelist, } else { - namelist_expanded = expand_group (namelist); - - /* Chain namelist and namelist_expanded */ - for (r = namelist; r; r = r->next) - { - if (!r->next) - { - r->next = namelist_expanded; - link = r; - break; - } - } + namelist_expanded = expand_group (namelist, 1); + namelist = namelist_expanded; /* Build the search context. */ for (n = 0, r = namelist; r; r = r->next) @@ -832,7 +823,18 @@ key_byname (ctrl_t ctrl, GETKEY_CTX *retctx, strlist_t namelist, release_kbnode (help_kb); if (retctx) /* Caller wants the context. */ - *retctx = ctx; + { + if (ctx->extra_list) + { + for (r=ctx->extra_list; r->next; r = r->next) + ; + r->next = namelist_expanded; + } + else + ctx->extra_list = namelist_expanded; + namelist_expanded = NULL; + *retctx = ctx; + } else { if (ret_kdbhd) @@ -843,12 +845,8 @@ key_byname (ctrl_t ctrl, GETKEY_CTX *retctx, strlist_t namelist, getkey_end (ctrl, ctx); } -leave: - if (namelist_expanded) - free_strlist(namelist_expanded); - /* Un-chain namelist and namelist_expanded */ - if (link) - link->next = NULL; + leave: + free_strlist (namelist_expanded); return rc; } @@ -1193,8 +1191,17 @@ get_pubkey_byname (ctrl_t ctrl, enum get_pubkey_modes mode, if (retctx && *retctx) { - log_assert (!(*retctx)->extra_list); - (*retctx)->extra_list = namelist; + GETKEY_CTX ctx = *retctx; + strlist_t sl; + + if (ctx->extra_list) + { + for (sl=ctx->extra_list; sl->next; sl = sl->next) + ; + sl->next = namelist; + } + else + ctx->extra_list = namelist; (*retctx)->found_via_akl = mechanism_type; } else diff --git a/g10/keydb.h b/g10/keydb.h index f94b14659..764cce98d 100644 --- a/g10/keydb.h +++ b/g10/keydb.h @@ -267,7 +267,7 @@ int check_signatures_trust (ctrl_t ctrl, PKT_signature *sig); void release_pk_list (PK_LIST pk_list); int expand_id (const char *id, strlist_t *into, unsigned int flags); -strlist_t expand_group (strlist_t input); +strlist_t expand_group (strlist_t input, int prepend_input); int build_pk_list (ctrl_t ctrl, strlist_t rcpts, PK_LIST *ret_pk_list); gpg_error_t find_and_check_key (ctrl_t ctrl, const char *name, unsigned int use, diff --git a/g10/pkclist.c b/g10/pkclist.c index 996b3ba6e..9ebfb131d 100644 --- a/g10/pkclist.c +++ b/g10/pkclist.c @@ -904,7 +904,7 @@ build_pk_list (ctrl_t ctrl, strlist_t rcpts, PK_LIST *ret_pk_list) /* Try to expand groups if any have been defined. */ if (opt.grouplist) - remusr = expand_group (rcpts); + remusr = expand_group (rcpts, 0); else remusr = rcpts;