crash at pgp_subkey_refresh_data

[mkmelin]

Description

Crash - see https://bugzilla.mozilla.org/show_bug.cgi?id=1637046 @ https://searchfox.org/comm-central/rev/32028983d85d767411a1487ed7b8394e00865b35/third_party/rnp/src/lib/pgp-key.cpp#1319

Looks like sub->revocation is null

0 librnp.so pgp_subkey_refresh_data(pgp_key_t, pgp_key_t) comm/third_party/rnp/src/lib/pgp-key.cpp:1319 context 1 librnp.so rnp_key_store_add_key(rnp_key_store_t, pgp_key_t) comm/third_party/rnp/src/librekey/rnp_key_store.cpp:530

[ni4]

@mkmelin Thanks for the report! Do you use the latest master's commit?

[ni4]

@mkmelin Sorry, no need to answer - we definitely should not call strlen() on possible NULL.

[ni4]

Some more details:

• the same applies to the primary key
• we should add a test with both primary key/subkey revocation signatures which doesn't contain revocation reason subpacket.

[pbrunschwig]

There's at least one more occurrence of that bug: (see https://crash-stats.mozilla.org/report/index/bb8bc9ba-f139-469d-9bf6-49a600200523)

pgp_key.cpp, line 1388:

signature_get_revocation_reason(&sig->sig, &revocation->code, &revocation->reason);
        if (!strlen(revocation->reason)) {
            free(revocation->reason);
            revocation->reason = strdup(pgp_str_from_map(revocation->code, ss_rr_code_map));
        }

[ni4]

@pbrunschwig @mkmelin This issue should be fixed since PR #1152 was merged. Could you please confirm this?

[kaie]

@ni4 Building Thunderbird is a bigger task, and Patrick probably doesn't do it himself. I'm guessing that for testing, Patrick relies on the automatic nightly development builds that the Thunderbird project provides.

We don't pick up new RNP snapshots automatically, it's a manual process. @ni4 whenever you think you have made progress and now is a good time to pick up a new RNP snapshot, please let me know in some way, and we trigger a rebase to a newer RNP snapshot.

I've started that process yesterday https://bugzilla.mozilla.org/show_bug.cgi?id=1641612

[kaie]

If someone can point me to an example key that triggers the crash, I can try to test locally.

[ni4]

@kaie Got it, I thought the integration process doesn't take much time/more automated. Then probably it worth to wait for a few days - so I will finish more issues with key import. Btw, are we on schedule with the TB beta release/any other blocking issues?

[ni4]

If someone can point me to an example key that triggers the crash, I can try to test locally.

I added test with some keys which should expose the problem, just to confirm whether originally reported keys work well now.

[kaie]

Btw, are we on schedule with the TB beta release/any other blocking issues?

We're considering to declare the TB OpenPGP support as experimental for a little longer, because we have some more work to do in TB's integration code, too. https://mail.mozilla.org/pipermail/tb-planning/2020-May/007627.html - so we have a little more time for RNP fixes.

Issue https://github.com/rnpgp/rnp/issues/1142 is very important. Ideally, while you're working on that, it would be very helpful to get an API for https://github.com/rnpgp/rnp/issues/1107 too, to obtain the encryption algorithms that were in use. I'll add another idea to 1142 shortly.

[ni4]

@kaie Great, thanks for the clarification.

[pbrunschwig]

The crash is gone - confirmed in the latest Thunderbird nightly build

[ni4]

@pbrunschwig Thanks for the update!