S/MIME signing (WIP) by rrrooommmaaa · Pull Request #3979 · FlowCrypt/flowcrypt-browser

rrrooommmaaa · 2021-09-14T14:10:01Z

This PR supports sending Signed S/MIME messages

Tasks:

encrypt/decrypt PKCS#7 message
support for armored PKCS#7
support for PKCS#7-encrypted drafts
send simple-text S/MIME message without attachments

close #2970

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

…smime

rrrooommmaaa · 2021-09-28T15:08:04Z

@tomholub a simple-text S/MIME was successfully generated, also S/MIME drafts (encrypted) are supported in this PR.
Which features of the task list should I implement in this PR and which to postpone? (The PR becomes quite big).

tomholub · 2021-09-28T15:15:55Z

Ok to merge limited functionality, for smaller PR.

The ultimate goal is to support signed+encrypted S/MIME, which is what one of our customer needs and has been asking for. That can also be in separate PR.

add authenticated attributes to PKCS#7 message

does this mean the actual signing? Or something else?

send simple text with attachments

can be a separate issue for the future

send richtext?

can be a separate issue for the future

signed and encrypted?

This is the most important. Could also be tackled separately.

rrrooommmaaa · 2021-09-28T16:47:54Z

add authenticated attributes to PKCS#7 message

does this mean the actual signing? Or something else?

Well, according to docs, it is possible for SignerInfo to have some optional signed attributes, listed here: https://datatracker.ietf.org/doc/html/rfc5652#section-11

From what I understood, we can omit all of these signed attributes (and Thundertbird successfully verified the signature without them).
However, if we require at least one (e.g. "Signing Time"), we MUST include "Content Type" and "Message Digest".
As said in https://datatracker.ietf.org/doc/html/rfc5652#section-5.6 ... If the SignedData signerInfo includes
signedAttributes

So, as far as my understanding goes, the question is whether we need Signing Time

rrrooommmaaa · 2021-09-28T16:54:36Z

They are quite easy to add. Not sure if necessary. The forge library shows this example

      authenticatedAttributes: [{
        type: forge.pki.oids.contentType,
        value: forge.pki.oids.data
      }, {
        type: forge.pki.oids.messageDigest
        // value will be auto-populated at signing time
      }, {
        type: forge.pki.oids.signingTime
        // value will be auto-populated at signing time
        //value: new Date('2050-01-01T00:00:00Z')
      }]

forge.pki.oids.data is a constant 1.2.840.113549.1.7.1, other values are placeholders to be filled by the forge library itself.
Took time to figure out.
Guessed you (or the client) had a position on whether this is required.

tomholub · 2021-09-28T17:17:59Z

Please file the above two comments in a separate issue. We'll deliver to customer without it. If they need it, they will know (their recipient system will reject the message). Then we'd add it at that point. Otherwise, we'd add it whenever we have spare time in the future (2023? :-) )

tomholub

This is a serious effort - very happy to see this.

tomholub · 2021-09-30T07:55:26Z

extension/js/common/core/crypto/key.ts

+    let smimePubs = pubs.filter(pub => pub.pubkey.type === 'x509');
+    if (pgpPubs.length && smimePubs.length) {
+      // get rid of some of my keys to resolve the conflict
+      // todo: how would it work with drafts?


Drafts could be enabled only when we have at least one OpenPGP private key? Then make them OpenPGP?

In general, it's ok to explicitly disable functionality when user only imports S/MIME private key and nothing else. It's not a scenario we are designing for, even if it's allowed.

I implemented drafts for S/MIME because there were errors popping up when testing S/MIME user (without OpenPGP keys) editing a message (although "experimental"). I will disable such drafts completely, if you like.

If it already works, no need to disable it. If you needed to spend a bunch of other work, then can disable as a way to save time

Yes, it works. I even added a test that loads an ecnrypted PKCS#7 draft. Was also planning on adding a test that saves a PKCS#7 draft, but will skip it, if you wish so.

…smime

rrrooommmaaa · 2021-10-06T09:15:01Z

Oh perhaps need to add a test to send an S/MIME signed message when passphrase is not cached.

tomholub

I'm starting to feel like we are writing some sort of an S/MIME library here. It may be worth to one day extract this functionality, including their own tests, and make an actual library from it that uses forge as dependency and get it security-reviewed. Then we'd import this s/mime library into our project. Either that, or contribute the improvements back to forge. Because these S/MIME operations are getting quite dense: ideally the browser extension code would work at a higher level of abstraction.

For now, it's more convenient to keep it in this codebase, for flexibility as you develop and add major S/MIME functionality. But once that mostly settles, it would be better to extract it into a library.

tomholub · 2021-10-06T09:30:58Z

extension/js/common/core/crypto/pgp/pgp-armor.ts

+      if (p7.type !== '1.2.840.113549.1.7.3') {
+        throw new Error('Not implemented');
+      }


Could you please add comment regarding what this means? 1.2.840.113549.1.7.3 and also what is the regular type, and why is 1.2.840.113549.1.7.3 different from whatever the regular type is.

tomholub · 2021-10-06T09:37:58Z

I'll merge this, then you can address the misisng test and a comment afterwards.

rrrooommmaaa added 14 commits September 14, 2021 14:09

removed dubious code

7d405a1

fix

8d602fe

Merge remote-tracking branch 'origin/master' into issue-2970-signing-…

329c733

…smime

support inline S/MIME armored message

31374a7

fix

6d78d30

fix

05a252e

Implemented S/MIME decryption

061b542

Merge remote-tracking branch 'origin/master' into issue-2970-signing-…

6e1c966

…smime

Merge remote-tracking branch 'origin/master' into issue-2970-signing-…

243c4d9

…smime

sign PKCS#7 message

fa81584

new types

4c2f153

PKCS#7 draft decryption

cd16fa8

Merge remote-tracking branch 'origin/master' into issue-2970-signing-…

9cf7cc7

…smime

Merge remote-tracking branch 'origin/master' into issue-2970-signing-…

0332b47

…smime

small fixes

df989ef

tomholub reviewed Sep 30, 2021

View reviewed changes

This was referenced Oct 5, 2021

Add "Signing Time" to PKCS#7 (if customer requires it) #4045

Open

Test and implement signed S/MIME message with attachments #4046

Open

Implement and test richtext in S/MIME message #4047

Open

Merge remote-tracking branch 'origin/master' into issue-2970-signing-…

10c7076

…smime

rrrooommmaaa mentioned this pull request Oct 5, 2021

Implement sending signed and encrypted S/MIME message #4048

Closed

rrrooommmaaa added 4 commits October 6, 2021 07:53

removed unneeded remarks

2bb338f

reading PKCS#7 message from DER

1e176c5

removed unneeded remarks

3353ef2

removed code duplication

1adeb7f

rrrooommmaaa added 4 commits October 6, 2021 08:26

fix

78700b2

more testing

9cfa19e

simplifications

d310f1e

added some remarks

d6015ab

rrrooommmaaa marked this pull request as ready for review October 6, 2021 09:04

rrrooommmaaa requested a review from tomholub October 6, 2021 09:04

tomholub approved these changes Oct 6, 2021

View reviewed changes

tomholub merged commit f5296eb into master Oct 6, 2021

tomholub deleted the issue-2970-signing-smime branch October 6, 2021 09:38

tomholub mentioned this pull request Oct 6, 2021

extract S/MIME functionality into a library? (end of 2022) #4050

Open

rrrooommmaaa mentioned this pull request Oct 6, 2021

Test signing S/MIME message when passphrase is not cached #4052

Closed

Comments

Conversation

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

Uh oh!

To be filled by reviewers

Uh oh!

rrrooommmaaa commented Sep 28, 2021

Uh oh!

tomholub commented Sep 28, 2021

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

tomholub commented Sep 28, 2021

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

tomholub Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

rrrooommmaaa Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

tomholub Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

rrrooommmaaa Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomholub commented Oct 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rrrooommmaaa commented Sep 14, 2021 •

edited

Loading

rrrooommmaaa commented Sep 28, 2021 •

edited

Loading

rrrooommmaaa commented Sep 28, 2021 •

edited

Loading

rrrooommmaaa commented Oct 6, 2021 •

edited

Loading

tomholub Oct 6, 2021 •

edited

Loading