Verifying user revocation certificate (DSA) fails

[toberndo]

One of my old test keys throws an exception in v4.0.1 with key.getPrimaryUser:
"TypeError: Cannot read property 'toBN' of undefined" at https://github.com/openpgpjs/openpgpjs/blob/master/src/crypto/signature.js#L49 when verifying a non-self user revoke certificate:

:user ID packet: "Passwort  \xc3\xa4\xc3\x84??^^12 2 \xc2\xb5  <pwd@test.com>"
# off=315 ctb=88 tag=2 hlen=2 plen=90
:signature packet: algo 17, keyid E1790A6F8C13574D
	version 4, created 1445956182, md5len 0, sigclass 0x30
	digest algo 10, begin of digest 0e 63
	hashed subpkt 2 len 4 (sig created 2015-10-27)
	hashed subpkt 29 len 18 (revocation reason 0x20 (Ich weis es nicht))
	subpkt 16 len 8 (issuer key ID E1790A6F8C13574D)
	data: [160 bits]
	data: [160 bits]
# off=407 ctb=89 tag=2 hlen=3 plen=306
:signature packet: algo 1, keyid 63649F53DA0BDE5C
	version 4, created 1421224226, md5len 0, sigclass 0x10
	digest algo 8, begin of digest fd ce
	hashed subpkt 2 len 4 (sig created 2015-01-14)
	hashed subpkt 11 len 5 (pref-sym-algos: 9 8 7 3 2)
	hashed subpkt 16 len 8 (issuer key ID 63649F53DA0BDE5C)
	hashed subpkt 21 len 3 (pref-hash-algos: 8 2 10)
	hashed subpkt 22 len 2 (pref-zip-algos: 2 1)
	hashed subpkt 27 len 1 (key flags: 03)
	hashed subpkt 30 len 1 (features: 01)
	data: [2046 bits]
# off=716 ctb=88 tag=2 hlen=2 plen=70
:signature packet: algo 17, keyid E1790A6F8C13574D
	version 4, created 1445955936, md5len 0, sigclass 0x10
	digest algo 10, begin of digest 33 48
	hashed subpkt 2 len 4 (sig created 2015-10-27)
	subpkt 16 len 8 (issuer key ID E1790A6F8C13574D)
	data: [156 bits]
	data: [160 bits]

Here is the key in question and the key that signed it:
Passwort äÄ??^^12 2 µ (DA0BDE5C) – public.asc.txt
Signing key public.asc.txt

[twiss]

OpenPGP.js currently doesn't support non-self revocation signatures, and never has. However, versions before 3.0.0 had a revCert.issuerKeyId.equals(certificate.issuerKeyId) check. If false, the revocation signature was ignored and the key was considered not revoked. This apparently got removed during refactoring.

Ideally we should provide a way to pass the "authorized revocation key", and use it to check the revocation signature. Another use case for passing "other keys" would be non-self key certifications, as e.g. discussed in the context of #740.

[toberndo]

OpenPGP.js currently doesn't support non-self revocation signatures, and never has. However, versions before 3.0.0 had a revCert.issuerKeyId.equals(certificate.issuerKeyId) check. If false, the revocation signature was ignored and the key was considered not revoked. This apparently got removed during refactoring.

Just to be clear to not mix up user revocation and key revocation: in the test keys above there is a user of a key that was signed by another key, and then later the other key also attached a revocation certificate to that user, thereby revoking the initial signature. But the other key could never revoke the first key itself. Except for the fact that https://tools.ietf.org/html/rfc4880#section-5.2.3.15 allows to specify a third-party revocation key.

Non-self user revocation signatures are not relevant for User.prototype.isRevoked, so I think it would be good to to filter in isDataRevoked first on all rev certificates that are relevant (by issuerKeyId) instead of verifying everything and relying on that the not relevant rev certs will fails to verify.

So verify shouldn't have been called for the non-self user revoke certificate in the test key above in the first place, but when it is called it shouldn't throw an exception.

Non-self user revocation certificates are then only relevant for User.prototype.verifyAllCertifications, but this feature is not supported in OpenPGP.js v2-4.

Ideally we should provide a way to pass the "authorized revocation key", and use it to check the revocation signature. Another use case for passing "other keys" would be non-self key certifications, as e.g. discussed in the context of #740.

I see "authorized revocation key" only in the context of https://tools.ietf.org/html/rfc4880#section-5.2.3.15. Had only a brief look at #740 but my tendency here is to implement such features on the application level and not in the library.

[toberndo]

Unrelated: would be good to have the info about deprecation of V3 keys in OpenPGP.js in the v4 release notes.

[twiss]

Thanks for the clarification. You're right that in this case the revocation signature is irrelevant and should be ignored.

Non-self user revocation signatures are not relevant for User.prototype.isRevoked

Revocation signatures from authorized keys are, right? AFAICT those could revoke the user binding self-signature, and they would have the same signature type and appear in the same place. Hopefully those would have a Signature Target subpacket, otherwise I don't quite see how to easily distinguish them.

I'm also curious what GPG does when it encounters such a signature but doesn't have the signing key. (I know this is a bit out of scope for this issue, I'm just trying to make sure we don't accidentally accept keys which GPG would reject, for example.)

would be good to have the info about deprecation of V3 keys in OpenPGP.js in the v4 release notes.

Thanks, done.

[toberndo]

Revocation signatures from authorized keys are, right? AFAICT those could revoke the user binding self-signature, and they would have the same signature type and appear in the same place. Hopefully those would have a Signature Target subpacket, otherwise I don't quite see how to easily distinguish them.

Right, authorized keys are mentioned here:

0x30: Certification revocation signature
       This signature revokes an earlier User ID certification signature
       (signature class 0x10 through 0x13) or direct-key signature
       (0x1F).  It should be issued by the same key that issued the
       revoked signature or an authorized revocation key.  The signature
       is computed over the same data as the certificate that it
       revokes, and should have a later creation date than that
       certificate.

I'm also curious what GPG does when it encounters such a signature but doesn't have the signing key.

Interesting, I would say if you are not able to verify the revoke signature then it should not be seen as valid. But I can try to find out how this is handled in GPG.

Thanks for adding v3 keys to the release notes!

[toberndo]

I'm also curious what GPG does when it encounters such a signature but doesn't have the signing key.

From: https://dev.gnupg.org/source/gnupg/browse/master/g10/sig-check.c;99c17b970bc0ca7e0cff7fe031c6f9feb05af3ff$639

* We are careful to make sure that GPG_ERR_NO_PUBKEY is
* only returned when a revocation signature is from a valid
* revocation key designated in a revkey subpacket, but the revocation
* key itself isn't present.

[twiss]

Thanks for looking into it. I think we should do the same thing, mainly because if someone uploads a revocation certificate like that to a keyserver, they'd expect the user ID to no longer be used.

I'd personally be fine with simply throwing "Authorized revocation keys are currently not supported" in isDataRevoked when the revocation signature's issuerKeyId is mentioned by the self-signature's Revocation Key subpacket, and ignoring third party revocation signatures if not.

Off topic, but another interesting comment from getkey.c is:

  /* Note: we allow invalidation of cert revocations
   * by a newer signature.  An attacker can't use this
   * because a key should be revoked with a key revocation.
   * The reason why we have to allow for that is that at
   * one time an email address may become invalid but later
   * the same email address may become valid again (hired,
   * fired, hired again).  */

AFAICT we don't do that, so there are probably some keys out there which GPG accepts and we reject.

[toberndo]

I'd personally be fine with simply throwing "Authorized revocation keys are currently not supported" in isDataRevoked when the revocation signature's issuerKeyId is mentioned by the self-signature's Revocation Key subpacket, and ignoring third party revocation signatures if not.

I'm wondering, at the time you are able to throw the exception you have almost done everything required to support this feature: 1. detect Revocation Key subpacket, 2. check issuerKeyId equals revocation key, 3. verify revocation signature. Given that you have the revocation key to verify, if not then the revocation would be ignored.

AFAICT we don't do that, so there are probably some keys out there which GPG accepts and we reject.

Yes, I think this could be implemented in the course of a revised getLatestSignature method as described in #762
So if the newest certificate of an issuer is valid, you don't need to check further if there are other certificates from this issuer (like revoke).

[twiss]

I'm wondering, at the time you are able to throw the exception you have almost done everything required to support this feature: 1. detect Revocation Key subpacket, 2. check issuerKeyId equals revocation key, 3. verify revocation signature. Given that you have the revocation key to verify, if not then the revocation would be ignored.

The Revocation Key subpacket only contains the fingerprint of the revocation key, not the key itself. So we'd also need a way to pass additional keys, and then select the revocation key from those by fingerprint (we already have KeyArray.prototype.getForId for that last part).

[toberndo]

But you have already the key parameter in isDataRevoked that is used to verify the revocation signature and this is also the key that corresponds to the the fingerprint of the revocation key in the subpacket, no?

[twiss]

Currently, getPrimaryUser calls user.isRevoked(primaryKey, cert, null, date) where null is the key parameter which gets passed to isDataRevoked, which makes it default to primaryKey, which is not the signing key in this case.

[sanjanarajan]

Hi, just catching up on this issue.
Is everyone okay with the following approach? @toberndo @twiss

When evaluating a non-self revocation signature:

1. If the issuer is not present in the revocation key subpacket, and is therefore not an authorized revocation key, disregard this signature in terms of determining whether the user is valid.

2. If the issuer is present in the revocation key subpacket:

   • Check to see which signature the Signature Target subpacket indicates is being revoked

     • if there is no target subpacket OR it is revoking the binding self-signature, throw an error stating that we do not support authorized revocation keys (and consider implementing this in the future so that, as @twiss said, we can properly verify these signatures with the appropriate keys if passed in by the user)

     • if it is just revoking another non-self signature, then disregard this signature in terms of determining whether the user is valid

[toberndo]

@sanjanarajan I think 2. we can only implement if we have the issuer key. Any information in a signature should not be seen as valid unless the signature has been verified. So the issuer keyid in the signature is not a valid information if we don't have the corresponding key. I would propose to not throwing an error here, because we can't verify the claim of the revocation signature.

[sanjanarajan]

Any information in a signature should not be seen as valid unless the signature has been verified, but it is also dangerous to not even provide the user the opportunity to verify this signature. So I agree that we should just implement the full 2. solution allowing other issuer keys to be passed in.

If, however, we implement this full solution and a user does not pass in the issuer key, then I do think we need to throw an error here. Otherwise, this completely defeats the purpose of authorized revocation keys.

[toberndo]

Not sure what this means for the interfaces. So let's say we add the issuer key parameter to User.isRevoked then one would need to check before calling this method if a revocation key subpacket exists, and pass over the key. If I don't have the key, and the method throws an exception, this will prevent that I get a status for this user. I still think that in such a case the status should be not-revoked. "innocent until proven guilty".

It gets worse when these errors are propagated to getPrimaryUser or if there would be the requirement to first do any of these checks before calling getPrimaryUser.

So overall, this feature adds quite some complexity. Probably it would be good to have a more fine grained error system in OpenPGP.js than the binary success/exception for such cases. Do we have a real world requirement for this feature? I don't know of any PGP UI applications that allows to generate a key with a revocation key subpacket or a signature target subpacket.

Also the initial problem reported in this issue was related to the problem that if another person signs one user of my key, then they can also revoke such a signature, which would be much easier to implement than the revocation key subpacket feature.

[sanjanarajan]

As far as a "real world requirement", I don't know of one off-hand, but given that this is specified in the RFC as a valid method of revocation and the same case is handled in GPG by throwing an error, it's certainly the safer choice not to accept these keys.

Anyway, while I agree that this feature adds some complexity (though this complexity is inherent in the RFC), it could be as simple as allowing an optional array of verification keys to be passed in and used whenever necessary. The checks could simply be done in the same places in the code as the current self certificate verifications.

Also the initial problem reported in this issue was related to the problem that if another person signs one user of my key, then they can also revoke such a signature, which would be much easier to implement than the revocation key subpacket feature.

I'm not sure exactly what you mean here. What I proposed in my previous comment for a case like this (Case 1) is that these certifications be ignored when determining user validity, as whether or not they verify is irrelevant to user validity anyway. Is this not the desired behavior?

[toberndo]

As far as a "real world requirement", I don't know of one off-hand, but given that this is specified in the RFC as a valid method of revocation and the same case is handled in GPG by throwing an error, it's certainly the safer choice not to accept these keys.

I talked to Werner from the GnuPG team how this situation is handled and he said that it's a very rarely used feature. It could happen that the designated revocation key is missing and this situation should trigger a warning.
So although we see a GPG_ERR_NO_PUBKEY in the GnuPG sources, a gpg --encrypt --recipient test@demo.com should succeed in the described situation with missing designated revocation key.
But I haven't tested this.

Anyway, while I agree that this feature adds some complexity (though this complexity is inherent in the RFC), it could be as simple as allowing an optional array of verification keys to be passed in and used whenever necessary. The checks could simply be done in the same places in the code as the current self certificate verifications.

This would spread out to all consumers of User.isRevoked. I even would need to provide this optional array when calling key.getExpirationTime.

I'm not sure exactly what you mean here. What I proposed in my previous comment for a case like this (Case 1) is that these certifications be ignored when determining user validity, as whether or not they verify is irrelevant to user validity anyway. Is this not the desired behavior?

Yes, they are not relevant for the user validity. I just wanted to point out that my initial post (#765 (comment)) in this issue is neither about Case 1 or 2 and what I described as "non-self user revoke certificate" is a certificate from a person that revokes a previous certificate of that same person on a user id.