gopass/pkg/pinentry: pinentry escapes '%' as '%25' and gopass does not unescape this

[svenpeter42]

Summary

I've been a big fan of this project for quite a while and stumbled upon a subtle issue while using another project.

I've recently switched my Yubikeys and used a PIN with special characters and ran into an issue while setting up https://github.com/FiloSottile/yubikey-agent. yubikey-agent internally uses github.com/gopasspw/gopass/pkg/pinentry which in turn is only used in the age backend within gopass itself. Because gopass always uses this implementation this issue will not result in any visible bugs.

pinentry escapes % signs (next to \r and \n, but these two probably never occur in a PIN) and gopass/pkg/pinentry does not unescape the received data. This breaks PINs that contain % as a character (supported by e.g. yubikeys).

There might be some hint about this in the protocol description. http://info2html.sourceforge.net/cgi-bin/info2html-demo/info2html?(pinentry)Protocol

To include linefeeds or other special characters, you may percent-escape them
(i.e. a line feed is encoded as %0A', the percent sign itself is encoded as %25').

The actual escaping comes from libassuan though. libassuan provides IPC communication between the various gnupg binaries.

gnupg uses this library to communicate with pinentry instead of using raw pipes: Specifically, the getpin request is done using do_getpin -> assuan_transact -> _assuan_read_from_server -> assuan_client_read_response which then unescapes the received line (see https://github.com/gpg/libassuan/blob/b270f2ec21b67f5728edae4b2ddf6fe17a0fd25a/src/client.c#L97).

Steps To Reproduce

The escape behaviour can be reproduced by running pinentry and using the GETPIN command. Then enter % as the pin:

$ pinentry
OK Pleased to meet you
GETPIN
D %25
OK

This can be reproduced on both Mac OS (using /usr/local/MacGPG2/libexec/pinentry-mac.app/Contents/MacOS/pinentry-mac and also pinentry-curses) and Linux (pinentry) and on all pinentry programs that use libassuan.

This does however not cause any issues within gopass itself: the age backend will happily use %25 as the passphrase for encryption and decryption.

Expected behavior

The % sign is correctly unescaped.

Environment

• OS: doesn't matter, but I've tried this on Mac OS and Linux
• OS version: doesn't matter, but Darwin [..] 18.7.0 Darwin Kernel Version 18.7.0: Thu Jun 18 20:50:10 PDT 2020; root:xnu-4903.278.43~1/RELEASE_X86_64 x86_64 and Linux [...] 5.7.19-hardened Add template feature #1-NixOS SMP Thu Aug 27 07:30:50 UTC 2020 x86_64 GNU/Linux
• gopass Version: git head
• Installation method: from source

Additional context

At first glance there is a quick way to fix this by adding the unescaping to

gopass/pkg/pinentry/pinentry.go

Line 114 in 11660a1

if !bytes.HasPrefix(buf, []byte("D ")) {

I believe this should be sufficient since the unescaping is only done when the reply starts with "D " and this is the only place in gopass that expects this reply from pinentry.

libassuan itself seems to be much more complex and I do not believe that a full go implementation is required for just using pinentry. I'll double check to see if there are any other subtle issues that may come up when directly talking to pinentry though.

There is one rather big problem with fixing this though: the age backend will currently use whatever it gets back from GetPIN. If the passphrase has a % sign this will just be replaced with %25 which will work just fine. If this escaping is fixed though suddenly any passphrase that contained a % before the fix will no longer work.

I'm not really sure what the best way forward is. Fixing this will break compatibility and make old users with % in their passphrases rather unhappy. Not fixing this will not cause any issues for users who just stick to gopass but might cause strange bugs for other projects (or other places within gopass who later switch to pinentry) who use gopass/pkg/pinentry

[AnomalRoil]

Thanks for the detailed explanation, it's very helpful.

@dominikschulz Since the pinentry package appears to be used externally, we might consider versioning it differently than the rest of Gopass, no?
This could be fixed in a v2.0.0 of the said package since it's indeed a breaking change.
We could decide to postpone switching to v2 of the pintentry package until Gopass v2.

Or we can just correct it now and have a disclaimer in the changelog warning our users that this is potentially a breaking change.
In both case we should consider how the migration path should look like.

[dominikschulz]

Ouch, this is indeed a nasty little bug. Thanks for the explanation.

Eventually the package github.com/gopasspw/gopass/pkg/pinentry should move to it's own repo, e.g. github.com/gopasspw/pinentry. And this escaping issue should be fixed.

How to get there is the interesting part. We might do as Go itself sometimes does and introduce the new escaping, disabled by default and controlled by an env var (e.g. GOPASS_PINENTRY_ESCAPE) so users like @svenpeter42 could enable it themselves and warn about the upcoming breaking change. After some time we could change the default but keep the env var to allow people being affected to disable it.