GPG Non-hQ Prefix Results In Invalid HMAC

[DigitalDJ]

So, I just spent several hours trying to figure out why fwknop-server kept rejecting my new GPG keyring.

It turns out the base64:hQ= magic number for GPG is not static. It can change. In fact, it's not really a magic number, it's a bitfield in the OpenPGP Packet Header (see https://tools.ietf.org/html/rfc4880#section-4.2).

In this case, I built a new keyring using GnuPG 2.1.17 on Win32. I also compiled fwknop on Windows using VC14 and got it working with gpgme.

In my case, the fwknop-client is transferring a header of "hL" (0x85 vs 0x84). Looking at that bits that have changed.....part of the header has two bits that define the length of the packet-length field in the header.

I have not tested it, but I would assume forcing a fixed header of hQ onto the GPG encrypted message would possibly cause the decryption to fail, if these do not match. Additionally, the fwknop HMAC fails since the client creates the HMAC using base64:hL and the server is verifying the HMAC using base64:hQ.

I propose that in the case of GPG messages, the prefix should not be striped, since it is NOT a static header, but instead a bitfield. Additionally, to clean things up, it would be preferable if the HMAC were generated based upon the encrypted text that is sent over the wire. Not something that is reconstructed.

I have looked at the source code and I propose the following attached patch. In the case of Rijndael encryption, I agree the "Salted__" prefix should still be striped to save bytes, but this should be taken care of during the encrypt/decrypt functions, so the HMAC can be calculated based on what is sent over the wire.

The only part I am uncertain of is the comment in incoming_spa.c:

    /* Ignore any SPA packets that contain the Rijndael or GnuPG prefixes
     * since an attacker might have tacked them on to a previously seen
     * SPA packet in an attempt to get past the replay check.  And, we're
     * no worse off since a legitimate SPA packet that happens to include
     * a prefix after the outer one is stripped off won't decrypt properly
     * anyway because libfko would not add a new one.

My assumption is that since we are no longer munging the GnuPG payload, we no longer have to worry about a replay attack. The packet will be added to the replay cache and adding or removing additional GnuPG headers will cause decryption of the packet to fail. Since we are still stripping the Rijndael header, the check for that is still required due to the way add_salted_str (cipher_funcs.c) does not add the header if it already exists. (i.e. the replayed packet will be a cache miss since it actually contains different data, i.e. the Salted__ string) Is this correct?

[DigitalDJ]

Sorry here is the attached patch. Untested thus far. fwknop-fix-hmac-validation.txt

EDIT: Just gave this a quick test, and seems to work.

If you think this is sane, I'll submit a pull request. Cheers

[mrash]

Thanks for the analysis. Agreed that a non-static bitfield is not something fwknop should try to interpret statically - definitely a bug. This will cause backwards compatibility issues, but that is unavoidable with this kind of fix. For timing, I'd like to put this into the 2.7.0 release, and get 2.6.10 out the door first I think. To answer your question above as well, yes, if we're not going to munge the payload before sending, then we won't need to worry about HMAC and/or decryption failures since any modification will fail.

For reference, the main motivation for stripping the prefixes was to make it a little harder to write a trivial Snort rule to detect fwknop communications. This wasn't a 'strong' measure, but at least one couldn't just write a rule to look for the base64-encoded version of 'Salted__' at the beginning of a UDP payload. The GnuPG prefix is less of a give away, but either way, the non-static nature of the bitfield is the overriding factor. Thanks.