ProtonMail / gosop

Stateless CLI for GopenPGP
MIT License
32 stars 10 forks source link

gosop doesn't handle 8-bit data correctly #15

Closed teythoon closed 3 weeks ago

teythoon commented 1 year ago

Seems it is replaced with the unicode REPLACEMENT CHARACTER suggesting a superfluous conversion to a string somewhere:

% echo -ne 'what is\xff\xff\xff\xff\xffhappening' | sqop encrypt alice-secret.pgp | gosop decrypt alice-secret.pgp | hd
00000000  77 68 61 74 20 69 73 ef  bf bd 68 61 70 70 65 6e  |what is...happen|
00000010  69 6e 67                                          |ing|
00000013
guillemj commented 1 year ago

I'm seeing something similar but with the dearmor command, but I guess this might be related. With the following file

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

This is a data file that will be signed
as part of the dpkg OpenPGP test suite.

It contains «UTF-8» characters so that
we can check binary and ASCII signatures ☺.
-----BEGIN PGP SIGNATURE-----

wr0EARYKAG8FgmNn/JoJEF8IeVKOdEodRxQAAAAAAB4AIHNhbHRAbm90YXRpb25z
LnNlcXVvaWEtcGdwLm9yZ6ner4BBATzhFswk2JmdwxDwRwEY7Ez64dxHFNuT0rIn
FiEEld5RAbTaqSHOdkqUXwh5Uo50Sh0AAH26AQCojIenPOWX7+GUk+lKeo+7hnpx
nozY9z/+4Pe1KamB4AEAsL9fpRgmecLcVhHBteK8t8/laLkzdY4nji+1BmeRrgQ=
=4UM2
-----END PGP SIGNATURE-----

Running the following commands to check for round-trip:

$ gosop dearmor <sign-file-inline.asc | gosop armor
dearmor: illegal base64 data at input byte 4
-----BEGIN PGP MESSAGE-----
Version: GopenPGP 2.5.2
Comment: https://gopenpgp.org

This
=IpsZ
-----END PGP MESSAGE-----

(If this is not related I'm happy to open a different issue for this.)

dkg commented 1 year ago

@guillemj i think your question is something different, and it has more to do with https://gitlab.com/dkg/openpgp-stateless-cli/-/issues/51 (SOP is unclear what to do when armoring or dearmoring CSF messages). If you have a strong preference about what should happen here, please follow up over there.

guillemj commented 1 year ago

@dkg ah sorry, I see this is not something the dpkg test suite is checking, because as you say the behavior changes among implementations. I think this might have been something I found while playing with gosop trying to see where it would fail from the test case failures.

In any case I think the error mode here is not great, and the UTF-8 and 8-bit handling should be improved. For the specific dearmor/armor round-trip it's probably not important how it behaves, at least both sqop and pgpainless-cli will just not round-trip and return the armored signature. But what seems that should never happen is generating truncated output.

twiss commented 3 weeks ago

I believe the original issue has been fixed :)

@guillemj regarding your issue, gosop dearmor returns an error, which I think is arguably more sensible than just throwing away the message and dearmoring only the signature. Since you're piping the result to gosop armor, the error gets swallowed, but that's more a bash quirk IMHO. In a script, you can set -eo pipefail to prevent this.

Admittedly the error message could be improved, feel free to open a separate issue or a PR if you care about that :)