Improve handling of CR characters within cleartext signatures. - Githubissues

rnpgp / rnp

RNP: high performance C++ OpenPGP library used by Mozilla Thunderbird

https://www.rnpgp.org

Other

194 stars 55 forks source link

Improve handling of CR characters within cleartext signatures. #1265

Closed ni4 closed 3 years ago

ni4 commented 4 years ago

Description

We have some incompatibility with GnuPG when handling CR characters during cleartext signature generation/verification.

Sample data

0x31 0x30 0x64 0x0D: when cleartext-signed with rnp, considered as BAD signature by GnuPG. However if signed with GnuPG, then signature is considered as valid by rnp. 0x31 0x30 0x64 0x0D 0x0D: BAD signature if signed with rnp and checked by GnuPG, and BAD signature if signed by GnuPG and checked by rnp.

More test cases could (and should) be added, if \r character is added to set of random characters in function cli_common.py: random_text.

Solution

when generating cleartext signature, we should transform CR characters to CRLF sequence, which should be more compatible with other implementations
when checking cleartext signatures, we should treat CR without following LF just as another character, not any kind of EOL.

antonsviridenko commented 4 years ago

let me take it

antonsviridenko commented 4 years ago

@ni4 just to be sure, this issue is still relevant after this PR https://github.com/rnpgp/rnp/pull/1263 right?

ni4 commented 4 years ago

@antonsviridenko yeah, it is related to #1263 . It was just too much to fix everything in a single PR, so I separated not-that-high-priority part of the work to this issue. Sure, you are welcome to take it, thanks!

antonsviridenko commented 4 years ago

0x31 0x30 0x64 0x0D: when cleartext-signed with rnp, considered as BAD signature by GnuPG. However if signed with GnuPG, then signature is considered as valid by rnp.

rnp hashes this sequence as is, gpg signs only 0x31 0x30 0x64 with CR stripped.

ni4 commented 4 years ago

@antonsviridenko Yeah, probably GnuPG strips CR because of the next LF. We should also react in the same way as GnuPG when there is CR used in different places/there are CR CR LFs and so on. Best way to check this would be to include \rin cli_common.py: random_text and make cli_tests pass.

antonsviridenko commented 3 years ago

when checking cleartext signatures, we should treat CR without following LF just as another character, not any kind of EOL.

Seems like this proposition comes in contradiction with PGP specs.

As with binary signatures on text documents, a cleartext signature is calculated on the text using canonical line endings. The line ending (i.e., the ) before the '-----BEGIN PGP SIGNATURE-----' line that terminates the signed text is not considered part of the signed text.

All CR characters (without following LF) in input should be interpreted as line-endings and converted to CRLF sequence, and cleartext signature containing CR without following LF must be treated as invalid.

antonsviridenko commented 3 years ago

CR line endings belong to old Mac style plain text format (the one before OS X)

ni4 commented 3 years ago

@antonsviridenko Sorry, I deleted previous comment as was messed up with text document signatures. Basically, we should become compatible with GnuPG on line ending treating.

antonsviridenko commented 3 years ago

So, 1) GnuPG does not treat CR characters as EOL-s

2) trailing CR characters are removed, that's the difference in handling them between RNP and GnuPG

antonsviridenko commented 3 years ago

More test cases could (and should) be added, if \r character is added to set of random characters in function cli_common.py: random_text. Best way to check this would be to include \rin cli_common.py: random_text and make cli_tests pass.

@ni4 I tried to add \r to random_text(), now cli_tests fail somewhere in detached signature tests. Probably should be addressed as a new issue

198: ======================================================================
198: ERROR: test_gpg_to_rnp_default_key (__main__.SignDefault)
198: ----------------------------------------------------------------------
198: Traceback (most recent call last):
198:   File "/home/odsk/Ribose/rnp/src/tests/cli_tests.py", line 2035, in test_gpg_to_rnp_default_key
198:     rnp_detached_signing_gpg_to_rnp(size, True)
198:   File "/home/odsk/Ribose/rnp/src/tests/cli_tests.py", line 702, in rnp_detached_signing_gpg_to_rnp
198:     rnp_verify_detached(sigpath, KEY_SIGN_GPG)
198:   File "/home/odsk/Ribose/rnp/src/tests/cli_tests.py", line 341, in rnp_verify_detached
198:     raise_err('rnp detached verification failed', err + out)
198:   File "/home/odsk/Ribose/rnp/src/tests/cli_common.py", line 34, in raise_err
198:     raise CLIError(msg, log)
198: cli_common.CLIError: <unprintable CLIError object>

ni4 commented 3 years ago

@antonsviridenko It's the part of the same issue as the text document signature mode is used. Some combination of CR/LFs causes incompatibility with GnuPG again.

antonsviridenko commented 3 years ago

https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=g10/textfilter.c;h=3e68900bbabd3515ba16f40bcf866c326a270e71;hb=4070f302e4decc8d54d1305cbd30f6dab052ef7e#l96

g10/textfilter.c line 96

This is the place in code where GnuPG handles text documents. Posting here for references.

antonsviridenko commented 3 years ago

https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=g10/textfilter.c;h=3e68900bbabd3515ba16f40bcf866c326a270e71;hb=4070f302e4decc8d54d1305cbd30f6dab052ef7e#l192

g10/textfilter.c line 192

This is where GnuPG strips trailing chars when doing cleartext signatures.

antonsviridenko commented 3 years ago

@ni4 I think I've fixed it (see #1340), but now SignDefault cli test sometimes fails in the same place. After some investigation, it is the "--armor" option that makes the difference. First, detached binary signature is created by gpg and is successfully verified by rnp. Next, armored signature is generated from the same source file and rnp fails to verify it. So probably there should be unrelated issue in dearmoring code, but somehow changes in handling CR characters triggered it.

198: /usr/bin/gpg --homedir /tmp/rnpctmpy94zxtib/.gpg --text --pinentry-mode=loopback --batch --yes --passphrase password --trust-model always -u signing@gpg --detach-sign /tmp/rnpctmpy94zxtib/cleartext.txt
198:
198:
198: /home/odsk/Ribose/rnp/build/src/rnp/rnp --homedir /tmp/rnpctmpy94zxtib/.rnp --verify /tmp/rnpctmpy94zxtib/cleartext.txt.sig
198: Good signature made Wed Nov 18 19:16:55 2020
198: using RSA (Encrypt or Sign) key 2f1378a1690f9ada
198:
198: pub   2048/RSA (Encrypt or Sign) 2f1378a1690f9ada 2020-11-18 [SC]
198:       43cb199dbeb4ca0f1772dd272f1378a1690f9ada
198: uid           signing@gpg
198: Signature(s) verified successfully
198:
198: /usr/bin/gpg --homedir /tmp/rnpctmpy94zxtib/.gpg --text --armor --pinentry-mode=loopback --batch --yes --passphrase password --trust-model always -u signing@gpg --detach-sign /tmp/rnpctmpy94zxtib/cleartext.txt
198:
198:
198: /home/odsk/Ribose/rnp/build/src/rnp/rnp --homedir /tmp/rnpctmpy94zxtib/.rnp --verify /tmp/rnpctmpy94zxtib/cleartext.txt.asc
198: [signature_validate() /home/odsk/Ribose/rnp/src/lib/crypto/signatures.cpp:217] wrong lbits
198: BAD signature made Wed Nov 18 19:16:56 2020
198: using RSA (Encrypt or Sign) key 2f1378a1690f9ada
198:
198: pub   2048/RSA (Encrypt or Sign) 2f1378a1690f9ada 2020-11-18 [SC]
198:       43cb199dbeb4ca0f1772dd272f1378a1690f9ada
198: uid           signing@gpg
198: Signature verification failure: 1 invalid signature(s), 0 unknown signature(s)
198:
198: ERROR

EDIT: Actually "--armor" option does not do any difference in result here, new source text document is generated by the cli test code each time between these verifications. Actual reason of failure was \r character at the end of 32k block which was stripped when it should not be stripped.

antonsviridenko commented 3 years ago

gpg: can't handle text lines longer than 19995 characters Seems like gpg stops converting line endings and signs as a binary document in this case. Do we need to behave the same way for compatibility?

ni4 commented 3 years ago

gpg: can't handle text lines longer than 19995 characters Seems like gpg stops converting line endings and signs as a binary document in this case. Do we need to behave the same way for compatibility?

I think that it would be enough to display a warning, like "Text signature: too long line, may cause incompatibility with other implementations. Consider using binary signature instead". In real world 20k text would barely happen.

antonsviridenko commented 3 years ago

https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=g10/plaintext.c;h=3e169d93fa98a0eb7974f65775745469ebe980bd;hb=4070f302e4decc8d54d1305cbd30f6dab052ef7e#l520 Another place where gpnugp does some workarounds with CR & LF characters when doing verification of detached canonical text document