ProtonMail/go-crypto

(Optionally?) skip HashTag quick test on verification

Closed this issue ยท 7 comments

As discussed at https://mailarchive.ietf.org/arch/msg/openpgp/Zv8v1IwbbXYiDPKytfbUmtNdxak/ and hockeypuck/hockeypuck#187 (comment) , it appears that go-crypto is in the minority of implementations that fast-fails on the HashTag test at

if hashBytes[0] != sig.HashTag[0] || hashBytes[1] != sig.HashTag[1] {
:

	hashBytes := signed.Sum(nil)
	if hashBytes[0] != sig.HashTag[0] || hashBytes[1] != sig.HashTag[1] {
		return errors.SignatureError("hash tag doesn't match")
	}

Several known keys in the wild fail on this test yet successfully validate in other implementations, including GnuPG and Sequoia.

While go-crypto is technically correct here, the fast-fail test is not required by the standard and is sufficiently broken elsewhere that its usefulness is questionable.

twiss commented

I don't think go-crypto is in the minority, as OpenPGP.js and rnp also do this check. To be perfectly honest, I'm a bit surprised that GnuPG and Sequoia don't (I wasn't aware of that), and arguably that has enabled these keys to exist in the wild. However, of course it's too late to prevent that now. So, depending on the result of the discussion on the mailing list, we could remove this check, yeah.

Just wanted to mention that I stumbled across this because https://github.com/web-flow.gpg (the key used to sign all GitHub web commits) exhibits this issue. We're using Flux, require GPG signed commits, and want to allow the GitHub web key, but Flux rejects commits signed by this key with invalid signature: hash tag doesn't match.

Edit: I see this has already been discussed in detail on the mailing list ๐Ÿ˜„

This was discussed to death on the WG list and consensus on a standard treatment of these malformed sigs was not forthcoming. We're back to the status quo ante: a small number of keys are malformed in a way that is technically incorrect but not ambiguous, and several common client implementations happily overlook these imperfections. While go-crypto is perfectly entitled to invalidate these sigs, it means that hockeypuck has no way to distribute these keys other than by duplicating of a significant chunk of code, which IMO is unmaintainable.

@twiss any further thoughts?

twiss commented

Hey ๐Ÿ‘‹ Since the draft spec now says

When verifying a v6 signature, an implementation MUST reject the signature if these octets don't match the first two octets of the computed hash.

I would be OK with disabling this check for v4 signatures.

Thanks, I'll make a new PR with such a version check in the next couple of days.

Note that since main still does not include v6 support, the linked PR tests sig.Version == 5. This will obviously need to be updated at some point but I have not done so here.

twiss commented

๐Ÿ‘ Thanks!