Verify message by sender #4144

rrrooommmaaa · 2021-11-25T07:17:24Z

This PR refactors decryption functions to use verificationPubs as parameter,
fetches unknown sender's pubkeys with publookup, automatically saving them to ContactStore and using to verify signature.
sender email is always displayed as Signer (primary UID from pubkey is ignored)

close #3884

Tests (delete all except exactly one):

Tests added or updated

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
is documented clearly and usefully, or doesn't need documentation

tomholub

this is roughly it, as a wip

extension/chrome/elements/attachment.ts

extension/chrome/elements/attachment_preview.ts

extension/chrome/elements/compose-modules/compose-quote-module.ts

extension/chrome/elements/pgp_block_modules/pgp-block-attachmens-module.ts

extension/chrome/elements/pgp_block_modules/pgp-block-decrypt-module.ts

extension/chrome/settings/modules/decrypt.ts

extension/chrome/elements/pgp_block_modules/pgp-block-signature-module.ts

extension/js/common/core/crypto/pgp/msg-util.ts

…tion-by-sender

… except successful verification

…tion-by-sender

lgtm-com · 2021-12-15T16:19:59Z

This pull request introduces 1 alert when merging 2df3953 into c636c10 - view on LGTM.com

new alerts:

1 for User-controlled bypass of security check

tomholub

New changes all look good. I'll do another review for all of the changes hollistically

tomholub

code looks good

tomholub · 2021-12-15T16:26:12Z

extension/chrome/elements/pgp_block_modules/pgp-block-signature-module.ts

+          // in some tests we load the block without sender information
+          $('#pgp_signature').addClass('bad').find('.result').text(`Cannot verify: missing pubkey, missing sender info`);


This should not really happen. If we have customers complain about this, we'll have to be fetching this information from Google API. Which I think we already do - before the frame is rendered. For now, we can leave this as is.

tomholub · 2021-12-15T16:28:17Z

extension/chrome/elements/pgp_block_modules/pgp-block-signature-module.ts

+  private renderBadSignature = () => {
+    $('#pgp_signature').addClass('bad');
+    $('#pgp_signature > .result').text('signature does not match');
+    this.view.renderModule.setFrameColor('red'); // todo: in what other cases should we set the frame red?


Decrypt error, which we are already doing.

tomholub · 2021-12-15T16:40:33Z

If you are talking about re-fetching the message because the DOM may have mingled it, then yes - we should display the re-fetched message we got from Google API.

If you mean re-fetching public keys for verification, then we don't have to re-render the message, just verification result.

Does this mean this is still actionable to change / update in this PR?

rrrooommmaaa · 2021-12-15T16:41:51Z

If you are talking about re-fetching the message because the DOM may have mingled it, then yes - we should display the re-fetched message we got from Google API.

yes, in this particular test the message is completely different when re-fetched from API.

tomholub · 2021-12-15T16:42:57Z

If you are talking about re-fetching the message because the DOM may have mingled it, then yes - we should display the re-fetched message we got from Google API.

yes, in this particular test the message is completely different when re-fetched from API.

We should indeed re-render the message body to match what we got from API. Since that's what we are verifying on the second attempt. Could be in another issue - since we were never doing that before, it's not new to this PR. Well noticed.

rrrooommmaaa · 2021-12-15T16:43:12Z

If you mean re-fetching public keys for verification, then we don't have to re-render the message, just verification result.

Is this for performance reasons? I didn't pay too much attention on what is rendered and re-rendered. I can do this in this or another PR

rrrooommmaaa · 2021-12-15T16:44:08Z

We should indeed re-render the message body to match what we got from API. Since that's what we are verifying on the second attempt. Could be in another issue - since we were never doing that before, it's not new to this PR. Well noticed.

The issue is already filed (#4164)

tomholub · 2021-12-15T16:45:01Z

If you mean re-fetching public keys for verification, then we don't have to re-render the message, just verification result.

Is this for performance reasons. I didn't pay too much attention on what is rendered and re-rendered. I can do this in this or another PR

It's just for less work reasons, development-wise. Usually re-rendering fewer things is less work. As long as it doesn't create any side effects, it doesn't really matter if the body is re-rendered.

There is one possible side effect - if the user starts wanting to copy and paste part of the message, re-rendering will upset them because their highlight will be gone if the text gets replaced while trying to copy it.

rrrooommmaaa · 2021-12-15T16:48:20Z

There is one possible side effect - if the user starts wanting to copy and paste part of the message, re-rendering will upset them because their highlight will be gone if the text gets replaced while trying to copy it.

I'm afraid there might be such side effects. I suggest to fix them after (or as part of) #4164 as there is a stack of modules calling each other. Perhaps, a bit of refactoring is in order.

tomholub · 2021-12-15T16:52:17Z

Getting an exception - this scenario is likely not tested? it's on flowcrypt.compatibility account.

TypeError: Cannot set properties of undefined (setting 'parsedSignature')
    at PgpBlockViewSignatureModule.renderPgpSignatureCheckResult (chrome-extension://pljjhfjdhkbkhhkmngllhdmccdflgnbp/chrome/elements/pgp_block_modules/pgp-block-signature-module.js:17:57)
    at PgpBlockViewRenderModule.decideDecryptedContentFormattingAndRender (chrome-extension://pljjhfjdhkbkhhkmngllhdmccdflgnbp/chrome/elements/pgp_block_modules/pgp-block-render-module.js:70:45)
    at PgpBlockViewDecryptModule.decryptAndRender (chrome-extension://pljjhfjdhkbkhhkmngllhdmccdflgnbp/chrome/elements/pgp_block_modules/pgp-block-decrypt-module.js:69:50)
    at async PgpBlockViewDecryptModule.initialize (chrome-extension://pljjhfjdhkbkhhkmngllhdmccdflgnbp/chrome/elements/pgp_block_modules/pgp-block-decrypt-module.js:34:21)
    at async PgpBlockView.render (chrome-extension://pljjhfjdhkbkhhkmngllhdmccdflgnbp/chrome/elements/pgp_block.js:44:17)
    at async chrome-extension://pljjhfjdhkbkhhkmngllhdmccdflgnbp/js/common/view.js:26:17

### Catch.reportErr calling stack ###
#     at Function.Catch.formatExceptionForReport (chrome-extension://pljjhfjdhkbkhhkmngllhdmccdflgnbp/js/common/platform/catch.js:252:90)
#     at Function.Catch.onErrorInternalHandler (chrome-extension://pljjhfjdhkbkhhkmngllhdmccdflgnbp/js/common/platform/catch.js:76:29)
#     at Function.Catch.reportErr (chrome-extension://pljjhfjdhkbkhhkmngllhdmccdflgnbp/js/common/platform/catch.js:92:18)
#     at PgpBlockViewErrorModule.handleInitializeErr (chrome-extension://pljjhfjdhkbkhhkmngllhdmccdflgnbp/chrome/elements/pgp_block_modules/pgp-block-error-module.js:56:23)
#     at PgpBlockViewDecryptModule.initialize (chrome-extension://pljjhfjdhkbkhhkmngllhdmccdflgnbp/chrome/elements/pgp_block_modules/pgp-block-decrypt-module.js:55:45)
#     at async PgpBlockView.render (chrome-extension://pljjhfjdhkbkhhkmngllhdmccdflgnbp/chrome/elements/pgp_block.js:44:17)
#     at async chrome-extension://pljjhfjdhkbkhhkmngllhdmccdflgnbp/js/common/view.js:26:17
# 
# url: chrome-extension://pljjhfjdhkbkhhkmngllhdmccdflgnbp/chrome/elements/pgp_block.htm?frameId=frame_tXWErApBbb&message=-----BEGIN%20PGP%20SIGNED%20MESSAGE-----%0AHash%3A%20SHA256%0A%0Athis%20message%20is%20signed%20and%20was%20modified%20in%20transit%0A-----BEGIN%20PGP%20SIGNATURE-----%0AVersion%3A%20FlowCrypt%205.0.4%20Gmail%20Encryption%20flowcrypt.com%0AComment%3A%20Seamlessly%20send%2C%20receive%20and%20search%20encrypted%20email%0A%0AwsFcBAEBCAAQBQJZ%2B8VXCRAGylU%2BwkVdcAAAgRwQAMkc%2B2WrIBf%2BX3T%2ByNps%0Ava4xMQEVvBFS4GxbTA1xndmfuo3hr3Vfq%2FsAI2Plo65D0rd9Wc6p88Awl%2B9w%0Aa0l4Yx3isH0jm9gcpypzYJf9leertBcvIxDvqGZc%2FxPxyRzQKgXiNkvaZI8U%0ANNkoAdCves4wx3sb0lJhIKyRhXwKWWroWKfoBYAryKDSE2q8d1z%2BP0mDbop7%0AJ5YLui%2B6VsGJoqZdM8lMGLp2TuyGVxy5moEvrIY8zvYm61Ke%2BwGLbL63Bqdg%0AZb8pMEp1xdW%2FxPgFT6IrtFdHTEiqczkeiHOG7%2BF8zPcKBbvx6luFAvFh103I%0Al06RRefeJkppLJ2S%2BA0iDzmqDknYin2Hm8VBa2ill%2FRfI4c1ThzM5trjrC1b%0ArOMzIMZfDdCDCRjfILNyjnanhsYEYZ30NnmjOrNDthA9VCEUupoRvSmDbA8O%0AVtckZH3YzIi6CsHs8dl2WTQ%2Flj1yXKDqnpoWzndzVrFRtjVW7I75lL%2F%2B%2FMYp%0AhktbFAWIkrqqWapwIUEGefzbrulRRVUU6m6slbKIurOgkEt%2BcoJUNMTsuPPX%0AaAa43ze8hUji5EXALE9I%2B41uz%2BdtnJymaQhsnXL8R8VHAgowrvQgLeuTZNVH%0AAeNkbcng%2BKz3ErVk3Q%2BRDt0i5FL0oIzP%2FAsxhGeOG38j%2BQVe6OgYm7IjfaIA%0Ar498%0A%3D76q6%0A-----END%20PGP%20SIGNATURE-----&msgId=15f7f7c5979b5a26&senderEmail=cryptup.tester%40gmail.com&isOutgoing=___cu_false___&acctEmail=flowcrypt.compatibility%40gmail.com&parentTabId=73%3A0
# 
######################

tomholub · 2021-12-15T16:54:12Z

Also happens on [enigmail] signed message inline + attachment

tomholub · 2021-12-15T16:59:16Z

Separately, it seems to me that the pgp block doesn't finish rendering anything until the decrypt -> try to verify -> aha missing pubkey -> fetch pubkeys -> reverify loop is complete.

I didn't investigate in detail, could it be true? I'd recommend to write a test as follows:

add /attester/pub/this.pubkey.takes.long.time.to.load@sender.test to attester mock api. It will include a 5 second sleep before returning the pubkey.
write a ui test that checks a message signed by that key
wait in ui test of the pgp block for texts in this sequence: 1) message body 2) "verifying signature" 3) good signature

Do you have a similar test that checks for the "verifying signature" step specifically?

tomholub · 2021-12-15T17:09:12Z

if one of the keys now contain the longid of the key that signed the message, we re-process the whole message with that public key - in background.

By background do you mean sending PgpMsgVerify? That is implemented.

By background I meant that the user should not need to wait for it. Render the text body as soon as it's decrypted even if pubkeys are missing to verify it, and render "verifying signature..". So user can see message right away, and signature verification result only later, when it's done verifying.

tomholub · 2021-12-15T17:11:22Z

Once we've done re-processing the message, we only look at the result of signature verification and re-render the signature result with the final verification result (valid / bad / error), without re-rendering the rest of the message.

Are you concerned about taking too much time to re-render the message? I need to double-check on that. The whole chain of calls DecryptModule->RenderModule->SignatureModule->DecryptModule is far too complicated. Also, I think we SHOULD re-render the message text when re-fetching it from API -- I filed an issue #4164

Here I was concerned that the user would have to wait 10 seconds to read the message if it takes 10 seconds to fetch public keys to verify it. Sorry for not clarifying earlier, I overlooked this comment.

lgtm-com · 2021-12-16T13:58:48Z

This pull request introduces 1 alert when merging 761f9a9 into c636c10 - view on LGTM.com

new alerts:

1 for User-controlled bypass of security check

rrrooommmaaa · 2021-12-16T14:12:15Z

Getting an exception - this scenario is likely not tested? it's on flowcrypt.compatibility account.

fixed this -- no longer re-fetching the encrypted message when verification fails

rrrooommmaaa · 2021-12-16T14:13:44Z

Also happens on [enigmail] signed message inline + attachment

This one should now be passing too

tomholub · 2021-12-16T14:27:07Z

This does not close the issue fully yet - I've filed #4197 which should be done right after.

refactor decryption functions to use verificationPubs as parameter

8f250eb

tomholub reviewed Nov 25, 2021

View reviewed changes

rrrooommmaaa added 10 commits November 28, 2021 10:49

more refactorings and fixes

b8e5ce5

Merge remote-tracking branch 'origin/master' into issue-3884-verifica…

e2baa50

…tion-by-sender

fix

6436edf

fix unit tests that used to query by longid

7c9c25c

fixed some tests

ba745e8

removed unused browser msg

8ae8d04

renames

b0c43d4

Merge remote-tracking branch 'origin/master' into issue-3884-verifica…

9287eaa

…tion-by-sender

fixed Message Not Signed and Missing Pubkey

482fff0

Merge remote-tracking branch 'origin/master' into issue-3884-verifica…

ba4db34

…tion-by-sender

This was referenced Dec 1, 2021

choose id carefully when rendering signature by multi-uid key #3894

Closed

Signature verification of attachments #4158

Open

rrrooommmaaa added 4 commits December 1, 2021 09:55

fix

d4160b7

added remarks to link to #4158

94a6846

remark

7130cf8

refactorings

e048d61

rrrooommmaaa mentioned this pull request Dec 5, 2021

Re-fetching signed-only message from API doesn't refresh the fetched text #4164

Closed

rrrooommmaaa added 2 commits December 5, 2021 04:44

re-fetching signed-only message on non-fatal verification error

2609b7d

Merge remote-tracking branch 'origin/master' into issue-3884-verifica…

4098b19

…tion-by-sender

This comment has been minimized.

Sign in to view

rrrooommmaaa added 3 commits December 5, 2021 11:04

remark

7dfdb80

Merge remote-tracking branch 'origin/master' into issue-3884-verifica…

ee7c40d

…tion-by-sender

Merge remote-tracking branch 'origin/master' into issue-3884-verifica…

937622c

…tion-by-sender

rrrooommmaaa mentioned this pull request Dec 6, 2021

Test how a message signed by a revoked key is displayed in our system. #4172

Open

rrrooommmaaa added 4 commits December 6, 2021 15:42

differentiate bad signature and missing pubkey, red color any outcome…

99b40fc

… except successful verification

Merge remote-tracking branch 'origin/master' into issue-3884-verifica…

a86ced2

…tion-by-sender

save fetched pubkeys to ContactStore

ca27504

lint fix

48f6bf7

FlowCrypt deleted a comment from lgtm-com bot Dec 15, 2021

rrrooommmaaa enabled auto-merge (squash) December 15, 2021 16:19

tomholub reviewed Dec 15, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

tomholub reviewed Dec 15, 2021

View reviewed changes

FlowCrypt deleted a comment from github-actions bot Dec 15, 2021

don't refetch encrypted message on verification error

761f9a9

tomholub mentioned this pull request Dec 16, 2021

make sure that fetching verification pubkeys does not delay message rendering #4197

Closed

tomholub approved these changes Dec 16, 2021

View reviewed changes

rrrooommmaaa merged commit d2aff54 into master Dec 16, 2021

rrrooommmaaa deleted the issue-3884-verification-by-sender branch December 16, 2021 14:27

		// in some tests we load the block without sender information
		$('#pgp_signature').addClass('bad').find('.result').text(`Cannot verify: missing pubkey, missing sender info`);

Verify message by sender #4144

Verify message by sender #4144

Uh oh!

Conversation

rrrooommmaaa commented Nov 25, 2021 • edited by tomholub Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To be filled by reviewers

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

lgtm-com bot commented Dec 15, 2021

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

tomholub Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

tomholub Dec 15, 2021

Choose a reason for hiding this comment

Uh oh!

tomholub commented Dec 15, 2021

Uh oh!

rrrooommmaaa commented Dec 15, 2021

Uh oh!

tomholub commented Dec 15, 2021

Uh oh!

rrrooommmaaa commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rrrooommmaaa commented Dec 15, 2021

Uh oh!

tomholub commented Dec 15, 2021

Uh oh!

rrrooommmaaa commented Dec 15, 2021

Uh oh!

tomholub commented Dec 15, 2021

Uh oh!

tomholub commented Dec 15, 2021

Uh oh!

tomholub commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomholub commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomholub commented Dec 15, 2021

Uh oh!

lgtm-com bot commented Dec 16, 2021

Uh oh!

rrrooommmaaa commented Dec 16, 2021

Uh oh!

rrrooommmaaa commented Dec 16, 2021

Uh oh!

tomholub commented Dec 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rrrooommmaaa commented Nov 25, 2021 •

edited by tomholub

Loading

rrrooommmaaa commented Dec 15, 2021 •

edited

Loading

tomholub commented Dec 15, 2021 •

edited

Loading

tomholub commented Dec 15, 2021 •

edited

Loading