Although gpg is in most cases used in a pipeline or via file descriptors, the classic use case of decrypting on the command line is still relevant. To avoid that a broken decrypted file lingers around on the disk it seems to be better to integrate the write-to-tmpfile-and-rename method in openfile.c and do the rename only after a good decryption.
Description
Revisions and Commits
Related Objects
Event Timeline
Here is my idea to implement the feature:
(1) Extend struct iobuf_struct to have a field of temporary output (of int), just after real_fname.
- OUTPUTFILE: When it's 1, it means that a temporary output file generated with real_fname original suffix removed and appended .tmp
(2) Modify get_output_file in plaintext.c and make_outfile_name in openfile.c, so that OUTPUTFILE semantics above is used and the field in iobuf_struct is marked accordingly.
(3) Modify proc_encrypted in mainproc.c so that it can rename .tmp file to the OUTPUTFILE or it can remove it when failure, when the field is active.
@ikloecker says that Kleo already support this feature. (I didn't know that.)
So, compatibility flag to switch on/off the feature would be needed,
or this feature is not needed in GnuPG at all.
When --output option is used and the user uses temporary file and is ready for checking an error, that is, it's already prepared, it's redundant and useless, indeed.
The patch of mine above handles a particular (quite limited) case where:
(1) No --output option
(2) Input file is file.asc or file.gpg, and GnuPG deduces its output name file removing the extension
(3) In this particular use case (traditional/old one), it uses temporary filename and rename/remove the temporary file.
The extension .part is used by Mozilla/Firefox. Curl uses .tmp. Is that OK for Windows machine to use .part?
I retract my patch of temp-output-then-rename-20251117.patch.
Better handling is needed, I suppose.
@gniibe asked:
Is it OK for gpg/gpgsm to do something like PartialFileGuard when --status-fd is not enabled? I assume that gpgme/Kleo does always --status-fd, thus, we can avoid double PartialFileGuard.
For Kleopatra (and other apps using gpgmeqt) this would be okay, but other apps that use gpgme or gpgmepp directly wouldn't profit from this feature in GnuPG. Adding a new option to gpg/gpgsm to disable the partial file deletion might be a better idea than relying on --status-fd.
@ikloecker Thank you for your comment.
I had misunderstood that the PartilaFileGuard were the feature of gpgme/gpgmepp. Indeed, it is in gpgmeqt.
I'm going to add an explicit option to disable the partial file handling in gpg/gpgsm. Then, I'll try to add that option in gpgmeqt.
Here is a patch for gpgsm:
@pl13 Could you please test it out?
- Make a encrypted file of S/MIME (I think that testing with AES256-GCM is good. It is AEAD)
- Modify (binary-edit) the encrypted file, specifically, modifying one of the authtag bytes, make an example input file for gpgsm
- Expected result by gpgsm is: failure by gcry_cipher_checktag at https://dev.gnupg.org/source/gnupg/browse/master/sm/decrypt.c;f99e4291200d507f5ac8a73066a2ea259a1d7eb8$1453?as=source&blame=off
- If it's difficult to binary-edit an example file, run gpgsm under gdb control, marking a breakpoint beforehand at: https://dev.gnupg.org/source/gnupg/browse/master/sm/encrypt.c;f99e4291200d507f5ac8a73066a2ea259a1d7eb8$870?as=source&blame=off then, modify one byte of authtag manually, to produce malformed file
- Test with the patch above with the example input file, to see if it works correctly
I tested your patch applied to current master 2.5.20.
$ gpgsm -o a.out -d test.encr gpgsm: data is not authentic: Checksum error gpgsm: message decryption failed: Checksum error <gcrypt>
Without the patch applied a file a.out lingers.
After the patch no broken file remains.
@pl13 Thank you for your testing.
I pushed rGca8633c55edf: gpgsm: Use partial file on decryption, remove on failure. for gpgsm.
Here is a patch for gpg:
Please test it out with OCB encryption/decryption.
(IIUC, OCB is now default with newly created key. So, just use new key for the test.)
- Make a encrypted file of PGP, OCB
- Run gpg under gdb control
- Place a breakpoint beforehand at: https://dev.gnupg.org/source/gnupg/browse/master/g10/encrypt.c;f99e4291200d507f5ac8a73066a2ea259a1d7eb8$340?as=source&blame=off
- run (with --encrypt option, specifying recipient with -r option)
- at the breakpoint, modity one byte of authtag manually to produce malformed file
- Test with the patch here with the encrypted file input, to see it works correctly
In case of gpg, it uses filter to process the authtag, so, it is a bit complicated.
Expected result by gpg is: failure by gcry_cipher_checktag at
https://dev.gnupg.org/source/gnupg/browse/master/g10/decrypt-data.c;f99e4291200d507f5ac8a73066a2ea259a1d7eb8$206?as=source&blame=off
And this error should be raised up to main, this is the important point.
For gpg, we need to check (and possibly fix) the cases with:
- a signed then encrypted message
- a compressed then encrypted message
- importing an encrypted keyring
- etc.
In gpg, it uses filters to process a cascade (of sign+encrypt, of compress+encrypt, etc.). AEAD failure happens in the middle of filters. We need to make sure that an error inside properly propagated to final result.
In the comment of mine {T7873#218499}, I was wrong. The place I explained for a breakpoint was for symmetric encryption.
For public key encryption, it is:
When encrypting a file, it is good to supply --chunk-size 12 option with larger file (say, a few megabytes). Or else, the error handling on decryption will be done before opening the output file.
Tested on Linux with GnuPG 2.5.20.
Testing with a small file (16~byte) did not leave a broken file. If I
understand Werner correctly it is due to libgpg-error/estream.c:
fcancel emptying the buffer if the file fits in the buffer.
Thus I tested with a 1GB file.
$ gpg -o bigfile.encr -z0 --force-ocb -c bigfile.txtThen I modified bigfile.txt.
$ gpg -o a.out -d bigfile.encrOutput before patch:
gpg: AES256.OCB encrypted session key gpg: encrypted with 1 passphrase gpg: gcry_cipher_checktag failed: Checksum error gpg: problem reading source (1069547542 bytes remaining) gpg: handle plaintext failed: System error w/o errno gpg: WARNING: encrypted message has been manipulated!
A broken file remains.
Output after patch:
gpg: AES256.OCB encrypted session key gpg: encrypted with 1 passphrase gpg: gcry_cipher_checktag failed: Checksum error gpg: problem reading source (1069547542 bytes remaining) gpg: handle plaintext failed: No such file or directory gpg: WARNING: encrypted message has been manipulated!
No broken file remains.
When encrypting a file, it is good to supply --chunk-size 12 option with larger file (say, a few megabytes). Or else, the error handling on decryption will be done before opening the output file.
Thanks for the hint.
I will report back after testing the other cases.
I found that for a signed+encrypted file, when AEAD failure occurs in the stream of signature part (after literal data part), output file remains.
It's also the case when signature (which comes after literal data packet) is wrong.
I'd like to push the changes above for gpg, even if it's not exactly same as what Kleo does (not perfect enough: when signature check fails, output file remains;).
I tested following cases with a 100~mb file (GnuPG 2.5.20 on linux):
- signed, encrypted, compressed
- signed, encrypted, not compressed
- encrypted, compressed
- encrypted, not compressed
Results with the two most recent patches applied:
Used --chunk-size 12:
- Modified the first auth tag
- No file remains in all cases
- Modified a later tag
- File with .part suffix remains in case 1 and 3
Without --chunk-size:
- Modified the first auth tag
- File remains in cases 1 and 3
- Modified a later tag:
- File remains in all cases
- Using strace shows that a .part file is created and renamed but not deleted
In cases without compression I used option -z0.
For each case, decrypting the same file without the patches applied left an output file.
Note regarding COMPAT_NOPARTIALFILEGUARD:
I added { COMPAT_NO_PARTIALFILEGUARD, "no-partial-file-guard" }, to gpg.c:1044 for testing the compatibility flag.
IIUC gpg (with your patches applied) sets GPG_ERR_CHECKSUM if the auth tag check fails. Similar to gpg setting GPG_BAD_SIGNATURE for a bad mdc check (g10/decrypt-data.c:551).
Thus, i think in mainproc.c:858 and in mainproc.c:882 we also need to check for gpg_err_code (result) == GPG_ERR_CHECKSUM.