Skip to content

ability to forward detached encrypted attachments(no inline)#1611

Merged
DenBond7 merged 5 commits intomasterfrom
issue_1538_include_attachments_in_forwarded_message
Dec 17, 2021
Merged

ability to forward detached encrypted attachments(no inline)#1611
DenBond7 merged 5 commits intomasterfrom
issue_1538_include_attachments_in_forwarded_message

Conversation

@DenBond7
Copy link
Collaborator

@DenBond7 DenBond7 commented Dec 15, 2021

This PR added the ability to forward any attachments.

It means we can forward an encrypted message with detached attachments as an encrypted message(in that case we will download, decrypt and encrypt attachments with new pub keys before sending) or as a standard message(in that case we will download and decrypt attachments before sending)

close #1538


Tests (delete all except exactly one):

  • Difficult to test (explain why): It relates to flow testing that we don't support yet. Tested manually.

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities

@DenBond7 DenBond7 force-pushed the issue_1538_include_attachments_in_forwarded_message branch from ec3981e to 2ec6e7d Compare December 16, 2021 08:07
@DenBond7 DenBond7 marked this pull request as ready for review December 16, 2021 08:41
@DenBond7 DenBond7 changed the title ability to forward any attachments ability to forward detached encrypted attachments(no inline) Dec 16, 2021
@DenBond7 DenBond7 requested a review from tomholub December 16, 2021 08:41
Comment on lines +163 to +166
fun isEncrypted(): Boolean {
val fileExtension = FilenameUtils.getExtension(name)
return fileExtension.equals(Constants.PGP_FILE_EXT, true)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's usually more complex than that. For example see

  public treatAs = (): Attachment$treatAs => {
    if (this.treatAsValue) { // pre-set
      return this.treatAsValue;
    } else if (['PGPexch.htm.pgp', 'PGPMIME version identification', 'Version.txt', 'PGPMIME Versions Identification'].includes(this.name)) {
      return 'hidden';  // PGPexch.htm.pgp is html alternative of textual body content produced by PGP Desktop and GPG4o
    } else if (this.name === 'signature.asc' || this.type === 'application/pgp-signature') {
      return 'signature';
    } else if (!this.name && !this.type.startsWith('image/')) { // this.name may be '' or undefined - catch either
      return this.length < 100 ? 'hidden' : 'encryptedMsg';
    } else if (this.name === 'msg.asc' && this.length < 100 && this.type === 'application/pgp-encrypted') {
      return 'hidden'; // mail.ch does this - although it looks like encrypted msg, it will just contain PGP version eg "Version: 1"
    } else if (Attachment.encryptedMsgNames.includes(this.name)) {
      return 'encryptedMsg';
    } else if (this.name.match(/(\.pgp$)|(\.gpg$)|(\.[a-zA-Z0-9]{3,4}\.asc$)/g)) { // ends with one of .gpg, .pgp, .???.asc, .????.asc
      return 'encryptedFile';
    } else if (this.name.match(/(cryptup|flowcrypt)-backup-[a-z0-9]+\.(key|asc)$/g)) {
      return 'privateKey';
    } else if (this.type === 'application/pgp-keys') {
      return 'publicKey';
    } else if (this.name.match(/^(0|0x)?[A-F0-9]{8}([A-F0-9]{8})?.*\.asc$/g)) { // name starts with a key id
      return 'publicKey';
    } else if (this.name.toLowerCase().includes('public') && this.name.match(/[A-F0-9]{8}.*\.asc$/g)) { // name contains the word "public", any key id and ends with .asc
      return 'publicKey';
    } else if (this.name.match(/\.asc$/) && this.hasData() && Buf.with(this.getData().subarray(0, 100)).toUtfStr().includes('-----BEGIN PGP PUBLIC KEY BLOCK-----')) {
      return 'publicKey';
    } else if (this.name.match(/\.asc$/) && this.length < 100000 && !this.inline) {
      return 'encryptedMsg';
    } else {
      return 'plainFile';
    }
  };

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I had to choose a better name for this method. It just check that an attachment can be an encrypted file. Like file.txt.pgp should be an encrypted file.

But now I see it should be improved. At least it should cover the following

if (this.name.match(/(\.pgp$)|(\.gpg$)|(\.[a-zA-Z0-9]{3,4}\.asc$)/g)) { // ends with one of .gpg, .pgp, .???.asc, .????.asc
      return 'encryptedFile';

@tomholub maybe you have more ideas. I need to mark an attachment as decrypt_before_sending.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I meant - at least that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be renamed into something like isPossiblyEncrypted

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, or couldBeEncrypted

var tempFileForDecryptionPurposes: File? = null
try {
val finalSrcInputStream = if (shouldBeDecryptedBeforeProcess) {
tempFileForDecryptionPurposes = File.createTempFile("tmp", null, destFile.parentFile)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's an actual file, and it contains plain - not encrypted data, then this is forbidden. We cannot save that locally, even temporarily, even if it's in app's own storage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we talking about tempFileForDecryptionPurposes? Do you mean I have not to use a temp file for decryption purposes? Have I to do it on the fly?

Copy link
Collaborator

@tomholub tomholub Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plain file can never be saved to the device when forwarding.

That means no intermediary files, unless that intermediary file is properly encrypted.

But better would be to download / decrypt / re-encrypt in single stream. The re-encrypted file could be saved temporarily before uploading it, that would be ok.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case 1: original message was encrypted, forwarded message will be encrypted too

I can do the following: I have input stream of encrypted file. I decrypt it to byte array (RAM. Actually it is not good for performance), then I encrypt it and save to a temporary file.

Looks good?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do that. How will you encrypt it? Already with the final public keys? Do you know them at that point?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything is possible

I wanted to say that it's possible for Java/Kotlin world. Just not sure that it's possible for PGPainless/Bouncy Castle. Anyway, I will check it tomorrow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hha, you are totally right 👍. It's a good idea. I can just move all this logic to the next step. As for me manipulation with attachments will be better to do at this stage, to be much clearer. Then the next step will be responsible just for sending... But security reasons encourage me to do file manipulation at the last stage(sending).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I talked about steps I meant the following: we have 3 different services

  1. Service that creates MIME message
  2. Service that downloads forwarded atts(all things in the current PR relate to it)
  3. Service that sends MIME messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it' good idea. Then you don't need to re-encrypt encrypted message. If you were about to re-encrypt, you would have to generate random temporary key for symmetric encryption algorithm (for example, AES-256), held only in memory, and then encrypt data with it, then later decrypt with it. Not sure if BC is required for this, JDK itself should be capable of symmetric encryption/decryption with some widely accepted algorithms.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the context of forwarding encrypted attachments, the re-encrypt here means encrypting it with the recipient's public keys. Since that's what you have to do before sending it out.

… an attachment as-is(without post-processing).| #1538
…tachments from ForwardedAttachmentsDownloaderWorker to MessagesSenderWorker. Refactored code.| #1538
@DenBond7
Copy link
Collaborator Author

I've added the requested changes. I hope it will be enough for now. Unfortunately, I'm not happy with this realization. Because some logic is located in the wrong place(in my opinion). If we have MessagesSenderWorker it should contain only logic that is related to transfer messages, but not preprocessing(decrypt/encrypt and so on). I hope we will improve that. I will create an issue for that where we can discuss it.

@DenBond7 DenBond7 requested a review from tomholub December 17, 2021 14:03
Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better

@DenBond7 DenBond7 merged commit 72cab1e into master Dec 17, 2021
@DenBond7 DenBond7 deleted the issue_1538_include_attachments_in_forwarded_message branch December 17, 2021 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

include attachments in forwarded message - detached

3 participants