Skip to content

Commit

Permalink
Drop subkey support from the internal OpenPGP parser
Browse files Browse the repository at this point in the history 
The world of OpenPGP subkeys is scarily complicated and we're failing at
it in too many ways to count. People who need the complexity of a full
implementation can use the Sequoia backend, meanwhile this tames the
internal parser back to something we may be able to support.

Update tests accordingly - as a nice side-effect this makes the internal
parser behave the same as Sequoia for these cases.

Fixes: #2278
  • Loading branch information
@pmatilai
pmatilai committed Nov 18, 2022
1 parent 9fe0a09 commit ca75034
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 227 deletions.
192 changes: 8 additions & 184 deletions rpmio/rpmpgp_internal.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,7 @@ static int pgpCurveByOid(const uint8_t *p, int l)

static int isKey(pgpDigParams keyp)
{
return keyp->tag == PGPTAG_PUBLIC_KEY || keyp->tag == PGPTAG_PUBLIC_SUBKEY;
return keyp->tag == PGPTAG_PUBLIC_KEY;
}

static int pgpPrtPubkeyParams(uint8_t pubkey_algo,
Expand Down Expand Up @@ -889,77 +889,6 @@ static pgpDigParams pgpDigParamsNew(uint8_t tag)
return digp;
}

static int hashKey(DIGEST_CTX hash, const struct pgpPkt *pkt, int exptag)
{
int rc = -1;
if (pkt->tag == exptag) {
uint8_t head[] = {
0x99,
(pkt->blen >> 8),
(pkt->blen ),
};

rpmDigestUpdate(hash, head, 3);
rpmDigestUpdate(hash, pkt->body, pkt->blen);
rc = 0;
}
return rc;
}

static int pgpVerifySelf(pgpDigParams key, pgpDigParams selfsig,
const struct pgpPkt *all, int i)
{
int rc = -1;
DIGEST_CTX hash = NULL;

switch (selfsig->sigtype) {
case PGPSIGTYPE_SUBKEY_BINDING:
hash = rpmDigestInit(selfsig->hash_algo, 0);
if (hash) {
rc = hashKey(hash, &all[0], PGPTAG_PUBLIC_KEY);
if (!rc)
rc = hashKey(hash, &all[i-1], PGPTAG_PUBLIC_SUBKEY);
}
break;
default:
/* ignore types we can't handle */
rc = 0;
break;
}

if (hash && rc == 0)
rc = pgpVerifySignature(key, selfsig, hash);

rpmDigestFinal(hash, NULL, NULL, 0);

return rc;
}

static int parseSubkeySig(const struct pgpPkt *pkt, uint8_t tag,
pgpDigParams *params_p) {
pgpDigParams params = *params_p = NULL; /* assume failure */

if (pkt->tag != PGPTAG_SIGNATURE)
goto fail;

params = pgpDigParamsNew(tag);

if (pgpPrtSig(tag, pkt->body, pkt->blen, params))
goto fail;

if (params->sigtype != PGPSIGTYPE_SUBKEY_BINDING &&
params->sigtype != PGPSIGTYPE_SUBKEY_REVOKE)
{
goto fail;
}

*params_p = params;
return 0;
fail:
pgpDigParamsFree(params);
return -1;
}

static const size_t RPM_MAX_OPENPGP_BYTES = 65535; /* max number of bytes in a key */

int pgpPrtParams(const uint8_t * pkts, size_t pktlen, unsigned int pkttype,
Expand All @@ -968,19 +897,13 @@ int pgpPrtParams(const uint8_t * pkts, size_t pktlen, unsigned int pkttype,
const uint8_t *p = pkts;
const uint8_t *pend = pkts + pktlen;
pgpDigParams digp = NULL;
pgpDigParams selfsig = NULL;
int i = 0;
int alloced = 16; /* plenty for normal cases */
struct pgpPkt _pkt, *pkt = &_pkt;
int rc = -1; /* assume failure */
int expect = 0;
int prevtag = 0;

if (pktlen > RPM_MAX_OPENPGP_BYTES)
return rc; /* reject absurdly large data */

struct pgpPkt *all = xmalloc(alloced * sizeof(*all));
while (p < pend) {
struct pgpPkt *pkt = &all[i];
if (decodePkt(p, (pend - p), pkt))
break;

Expand All @@ -993,48 +916,16 @@ int pgpPrtParams(const uint8_t * pkts, size_t pktlen, unsigned int pkttype,
} else if (pkt->tag == PGPTAG_PUBLIC_KEY)
break;

if (expect) {
if (pkt->tag != expect)
break;
selfsig = pgpDigParamsNew(pkt->tag);
}

if (pgpPrtPkt(pkt, selfsig ? selfsig : digp))
if (pgpPrtPkt(pkt, digp))
break;

if (selfsig) {
/* subkeys must be followed by binding signature */
int xx = 1; /* assume failure */

if (!(prevtag == PGPTAG_PUBLIC_SUBKEY &&
selfsig->sigtype != PGPSIGTYPE_SUBKEY_BINDING))
xx = pgpVerifySelf(digp, selfsig, all, i);

selfsig = pgpDigParamsFree(selfsig);
if (xx)
break;
expect = 0;
}

if (pkt->tag == PGPTAG_PUBLIC_SUBKEY)
expect = PGPTAG_SIGNATURE;
prevtag = pkt->tag;

i++;
p += (pkt->body - pkt->head) + pkt->blen;
if (pkttype == PGPTAG_SIGNATURE)
break;

if (alloced <= i) {
alloced *= 2;
all = xrealloc(all, alloced * sizeof(*all));
}
}

rc = (digp && (p == pend) && expect == 0) ? 0 : -1;
rc = (digp && (p == pend)) ? 0 : -1;

free(all);
selfsig = pgpDigParamsFree(selfsig);
if (ret && rc == 0) {
*ret = digp;
} else {
Expand All @@ -1047,77 +938,10 @@ int pgpPrtParamsSubkeys(const uint8_t *pkts, size_t pktlen,
pgpDigParams mainkey, pgpDigParams **subkeys,
int *subkeysCount)
{
const uint8_t *p = pkts;
const uint8_t *pend = pkts + pktlen;
pgpDigParams *digps = NULL;
int count = 0;
int alloced = 10;
struct pgpPkt pkt;
int rc, i;

digps = xmalloc(alloced * sizeof(*digps));

while (p < pend) {
if (decodePkt(p, (pend - p), &pkt))
break;

p += (pkt.body - pkt.head) + pkt.blen;

if (pkt.tag == PGPTAG_PUBLIC_SUBKEY) {
if (count == alloced) {
alloced <<= 1;
digps = xrealloc(digps, alloced * sizeof(*digps));
}

digps[count] = pgpDigParamsNew(PGPTAG_PUBLIC_SUBKEY);
/* Copy UID from main key to subkey */
digps[count]->userid = xstrdup(mainkey->userid);

if (getKeyID(pkt.body, pkt.blen, digps[count]->signid)) {
pgpDigParamsFree(digps[count]);
continue;
}

if (pgpPrtKey(pkt.tag, pkt.body, pkt.blen, digps[count])) {
pgpDigParamsFree(digps[count]);
continue;
}

pgpDigParams subkey_sig = NULL;
if (decodePkt(p, pend - p, &pkt) ||
parseSubkeySig(&pkt, 0, &subkey_sig))
{
pgpDigParamsFree(digps[count]);
break;
}

/* Is the subkey revoked or incapable of signing? */
int ignore = subkey_sig->sigtype != PGPSIGTYPE_SUBKEY_BINDING ||
!((subkey_sig->saved & PGPDIG_SIG_HAS_KEY_FLAGS) &&
(subkey_sig->key_flags & 0x02));
if (ignore) {
pgpDigParamsFree(digps[count]);
} else {
digps[count]->key_flags = subkey_sig->key_flags;
digps[count]->saved |= PGPDIG_SIG_HAS_KEY_FLAGS;
count++;
}
p += (pkt.body - pkt.head) + pkt.blen;
pgpDigParamsFree(subkey_sig);
}
}
rc = (p == pend) ? 0 : -1;

if (rc == 0) {
*subkeys = xrealloc(digps, count * sizeof(*digps));
*subkeysCount = count;
} else {
for (i = 0; i < count; i++)
pgpDigParamsFree(digps[i]);
free(digps);
}

return rc;
/* We don't support subkeys due their complexities */
*subkeys = NULL;
*subkeysCount = 0;
return 0;
}

rpmRC pgpVerifySignature(pgpDigParams key, pgpDigParams sig, DIGEST_CTX hashctx)
Expand Down
56 changes: 13 additions & 43 deletions tests/rpmsigdig.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -510,61 +510,35 @@ Checking package after importing key, no signature:
[])
AT_CLEANUP

AT_SETUP([rpmkeys --import invalid keys])
AT_KEYWORDS([rpmkeys import])
RPMDB_INIT

# The following certificates include subkeys with errors. In general,
# all subkeys should be returned whether they are valid or not. Keys
# should only be rejected when verifying a signature. This is because
# a key's validity depends on the current time. Thus, the time to
# check for validity is when the key is used, not when it is imported.
# The internal OpenPGP implementation checks for validity when the key
# is imported; other implementations should not do this.
AT_SETUP([rpmkeys --import invalid keys])
AT_KEYWORDS([rpmkeys import])
RPMDB_INIT

AT_CHECK_UNQUOTED([
runroot rpmkeys --quiet --import /data/keys/CVE-2021-3521-badbind.asc
echo exit code: $? >&2
],
[ignore],
[0],
[],
[`if test x$PGP = xinternal;
then
echo 'error: /data/keys/CVE-2021-3521-badbind.asc: key 1 import failed.'
echo exit code: 1
else
echo exit code: 0
fi`]
)
[])

AT_CHECK_UNQUOTED([
runroot rpmkeys --quiet --import /data/keys/CVE-2021-3521-nosubsig.asc
echo exit code: $? >&2
],
[ignore],
[0],
[],
[`if test x$PGP = xinternal;
then
echo 'error: /data/keys/CVE-2021-3521-nosubsig.asc: key 1 import failed.'
echo exit code: 1
else
echo exit code: 0
fi`]
)
[])

AT_CHECK_UNQUOTED([
runroot rpmkeys --quiet --import /data/keys/CVE-2021-3521-nosubsig-last.asc
echo exit code: $? >&2
],
[ignore],
[0],
[],
[`if test x$PGP = xinternal;
then
echo 'error: /data/keys/CVE-2021-3521-nosubsig-last.asc: key 1 import failed.'
echo exit code: 1
else
echo exit code: 0
fi`]
)
[])
AT_CLEANUP

# -----------------------------------------
Expand Down Expand Up @@ -627,20 +601,16 @@ gpg(a72b7d4f62837bea) = 4:a72b7d4f62837bea-62553e62
[])
AT_CLEANUP

# The internal OpenPGP parser rejects the key. The Sequoia parser
# just ignores the invalid components and imports the rest. This test
# checks the behavior of the internal parser.
# The key has invalid components but those should be just ignored.
AT_SETUP([rpmkeys type confusion])
AT_SKIP_IF([test x$PGP != xinternal])
AT_CHECK([
RPMDB_INIT

runroot rpmkeys --import /data/keys/type-confusion.asc
],
[1],
[0],
[],
[error: /data/keys/type-confusion.asc: key 1 import failed.]
)
[])
AT_CLEANUP

# ------------------------------
Expand Down

0 comments on commit ca75034

Please sign in to comment.