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 32893a5
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. Just ignore any. */

This comment has been minimized.

Sign in to view

Copy link
@nwalfield

nwalfield Nov 23, 2022

Contributor

due to their
*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([

This comment has been minimized.

Sign in to view

Copy link
@nwalfield

nwalfield Nov 23, 2022

Contributor

This (and the other tests) can be changed to AT_CHECK. According to the documentation

The difference between AT_CHECK and AT_CHECK_UNQUOTED is that only the latter performs shell variable expansion (‘$’), command substitution (‘`’), and backslash escaping (‘\’) on comparison text given in the stdout and stderr arguments

That functionality is not longer used.
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])

This comment has been minimized.

Sign in to view

Copy link
@nwalfield

nwalfield Nov 23, 2022

Contributor

This fails on the sequoia backend:

#                             -*- compilation -*-
281. rpmsigdig.at:605: testing rpmkeys type confusion ...
/rpmsigdig.at:606:


if ! [ -d testing/ ]; then
    cp -aP "${RPMTEST}" .
    chmod -R u+w testing/
    mkdir -p testing/build
    ln -s ../data/SOURCES testing/build/
fi
export RPMTEST="${PWD}/testing"
export TOPDIR="${RPMTEST}/build"
export HOME="${RPMTEST}"

rm -rf "${RPMTEST}"`rpm --eval '%_dbpath'`/*
runroot rpm --initdb


runroot rpmkeys --import /data/keys/type-confusion.asc

--- /dev/null   2022-11-10 14:10:26.774000000 +0000
+++ /home/neal/rpm/_build/tests/rpmtests.dir/at-groups/281/stderr       2022-11-23 14:59:52.108283929 +0000
@@ -0,0 +1,2 @@
+warning: Certificate 4344591E1964C5FC:
+  Policy rejects subkey 185E6146F00650F8: No binding signature at time 2022-11-23T14:59:52Z
281. rpmsigdig.at:605: 281. rpmkeys type confusion (rpmsigdig.at:605): FAILED (rpmsigdig.at:606)

As per the previous discussion here, I think this should be skipped on Sequoia.
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

1 comment on commit 32893a5

@nwalfield
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still haven't wrapped my head around the internal pgp parser, so I did not thoroughly review pgpPrtParams. Perhaps @DemiMarie can look at the changes to that function. Otherwise, I have no issues with this commit.

Please sign in to comment.