1240290 - (CVE-2019-11755) Susceptibility to S/MIME Message Takeover Attacks

Status: RESOLVED FIXED

(MailNews Core :: Security: S/MIME, defect)

Description

[Falko Strenzke]

Attachment: thb_mta.pdf

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20160106234230

Steps to reproduce:

Thunderbird is susceptible to a new form of attack which builds on a flaw in the S/MIME standard. 

When Alice sends a signed-then-encrypted (standard format of today's mail clients) mail to Bob, Eve blocks that mail, strips off the signature, replaces it with her own signature and sends it to Bob. Bob now believes the message originates from Eve. If he replies, he potentially discloses information to Eve. Other attack goals are also feasible. See the attachment for details.


Actual results:

Thunderbird displayed the message as validly signed.


Expected results:

Display of an incorrect signature and suspected attack, with the urge not to respond to the email. Ideally, the contents of the mail are not even displayed.

Comment 1

[Daniel Veditz [:dveditz]]

Richard and Kai: could you guys take a look at this crypto attack.

Comment 2

[Joshua Cranmer [:jcranmer]]

I would like to see examples of the mangled resulting message to understand what measures to best take.

It is my opinion that we should only treat signed-then-encrypted messages as having the UI of signed and encrypted, with encrypted-then-signed being displayed as merely encrypted, although I'm not sure how well our current S/MIME code is at being able to control that level of information.

Comment 3

[Richard Barnes [:rbarnes]]

Is the following an accurate summary of the attack?

1. Alice constructs Encrypt(Bob_pub, Sign(Alice_priv, message))
2. Eve recovers a slightly garbled message2 = Encrypt(Bob_pub, message) by stripping Alice's signature
3. Eve constructs Sign(Eve_priv, message2)
4. Thunderbird verifies Eve's signature over message2, and ignores the garbling on message2

In that case, I agree that this is a vulnerability, and I agree that we should disable encrypt-then-sign messages.  (That's been the best practice for years now, hasn't it!?)  I don't know the Thunderbird code base at all, so unfortunately I have no idea how to do this.

Comment 4

[Richard Barnes [:rbarnes]]

I could live with displaying messages as encrypted-not-signed (vs. rejecting), but this seems like a pretty fine distinction for users to get.

Comment 5

[Joshua Cranmer [:jcranmer]]

(In reply to Richard Barnes [:rbarnes] from comment #4)
> I could live with displaying messages as encrypted-not-signed (vs.
> rejecting), but this seems like a pretty fine distinction for users to get.

Yeah, I realize that the distinction is fine. But "merely" encrypted or signed messages already exist and occur in practice (e.g., the message that these notifications give me), so it's not like it's a distinction that doesn't already exist.

(In reply to Richard Barnes [:rbarnes] from comment #3)
> In that case, I agree that this is a vulnerability, and I agree that we
> should disable encrypt-then-sign messages.  (That's been the best practice
> for years now, hasn't it!?)  I don't know the Thunderbird code base at all,
> so unfortunately I have no idea how to do this.

I know the MIME code better than anybody else, so I can make necessary changes (although finding reviewers for that will be a challenge). I'm substantially less confident on being able to make security UI decisions, and my crypto knowledge is effectively limited to "I know what these things are and sort of when to use them and I know I shouldn't try to come up with this stuff myself."

Comment 6

[Kai Engert [:KaiE:]]

I can try to look at this by Thursday.

So far I've only had a brief look. What I don't understand yet: If a message is "signed-then-encrypt", the inner part is signed, and everything is wrapped in an encrypted container. If that's true, if the attacker cannot decrypt, how can the attacker strip the inner signature?

Comment 7

[Falko Strenzke]

Attachment: plaintext MIME after cutting off a signature accepted by TB as unsigned

This is a resulting plaintext MIME as the recipient will decrypt it. The attacker has removed the signature attachment by truncating the ciphertext while keeping intact the final two blocks in order not to disturb the padding.

Comment 8

[Falko Strenzke]

(In reply to Joshua Cranmer [:jcranmer] from comment #2)
> I would like to see examples of the mangled resulting message to understand
> what measures to best take.
> 
Added new attachment decrypted_of_truncated

> It is my opinion that we should only treat signed-then-encrypted messages as
> having the UI of signed and encrypted, with encrypted-then-signed being
> displayed as merely encrypted, although I'm not sure how well our current
> S/MIME code is at being able to control that level of information.

Sounds like a good suggestion.

Comment 9

[Falko Strenzke]

(In reply to Richard Barnes [:rbarnes] from comment #3)
> Is the following an accurate summary of the attack?
> 
> 1. Alice constructs Encrypt(Bob_pub, Sign(Alice_priv, message))
> 2. Eve recovers a slightly garbled message2 = Encrypt(Bob_pub, message) by
> stripping Alice's signature
> 3. Eve constructs Sign(Eve_priv, message2)
> 4. Thunderbird verifies Eve's signature over message2, and ignores the
> garbling on message2
> 
Yes, that is correct.

> In that case, I agree that this is a vulnerability, and I agree that we
> should disable encrypt-then-sign messages.  (That's been the best practice
> for years now, hasn't it!?)  I don't know the Thunderbird code base at all,
> so unfortunately I have no idea how to do this.

Comment 10

[Falko Strenzke]

Here is a link which provides some more background on the attacks (but no additional information relevant for the fixing of Thunderbird):

http://cryptosource.de/posts/smime_mta_en.html

Comment 11

[Falko Strenzke]

Attachment: An example where the attacker strips of the beginning of the email.

I am adding another plaintext example. It is achieved by removing the beginning of the MIME:

================>
Content-Type: multipart/signed; protocol="application/pkcs7-signature"; micalg=sha-512; boundary="------------ms020405050706010302010500"

--------------ms020405050706010302010500
Content-Type: multipart/alternative;
 boundary="------------030900070605060900030608"

This is a multi-part message in MIME format.
--------------030900070605060900030608

<=================

Accordingly the message starts with 
===================>
Content-Type: text/plain; charset=utf-8
<===================
The attacker can always produce this from the original email by removing leading ciphertext blocks and if the length of the to-be-removed text is not a multiple of eight blocks, he can render the leading chars of the first line as whitespaces, which is currently accepted by Thunderbird. But even if he doesn't achieve the latter, this hardly matters, as when Thunderbird doesn't find the initial MIME header, it just displays the message as plain text and thus we must assume the attack as successful. If that is what an email client must commit to, then there is no beautiful defence. What remains is to actively look for 
===>
Content-Type: application/pkcs7-signature; name="smime.p7s"
<===
and deduce from this the presence of a signature and that the message was originally multipart/signed.
If the attacker destroys that text part with bit-flip errors (which always can, he knows the signature offset from the end), than at least invalid character codes should result which can also be detected.

Comment 12

[Eric Rescorla (:ekr)]

dveditz, what did you need here?

Comment 13

[chris hofmann]

Trying to figure out the bounty for this bug.  We need a rating. Sounds like sec-high is confirmed. 

And we need to understand if this is a spec bug or just an implementation.

Also sounds like comment 9 proposes a path toward fixing by disabling encrypt-then-sign messages.  Is that the path we intend to take?

Comment 14

[Falko Strenzke]

(In reply to chris hofmann from comment #13)
> Trying to figure out the bounty for this bug.  We need a rating. Sounds like
> sec-high is confirmed. 
> 
> And we need to understand if this is a spec bug or just an implementation.
> 
> Also sounds like comment 9 proposes a path toward fixing by disabling
> encrypt-then-sign messages.  Is that the path we intend to take?

It is definitely a spec bug.

Comment 15

[Adam Muntner [:adamm] (use NEEDINFO)]

(In reply to Kai Engert (:kaie) from comment #6)
> I can try to look at this by Thursday.
> 
> So far I've only had a brief look. What I don't understand yet: If a message
> is "signed-then-encrypt", the inner part is signed, and everything is
> wrapped in an encrypted container. If that's true, if the attacker cannot
> decrypt, how can the attacker strip the inner signature?

Kai is correct in msg #6. The original issue report said, "When Alice sends a signed-then-encrypted (standard format of today's mail clients) mail to Bob, Eve blocks that mail, strips off the signature, replaces it with her own signature and sends it to Bob."

I think we are talking about two different issues here. 

Encrypt-then-sign has been a known-bad order of operations for a long time. If Thunderbird currently supports this, the feature should be disabled. 

The issue Falko reported is different, it's a weakness in sign-then-encrypt. 

Reading the attached PDF description of the attack, it says:

"Eve learns the Email Format used by Alice
In order for Eve to learn the length of the MIME signature appended to the clear text she requests from Alicea signed email. The format of the plaintext message in our example attack is
as in Figure 1. Eve determines the number of blocks to remove from end of the
ciphertext in order to strip o  the signature, where she keeps intact the last two
blocks in order not to disturb the padding."

Falko, if I understand correctly, to perform this attack, the attacker (Eve) must get Alice to send her(Eve) an encrypted then signed email. Once learning the structure of the mail, as the PDF explains, "Eve determines the number of blocks to remove from end of the ciphertext in order to strip o  the signature, where she keeps intact the last two blocks in order not to disturb the padding."

This is a form of known-plaintext attack.

Then, Eve must be in position to intercept, modify, and reinject the modified message. For the attack to work, "...Eve applies an outer S/MIME signature with her own private key to the enveloped data before she sends the email to Bob."

If I'm reading this correctly, the 'sends the mail to Bob' convincingly part is not trivial and would require network positioning or mail server mis-configuration. 

If all that is true, this bug should be ranked sec-moderate. 

https://wiki.mozilla.org/Security_Severity_Ratings
"Vulnerabilities which can provide an attacker additional information or positioning that could be used in combination with other vulnerabilities. Disclosure of sensitive information that represents a violation of privacy but by itself does not expose the user or organization to immediate risk. The vulnerability combined with another moderate vulnerability could result in an attack of high or critical severity (aka stepping stone)."

Comment 16

[Adam Muntner [:adamm] (use NEEDINFO)]

Falko blogged about the category of attack in January, though didn't mention Thunderbird by name. 
http://cryptosource.de/posts/smime_mta_en.html

Comment 17

[Falko Strenzke]

(In reply to Adam Muntner [:adamm] from comment #15)
> (In reply to Kai Engert (:kaie) from comment #6)
> > I can try to look at this by Thursday.
> > 
> > So far I've only had a brief look. What I don't understand yet: If a message
> > is "signed-then-encrypt", the inner part is signed, and everything is
> > wrapped in an encrypted container. If that's true, if the attacker cannot
> > decrypt, how can the attacker strip the inner signature?
> 
> Kai is correct in msg #6. The original issue report said, "When Alice sends
> a signed-then-encrypted (standard format of today's mail clients) mail to
> Bob, Eve blocks that mail, strips off the signature, replaces it with her
> own signature and sends it to Bob."
> 
> I think we are talking about two different issues here. 
> 
> Encrypt-then-sign has been a known-bad order of operations for a long time.
> If Thunderbird currently supports this, the feature should be disabled. 
> 
> The issue Falko reported is different, it's a weakness in sign-then-encrypt. 
> 
> Reading the attached PDF description of the attack, it says:
> 
> "Eve learns the Email Format used by Alice
> In order for Eve to learn the length of the MIME signature appended to the
> clear text she requests from Alicea signed email. The format of the
> plaintext message in our example attack is
> as in Figure 1. Eve determines the number of blocks to remove from end of the
> ciphertext in order to strip o  the signature, where she keeps intact the
> last two
> blocks in order not to disturb the padding."
> 
> Falko, if I understand correctly, to perform this attack, the attacker (Eve)
> must get Alice to send her(Eve) an encrypted then signed email. Once
Just any signed email from Alice will do (encrypted either way or not at all).
> learning the structure of the mail, as the PDF explains, "Eve determines the
> number of blocks to remove from end of the ciphertext in order to strip o 
> the signature, where she keeps intact the last two blocks in order not to
> disturb the padding."
> 
> This is a form of known-plaintext attack.
> 
> Then, Eve must be in position to intercept, modify, and reinject the
> modified message. For the attack to work, "...Eve applies an outer S/MIME
> signature with her own private key to the enveloped data before she sends
> the email to Bob."
> 
> If I'm reading this correctly, the 'sends the mail to Bob' convincingly part
> is not trivial and would require network positioning or mail server
> mis-configuration. 
> 
> If all that is true, this bug should be ranked sec-moderate. 
> 
> https://wiki.mozilla.org/Security_Severity_Ratings
> "Vulnerabilities which can provide an attacker additional information or
> positioning that could be used in combination with other vulnerabilities.
> Disclosure of sensitive information that represents a violation of privacy
> but by itself does not expose the user or organization to immediate risk.
> The vulnerability combined with another moderate vulnerability could result
> in an attack of high or critical severity (aka stepping stone)."
I certainly don't claim to be know your rules for severity ratings in detail, but I don't see that the message takeover attacks need another vulnerability to be mounted. "Network positioning" is trivial to achieve. If the attacker runs a WLAN hotspot he is already in a network position.

Please note that what you described above is just the weaker variant of the attack. This one could indeed be mitigated by more strict MIME parsing of Thunderbird. The more general attack is described at
http://cryptosource.de/posts/smime_mta_improved_en.html
The only real countermeasure against that one is to disable the acceptance of outer signatures in the recipient's client.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Wayne, is there something you can do to move this along and either resolve the underlying issue or make a decision that we're not going to for TB?

Comment 19

[Wayne Mery (:wsmwk)]

(In reply to Al Billings [:abillings] from comment #18)
> Wayne, is there something you can do to move this along and either resolve
> the underlying issue or make a decision that we're not going to for TB?

The NI still exists for kaie, though I'm not sure what else he intends to do.

S/mime has no module owners or peers listed, so cc:ing some mime peers who perhaps can decide on next step

Comment 20

[Kai Engert [:KaiE:]]

The sad reality is that I still haven't found time to look into this bug :-(

Comment 21

[Wayne Mery (:wsmwk)]

FWIW, clokep agrees about "suggestions for not displaying it as encrypted & signed makes sense."

Comment 22

[Wayne Mery (:wsmwk)]

Patrick can you offer some perspective?

Comment 23

[Patrick Brunschwig]

(In reply to Falko Strenzke from comment #17)

> The only real countermeasure against that one is to disable the acceptance
> of outer signatures in the recipient's client.

I don't fully agree. If the outer signature is created with the same certificate as the signature in the encrypted message, then the message is valid and the signature is accepted. This is known as sign + encrypt + sign, and is the only possible proof that the message is really encrypted and signed by the same person.

That is, the real countermeasure against this attack is to disable the acceptance of outer signatures in the recipient's client, if there is no signature inside the encrypted message. 

I will try to find out if I can fix this.

Comment 24

[Falko Strenzke]

(In reply to Patrick Brunschwig from comment #23)
> (In reply to Falko Strenzke from comment #17)
> 
> > The only real countermeasure against that one is to disable the acceptance
> > of outer signatures in the recipient's client.
> 
> I don't fully agree. If the outer signature is created with the same
> certificate as the signature in the encrypted message, then the message is
> valid and the signature is accepted. This is known as sign + encrypt + sign,
> and is the only possible proof that the message is really encrypted and
> signed by the same person.
> 
> That is, the real countermeasure against this attack is to disable the
> acceptance of outer signatures in the recipient's client, if there is no
> signature inside the encrypted message. 
> 
> I will try to find out if I can fix this.

Yes, I that is correct.

Comment 25

[Stephanie Ouillon [:arroway] (needinfo me)]

Sec-high triage: Patrick, thanks for taking this bug. Did you have any luck finding a fix for it?

Comment 26

[Patrick Brunschwig]

I was on holidays, and I'm pretty busy with many other things. I said I will look into it, but it won't happen in the next 1-2 weeks. Given that this bug is already more than a year old, I don't think it's a problem if it needs to wait a little longer.

Comment 27

[Stephanie Ouillon [:arroway] (needinfo me)]

(In reply to Patrick Brunschwig from comment #26)
> Given that this bug is already more than a year old, I don't think it's a problem if it needs to wait a little longer.

This is a common misconception. 
The longer a bug goes unfixed, the more chance it has to be found (and exploited) by other people than us.
Actually, a research paper showed that "Between 15% and 20% of all vulnerabilities in browsers have at least one duplicate. For data available on Android between 2015 and 2016, 22% of vulnerabilities are rediscovered at least once an average of 2 months after their original disclosure. There are reasons to believe that the actual rate is even higher for certain types of software." [1]

So having a one year old sec-high bug waiting to be fixed _is_ a problem. 

[1] https://papers.ssrn.com/sol3/papers.cfm?abstract_id=2928758

Comment 28

[Frederik Braun [:freddy]]

Al, the sec-bounty flag is over a year old. What's preventing us from making a decision?

Comment 29

[Al Billings [:abillings - ex-MoCo]]

(In reply to Frederik Braun [:freddyb] from comment #28)
> Al, the sec-bounty flag is over a year old. What's preventing us from making
> a decision?

Nothing but we ask people to write to [e-mail] for these sorts of questions and to not have conversations in the bug about it.

Comment 30

[Frederik Braun [:freddy]]

(In reply to Patrick Brunschwig from comment #26)
> I was on holidays, and I'm pretty busy with many other things. I said I will
> look into it, but it won't happen in the next 1-2 weeks.

This happened a month ago.
What's the timeline here?

Comment 31

[Patrick Brunschwig]

I tried to fix this, but I don't know enough about the S/MIME code in Thunderbird and the S/MIME protocol in general.

Comment 32

[Philipp Kewisch [:Fallen][📱📆]]

Joshua, any chance you could look into this one as you've already made some starting comments earlier? I am happy to help with UI or reviewing if needed.

Comment 33

[Joshua Cranmer [:jcranmer]]

So every time I look at this bug, I keep exceeding the amount of time I have allotted for making progress.

The main question is what we would agree constitutes a "fix" for this bug. There seems to be agreement that making the UI for encrypt-and-sign reflecting a different status (i.e., just encrypted or maybe even questionable security status) is necessary. What countermeasures beyond that are necessary, desirable, or even feasible is unclear to me. It looks like switching from CBC to some other mode (GCM?) might be warranted, but my knowledge turns into a blackbox around the CMS format layer, so I don't know what algorithms are possible here, either from a spec standpoint or a deployed standpoint. In any case, that sort of poking around would need to be done in NSS, not mailnews code (we basically just say "hey, NSS, give me an algorithm I can use for all these recipients").


(In reply to Falko Strenzke from comment #17)
> I certainly don't claim to be know your rules for severity ratings in
> detail, but I don't see that the message takeover attacks need another
> vulnerability to be mounted. "Network positioning" is trivial to achieve. If
> the attacker runs a WLAN hotspot he is already in a network position.

Most email traffic is conducted over SSL, particularly MUA<->MTA traffic. You have to click through a scary dialog or two to configure an account in Thunderbird to not use SSL or STARTTLS.

Comment 34

[Falko Strenzke]

(In reply to Joshua Cranmer [:jcranmer] from comment #33)
> So every time I look at this bug, I keep exceeding the amount of time I have
> allotted for making progress.
> 
> The main question is what we would agree constitutes a "fix" for this bug.
> There seems to be agreement that making the UI for encrypt-and-sign
> reflecting a different status (i.e., just encrypted or maybe even
> questionable security status) is necessary. 
I would definitely suggest that outer signatures should be indicated as questionable to the user. There should be almost no clients in the field which produce such today, I suppose. As a matter of fact, merely encrypted emails offer only limited security, this is even directly admitted by the S/MIME standard. So it might really be good to indicate to the user that a merely encrypted email might have been tempered with.
> What countermeasures beyond that
> are necessary, desirable, or even feasible is unclear to me. 
It would help if Thunderbird would be able to detect the cutting out of an inner signature. But that is entirely about MIME parsing, the specifics of which I do not know much about. However, it seems quite straightforward to me, that the following measures should help:
- seeing "Content-Type: application/pkcs7-signature" and no signature in the MIME should lead to an invalid message indicated to the user. This is certainly mostly only meaningful for encrypted messages.
- an encrypted message for which the mime parsing fails, and which thus would now be displayed to the user as raw text, should, in a secure version, not be displayed at all or at least only after displaying a clear warning to the user. This would cause a compatibility problem with clients who send plain text messages without any MIME encoding. I suppose that there  aren't any clients today which show such a behavior.
> It looks like
> switching from CBC to some other mode (GCM?) might be warranted, but my
> knowledge turns into a blackbox around the CMS format layer, so I don't know
> what algorithms are possible here, either from a spec standpoint or a
> deployed standpoint. In any case, that sort of poking around would need to
> be done in NSS, not mailnews code (we basically just say "hey, NSS, give me
> an algorithm I can use for all these recipients").
It is not possible to use GCM or any other authenticated encryption mode in S/MIME so far, to the best of my knowledge. In their reaction to the published attacks the S/MIME group started discussing this topic, but I havent't heard anything about it actually being done.

> 
> 
> (In reply to Falko Strenzke from comment #17)
> > I certainly don't claim to be know your rules for severity ratings in
> > detail, but I don't see that the message takeover attacks need another
> > vulnerability to be mounted. "Network positioning" is trivial to achieve. If
> > the attacker runs a WLAN hotspot he is already in a network position.
> 
> Most email traffic is conducted over SSL, particularly MUA<->MTA traffic.
> You have to click through a scary dialog or two to configure an account in
> Thunderbird to not use SSL or STARTTLS.
Note that afterwards I also devised a different variant of the attack, where the attacker doesn't need to block the original message.
Generally I tend to think that any argument in favor of not needing to take this type of attack serious could, as it seems to me, equally be used to question the necessity of S/MIME in general. If there are other measures, such as STARTTLS, which one completely relies on, then S/MIME is indeed superfluous. But there are good reasons for end-to-end security. For instance email accounts may be hacked. Access to an encrypted (and signed) message would be sufficient to mount an attack. Or someone might have good reasons not to trust the STARTTLS certificates or the service provider entirely.

Comment 35

[Joshua Cranmer [:jcranmer]]

(In reply to Falko Strenzke from comment #34)
> It would help if Thunderbird would be able to detect the cutting out of an
> inner signature. But that is entirely about MIME parsing, the specifics of
> which I do not know much about. However, it seems quite straightforward to
> me, that the following measures should help:

MIME is one of those things where almost any input is valid (kind of like HTML 5). In theory, you could try to deal with a missing header block or malformed base64/qp content, but that's really about it. Extraneous MIME boundaries aren't really detectable, especially because there's reason to send content that could be MIME boundaries (diffs, for examples).

In practice, libmime can't really pass parser failures around, even if we did indicate them. Even trying to make decisions on the basis of child parts can be difficult (don't attempt to understand how multipart/related and multipart/alternative work, you don't want to).

> - seeing "Content-Type: application/pkcs7-signature" and no signature in the
> MIME should lead to an invalid message indicated to the user. This is
> certainly mostly only meaningful for encrypted messages.

You mean an application/pkcs7-signature that's not an immediate child of multipart/signed? Since it's going to be the MIME type picked for a detached signature, I can see it plausible that someone is going to want to send it as an attachment.

> - an encrypted message for which the mime parsing fails, and which thus
> would now be displayed to the user as raw text, should, in a secure version,
> not be displayed at all or at least only after displaying a clear warning to
> the user. This would cause a compatibility problem with clients who send
> plain text messages without any MIME encoding. I suppose that there  aren't
> any clients today which show such a behavior.

As mentioned above, MIME parsing failures don't really exist, and libmime makes it really difficult to pass them around even if they did.

> Generally I tend to think that any argument in favor of not needing to take
> this type of attack serious could, as it seems to me, equally be used to
> question the necessity of S/MIME in general. If there are other measures,
> such as STARTTLS, which one completely relies on, then S/MIME is indeed
> superfluous. But there are good reasons for end-to-end security. For
> instance email accounts may be hacked. Access to an encrypted (and signed)
> message would be sufficient to mount an attack. Or someone might have good
> reasons not to trust the STARTTLS certificates or the service provider
> entirely.

I don't deny that S/MIME issues can be very grave. But this does need to be balanced by the fact that the standard network encryption argument of "coffee shop wifi" isn't sufficient to motivate practical attacks with S/MIME.

P.S. I'm going to be busy for the rest of this week, so I can't do any work on it for now.

Comment 36

[Stephanie Ouillon [:arroway] (needinfo me)]

Joshua, have you been able to work again on this bug?

Comment 37

[Kai Engert [:KaiE:]]

Trying to summarize:

- S/MIME doesn't provide any protections whatsoever, which could allow the processing of S/MIME message
  to detect manipulation of the data stream (no checksum over the entirety of the stream)

- Eve learns about the size of signatures usually created by Alice

- despite not being able to decrypt, Eve knows which parts of the encrypted stream contain the inner signature

- Eve removes the parts that likely contain the inner signature, and replaces it with a different encrypted signature,
  a signature chosen by Eve

- because we cannot check the consistency of the stream, but process S/MIME messages on a best effort basis,
  we display the replacement signature


What I don't understand:

If Eve is unable to decrypt the inner message, how is Eve able to create a fake signature that matches the message content?

Why don't we detect that the fake signature doesn't match the decrypted message?

Why are we displaying a valid signature, if the signature doesn't match the content?

Comment 38

[Kai Engert [:KaiE:]]

Could we get a message example that we could use to debug and test?

We'd require a certificate (including private key) that everyone can use to import into TB, so we that every developer and QA person working on this issue is able to decrypt the messages.

We should get a copy of the original signed message from Alice, and a copy of the manipulated message from Eve. Ideally, to simplify testing, the person who helps with reproducing this issue could create a new mailbox in the Thunderbird Local Folders, copy only these two messages into the folder, and then find the mailbox file on your local disk (TB profile directory/Mail/Local Folders), find that mailbox file (excluding .msf) and attach it to this bug.

I've tried to use the attachment named
   "An example where the attacker strips of the beginning of the email."
and create a local mailbox containing that message,
but Thunderbird doesn't display any signature UI for it.

Comment 39

[Joshua Cranmer [:jcranmer]]

(In reply to Kai Engert (:kaie:) from comment #37)
> If Eve is unable to decrypt the inner message, how is Eve able to create a
> fake signature that matches the message content?
> 
> Why don't we detect that the fake signature doesn't match the decrypted
> message?
> 
> Why are we displaying a valid signature, if the signature doesn't match the
> content?

AIUI, what's going is this:

1. Eve intercepts original sign & encrypt message
2. Eve scrunches out the signature so that it's just an encrypted message.
3. Eve wraps the message in a new sign part so it looks like Encrypt & Sign.
4. Bob sees message as signed & encrypted, coming from Eve.

(In reply to Stephanie Ouillon [:arroway] from comment #36)
> Joshua, have you been able to work again on this bug?

No, I haven't had time.

Comment 40

[Kai Engert [:KaiE:]]

Is every byte encrypted individually? If yes, I agree that the surgery seems possible.

Or are we encrypting in chunks? Then it seems that it's difficult to cut away exactly the outer parts that are required.

Comment 41

[Joshua Cranmer [:jcranmer]]

It's block-encryption. You'd lose the last few blocks, which would include part of the end of the message. For HTML messages, we're ending with CRLF  </body>CRLF</html>, which gives you some slack in things you could chomp the end of without people noticing.

Comment 42

[Kai Engert [:KaiE:]]

Does it work without stripping anything away from the beginning of the message?

Comment 43

[Joshua Cranmer [:jcranmer]]

Attachment: Forbid encrypt & sign (outdated, crashes)

Well, I'm basically cramming three things in this patch:

1. A framework to make decisions about validity that is more holistic than "yep, it's signed" and "yep, it's encrypted."
2. A cleanup of the libmime integration with S/MIME (basically, I shunted most of the verification logic from mimecms.cpp into SMimeInfo.cpp, threw out dead code, and then cleaned up most of the shunted code to follow more modern C++ style guidelines).
3. New logic for validity that accepts only signed, encrypted, sign-and-encrypt, and sign-encrypt-sign as valid S/MIME forms. (i.e., encrypt-and-sign is now forbidden).

I'd call it narrow, but it's narrow only insofar as I'm not really changing the way we communicate the security, even though I've built the platform to do that. As a result, the resulting UI is a little suboptimal. I'd love to see a more fundamental rethinking of UI (particularly in regards to explaining why we're making the decision we're displaying), but this is not the bug for it.

This turned out to be more complicated than I expected--I ultimately had to change the interface from what I was planning (namely, the nsIMsgSMIMEHeaderSink was going to get the statuses reported in postorder MIME traversal, but that turned out to be far more difficult than it was worth). I did write comments to try to explain what's going on, but I wrote them at different stages in the attempt to get the code working correctly, so I may not have fixed all the comments to reflect this version of the patch. If the comments (or the names) are confusing or seem not to match what the code is doing, please tell me, and I'll try to fix them ASAP.

As for testing... well, I'm not going to subject you to my half-baked automated testing for S/MIME. The easiest thing to do is to generate some local certificates and create some encrypt-and-sign and sign-and-encrypt messages (using openssl smime) and see what the status indicators display. I haven't tested all possible error paths, and seeing what happens in situations like opening up attachments is probably a good idea.

Comment 44

[Eric Rescorla (:ekr)]

This definitely is going to need some automated testing.

Comment 45

[Jorg K (CEST = GMT+2)]

No comments from the reviewer doesn't mean the reviewer isn't doing anything. I'm gearing up to test this, creating certificates, etc.

My first attempt to compile with the patch failed, you need to:
-class MimeObject;
+struct MimeObject;
in SMimeInfo.h. Strange, I thought the Windows compiler is more lenient than any other.

As a basic test I tried signed, encrypted, signed+encrypted messages.

Next would be encrypted+signed and signed+encrypted+signed (see comment #23). That will take longer since I need to handcraft those.

Sadly just opening a folder into which I have placed three messages exported from TB Daily gives a crash whose stack I will attach. That's before I imported my own certificate into a new profile. It works better after the import of own certificate with which I signed the messages ;-)

Comment 46

[Jorg K (CEST = GMT+2)]

Attachment: (for old patch) 1240290-crash.txt - crash on now profile with no certificates installed

Comment 47

[Jorg K (CEST = GMT+2)]

Attachment: (for old patch) 1240290-crash2.txt

After installing my certificate, importing a signed message by dragging it onto a folder also crashes, see enclosed.

I also saw some JS errors on the console:
JavaScript error: chrome://messenger-smime/content/msgHdrViewSMIMEOverlay.js, line 163: TypeError: result is null

I think I'll stop testing now until a new patch arrives.

If it doesn't crash, the three messages signed, encrypted and signed+encrypted are shown with the correct icons.

Comment 48

[Jorg K (CEST = GMT+2)]

I saw another problem: I have two sent messages, one encrypted, one signed+encrypted.

I have a self-signed certificate issued be a fictitious CA which I also created, both following:
https://gist.github.com/richieforeman/3166387

In a new profile, I imported those messages, and my personal certificate, not the CA certificate under "Authorities".
In a Daily, I can decrypt/display those two messages, with your patch I cannot display the signed+encrypted. Is that intentional? (An only signed message displays with an (x) on the envelope to indicate a broken certificate.)

Sorry about these silly test cases, they came about while I was preparing the test environment.

Comment 49

[Jorg K (CEST = GMT+2)]

Comment on attachment 8903424 [details] [diff] [review]
Forbid encrypt & sign (outdated, crashes)

I think this needs another round, clearing r? for now.

Comment 50

[Stephanie Ouillon [:arroway] (needinfo me)]

Joshua, is that still your plan to continue working on this patch?

Comment 51

[Joshua Cranmer [:jcranmer]]

(In reply to Stephanie Ouillon [:arroway] from comment #50)
> Joshua, is that still your plan to continue working on this patch?

Yeah. I've got some updates, but I need to double-check that I've fixed all of the problems Jorg found.

Comment 52

[Frederik Braun [:freddy]]

Josh, please let us know if there's anything we can help you with, unblocking this bug.

Comment 53

[Frederik Braun [:freddy]]

Jorg, maybe you can push this over the finish line yourself?

Comment 54

[Jorg K (CEST = GMT+2)]

I think you've misunderstood my job description:
https://mail.mozilla.org/pipermail/tb-planning/2016-December/005142.html
I appreciate the importance and severity of security bugs, but we can't have the project come to a grinding halt because the only paid developer is not dedicating time to his prime tasks any more.

Since TB is an open source maybe you academics want to contribute to the fix. If you think this isn't receiving enough attention, please get in touch with the Thunderbird Council ([e-mail]).

Sorry I can't give you a better answer.

Comment 55

[Jorg K (CEST = GMT+2)]

Sorry, I mixed up the reporter here. So this should read:
Since TB is an open source, maybe *the* academics want to contribute to the fix.

Comment 56

[Magnus Melin [:mkmelin]]

I was staring at this bug for a long time, but then I realized this is actually fixed now as part of the efail fixes, specifically bug 1464056.

Since bug 1464056 we don't (unless mail.decrypt_children) decrypt sub-parts of a message, meaning we have essentially already disabled encrypt+sign. Now only sign+encrypt is supported since only then the top level type would be application/pkcs7-mime. 

Can anyone confirm? Did I misunderstand something?
And yes, tripple-wrap would now be broken :/

Comment 57

[Falko Strenzke]

What do you mean by "sign+encrypt". Does that mean "sign-then-encrypt" or "encrypt-then-sign" ? What prevents the MTA attacks is when the recipient insists on encrypted-then-signed messages. The outer signature ensures that the encrypted message is integer. Purely encrypted messages are certainly still affected.

Nice if the eFail countermeasures cover also this bug. But this also seems to suggests that the countermeasures for this bug, had they ever been implemented, would have at least mitigated eFail.

Comment 58

[Magnus Melin [:mkmelin]]

Yes s/+/-then-/

Good point, encrypted-only messages would still leak the then decrypted content back to the sender in a reply. Not sure what to do about that. If the encrypted part isn't signed-the-encrypted there's no knowing who encrypted it.

I didn't see any suggestions for how to handle that. Suggestions?

Comment 59

[Falko Strenzke]

(In reply to Magnus Melin from comment #58)
> Yes s/+/-then-/
> 
> Good point, encrypted-only messages would still leak the then decrypted
> content back to the sender in a reply. Not sure what to do about that. If
> the encrypted part isn't signed-the-encrypted there's no knowing who
> encrypted it.
> 
> I didn't see any suggestions for how to handle that. Suggestions?

I think the only case which potentially can be defended against is where the original email was signed-then-encrypted. In that case, as I understand it, the decrypted message will start with a line

Content-Type: multipart/signed; protocol="application/pkcs7-signature"; micalg=sha-512; boundary="------------ms040709010602000909080300" 

If then no actual signature follows, the message can be classified as having been tampered with. Forging a useful first line should not be easily possible.

Comment 60

[Magnus Melin [:mkmelin]]

In that case the message would be noted as unsigned. Or perhaps broken signature. (These things are hard to test.)

An encrypted-only message, or one with broken signature could leak information back in a reply. But given that the encrypted message can't be only a subpart anymore, the attacker can't e.g. add "Hey, what do you think about the below?"

Comment 61

[Jorg K (CEST = GMT+2)]

Kai, here's another building site ("Baustelle") left unfinished.

Comment 62

[Kai Engert [:KaiE:]]

Let's try to summarize again what's remaining.

In my understanding, at the time this bug was reported, TB supported processing of "encrypt-then-sign" email (outer layer is a signature). If the outer signature was valid, Thunderbird would display a valid signature icon.

After efail, is has been claimed in this bug, that recent versions of TB no longer support messages that are "encrypt-then-sign" and never display signature status for those. Is my understanding of the claim correct?

We should verify the claim is correct. We now have some automated tests for S/MIME. We should add an appropriate test case, before we proceed with the discussion. I can work on that.

Comment 63

[Jorg K (CEST = GMT+2)]

(In reply to Kai Engert (:kaie:) from comment #62)

After efail, is has been claimed in this bug, that recent versions of TB no longer support messages that are "encrypt-then-sign" and never display signature status for those. Is my understanding of the claim correct?

Apparently so, see comment #56, second paragraph.

It would be a pity to lose Joshua's clean-up work, see comment #43.

Comment 64

[Kai Engert [:KaiE:]]

I produced two additional test messages.
Both use an inner encrypted message part.

The first test message uses S/MIME clear signing, multipart/signed, for the outer layer.
Opening that message displays a message with an "invalid signature status" icon.
The inner encrypted text isn't decrypted, the inner plaintext contents aren't shown.

I've toggled pref mailnews.p7m_subparts_external to false (default is true). With this change, the inner contents are decrypted. In addition, I get the encryption status icon.

I guess this confirms that the efail fix prevents the non-top-level message from being decrypted, in the multipart/signed scenario.

For the second test message, I used opaque signing. That is, we have only one MIME part, which is the base64 encoded CMS signature. Embedded inside the CMS signature is the encrypted CMS message.

This message is shown with an encryption icon, plus a valid signature status icon, and the inner message is decrypted and shown.

This means, we still process some encrypted-then-signed messages.

Comment 65

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie:) from comment #64)

For the second test message, I used opaque signing. That is, we have only one MIME part, which is the base64 encoded CMS signature.

Well, we have an additional MIME part nested insigned the opaque signature. That part is visible only after the decoding performed in mimecms.cpp

If our intention is to prevent nested encrypted messages, we must teach mimecms.cpp to peek at the content type it is decoding, and forbid it to process a contained encrypted message.

Comment 66

[Kai Engert [:KaiE:]]

FYI, I have started to work on fixes. Including the tests, won't be a small amount of changes. I'll file a separate bug (tomorrow) to discuss the change (and CC all of you), so we can separate the general discussion from the suggested first round of fixes (I don't know if there will be more to come).

Comment 67

[Kai Engert [:KaiE:]]

Attachment: 1240290-nss-testsuite.patch

This patch creates new test cases for enveloped-then-signed messages.

It should be attached to, and reviewed in, a separate NSS bug. I'll postpone that, to avoid getting too much attention for now.

Comment 68

[Kai Engert [:KaiE:]]

Attachment: 1240290-test-data.patch

This patch was created by refreshing the NSS test suite data, with NSS that includes the previous patch. It includes the additional envelope-then-sign testcases.

Comment 69

[Kai Engert [:KaiE:]]

Sigh. Somehow, previously, I had failed to notice that Joshua had attached a patch. Sorry. I have worked on an alternative patch.

Looking at Josh's patch now, my patch is less invasive. Given that Joshua's patch had triggered crashes, we don't know what side effects that rework might cause, and given that the S/MIME code has changed in 2018 during the EFAIL fixes, and given that we recently changed how the S/MIME code checks that a status update is related to the currently shown message (based on URIs), it might be useful to start with my patch, which is also just half the size.

Comment 70

[Kai Engert [:KaiE:]]

Attachment: 1240290-fix-v2.patch

Comment 71

[Kai Engert [:KaiE:]]

Attachment: 1240290-nss-testsuite-v2.patch (will be reviewed in separate NSS bug)

Comment 72

[Kai Engert [:KaiE:]]

Attachment: 1240290-test-data-v2.patch

Comment 73

[Kai Engert [:KaiE:]]

The released versions of TB, already no longer display any S/MIME signature status for Falko's attached sample messages.

However, there's at least one similar scenario, which is unfixed in released versions, and which is fixed by the latest patch.
I'll iterate through the scenarios, which are covered by the added tests.

As a reminder, "clear text signed" means the "multipart/signed" mechanism (actively used by Thunderbird when creating signatures),
while "opaque signed" means the "application/pkcs7-mime" type, which embeds the message and signature in a CMS blog, but without encryption (this kind of signature is used by Outlook, and supported by Thunderbird for decoding and display).

In all scenarios below that use two signatures, the first (inner signature) is by Alice.

(1) Inner Encrypted, then outer "opaque signed".

Released Thunderbird shows that as correctly signed and encrypted. Because the inner encrypted message could have originally been intended for a different recipient, we had already decided that we don't want to allow/display this kind of combination.

The message contents are shown.

The efail fixes have been incomplete to detect this scenario, which consists of two nested "application/pkcs7-mime" messages.

With the fix applied:
Message NOT shown, invalid signature.

(2) Inner Encrypted, then outer "clear-text signed".

Released TB shows that as "invalid signature".

Message contents are NOT shown, rather we get an attachment.

With the fix applied:
Same.

(3) Similar to 1, but with an additional outer signature layer. Inner Encrypted, then outer "opaque signed", then again opaque signed by Dave.

Released TB shows that encrypted, and with a bad signature from Alice. The additional signature isn't mentioned.

The message contents are shown.

With the fix applied:
Message NOT shown, invalid signature.

(4) Similar to 2, but with an additional outer signature layer. Inner Encrypted, then outer "clear-text signed", then again opaque signed by Dave.

Released TB shows that as having a bad signature from Alice. The additional signature isn't mentioned.

Message contents are NOT shown, rather we get an attachment.

With the fix applied:
Message NOT shown, no attachment, invalid signature.

(5) Two signatures, inner opaque signed, outer opaque signed by Dave.

Released TB shows that as a "mismatch signature" from Alice, because Dave is the sender. The additional signature isn't mentioned.

The message contents are shown.

With the fix applied:
Message NOT shown, no attachment, invalid signature.

(6) Two signatures, inner clear-text signed, outer opaque signed by Dave.

Released TB shows that as a "mismatch signature" from Alice, because Dave is the sender. The additional signature isn't mentioned.

The message contents are shown.

With the fix applied:
Message NOT shown, no attachment, invalid signature.

(7) Two signatures, inner opaque signed, outer clear-text signed by Dave.

Released TB shows that as a good signature from Dave.

The message contents are NOT shown, and we get an attachment.

With the fix applied:
Same

(8) Two signatures, inner clear-text signed, outer clear-text signed by Dave.

Released TB shows that as a mismatch signature from Alice, the second signature isn't mentioned.

Message contents are shown,.

With the fix applied:
Message NOT shown, no attachment, invalid signature.

Comment 74

[Kai Engert [:KaiE:]]

To summarize the previous comment:

• Scenario 1 should have been fixed by EFAIL, but wasn't.

• If we detect that more than one signature is present, we'll never show the email contents.

• If we detect that more than one signature is present, we'll usually show an invalid signature status. The exception is scenario 7, if the inner message looks like an encrypted message, which we don't decode, we don't detect that we have two signatures.

Because this fix doesn't require any new UI or new strings, we should consider to backport it to TB 68.x. (My only worry is that we don't have automated tests for the pasts EFAIL scenarios yet. I've filed a bug to get that done eventually.)

(In the future, we could consider to improve the UI. Instead of simply not showing a message, we could inform the user about the reason for not showing the message. I suggest to track that in a separate bug.)

Comment 75

[Kai Engert [:KaiE:]]

Maybe we shouldn't allow scenario 7 either?
I'll attach an incremental patch, which treats scenario 7 the same as the other ones: shown with an invalid signature.

Comment 76

[Kai Engert [:KaiE:]]

Attachment: 1240290-incremental-fix-for-scenario-7.patch

Comment 77

[Kai Engert [:KaiE:]]

Please give me your opinions, if the behavior of the fixes described in comment 73 and comment 74 are good improvements.

Also, please tell me your opinion on comment 75.

Comment 78

[Jorg K (CEST = GMT+2)]

Difficult stuff, especially on a Sunday late at night. That aside, how relevant to all this is the "half-Efail" fix of not decoding any child parts? Pref mailnews.p7m_subparts_external implemented in bug 1464056 (apparently causing crashes) and regressing bug 1524750. Can that pref be removed or flipped around at some stage?

So (1) and (2) are encrypted, then signed in two different ways and (3) and (4) are similar but with additional signatures. Maybe a silly question: since you have two signing mechanisms, there are four cases of adding two signatures, just like you have four cases in (5)-(8). IOW, (3) = (1) + op sig, (4) = 2 + op sig, what about (1) + cl sig and (2) + cl sig? Sorry, you have to educate us all.

So cases (5) to (8) have no encrypted part? They are just four permutation of clear/opaque. I'm surprised that the system handles those signature types so differently. What's the point of not showing the messages? It's definitely being tampered with? Wouldn't it be desirable to show invalid signatures for all four cases?

Comment 79

[Kai Engert [:KaiE:]]

(In reply to Jorg K (GMT+2) from comment #78)

how relevant to all this is the "half-Efail" fix of not decoding any child parts? Pref mailnews.p7m_subparts_external implemented in bug 1464056 (apparently causing crashes) and regressing bug 1524750. Can that pref be removed or flipped around at some stage?

We don't want to flip the pref, that would re-introduce security issues. We want to remove the pref and keep the current default, I've filed bug 1576828 for that.

I've explained why bug 1524750 isn't a bug, but rather the intended behavior. I agree user feedback could be better, see bug 1576655.

You didn't say what crashes you're referring to. Are you talking about bug 1464056 comment 51 from me? If yes, then yes, the patch here will fix potential crashes, by avoiding the early return, and reaching the init code at the end of mime_find_class.

Comment 80

[Kai Engert [:KaiE:]]

(In reply to Jorg K (GMT+2) from comment #78)

So (1) and (2) are encrypted, then signed in two different ways and (3) and (4) are similar but with additional signatures, just like you have four cases in (5)-(8).

Correct.

Maybe a silly question: since you have two signing mechanisms, there are four cases of adding two signatures,

I've added tests for all four combinations of the two signature mechanisms. See attachment 9087994 [details] [diff] [review] and search for "// sign, then sign again". That section has 16 tests (4 sig combinations * 4 hash algorithm variations).

That's scenarios 5, 6, 7, 8 from above.

It's correct, I didn't test all of those with the additional encrypt.
Test cases 1 and 2 should sufficiently test that we don't decrypt the inner encrypted part, if they are wrapped inside a signature of either of the two styles.

It's true that for completeness I could potentially add variations of the encrypt-sign-sign scenarios 3 and 4, which use the multipart sig for the outermost layer, but I didn't consider it necessary.

So cases (5) to (8) have no encrypted part? They are just four permutation of clear/opaque.

right.

I'm surprised that the system handles those signature types so differently.

That's because they work very differently!

For the signature type "multipart/signed", we have multiple mime parts. Outer multipart/signed, with inner "any encoding" plus second inner part "application/pkcs7-signature". MIME handler in mimemcms.cpp. That's because no other content shares that mime type.

The second signature type uses the same mime type as encryption, and it's just a single mime part.

Although they look slightly different:

Content-Type: application/pkcs7-mime; name="smime.p7m"; smime-type=enveloped-data
Content-Description: S/MIME Encrypted Message

vs.

Content-Type: application/pkcs7-mime; name=smime.p7m; smime-type=signed-data
Content-Description: S/MIME Cryptographic Signature

we route both of them through the same MIME decoder type in "mimecms.cpp".

The smime-type might be lying. We can't be certain what's inside, until we are decoding that CMS message blob. Because it could potentially be encrypted content (which we want to prevent from being exposed accidentally), we don't look inside it, we already prevent processing it.

What's the point of not showing the messages?

Arguments:

• We want to fix scenario 1. That means, whenever we see an outer layer that is opaque signed, we don't want to decode the inner parts that could be either signed (inner opaque) or encrypted. Consequently, we'd no longer show the contents for scenario 5 (which is only signatures, no encryption), and also no longer show contents for scenario 3.

• If we fix scenario 1, the only scenarios with a double-signature that could potentially still show contents are 6 and 8. However, for consistency reasons, I'd prefer to NOT show contents for anything that has a double signature.

It's definitely being tampered with?

Unknown.

Wouldn't it be desirable to show invalid signatures for all four cases?

Yes, that's why I'm suggesting. That's why for consistency reasons, I've also worked on the additional incremental fix for scenario 7.
(If we agree we want that, I'll merge the incremental patch, so we only have one patch for code review.)

While looking around, I decided that I should also review function MIMEGetRelativeCryptoNestLevel if it's still a good implementation.

Comment 81

[Jorg K (CEST = GMT+2)]

(In reply to Kai Engert (:kaie:) from comment #79)

You didn't say what crashes you're referring to. Are you talking about bug 1464056 comment 51 from me?

Yes. I'll look at comment #80 later.

Comment 82

[Kai Engert [:KaiE:]]

Justification for the implementation approach I'm using:

Because of the flow of the streaming processing of MIME data, we face the following dilemma:
The outer signature is processed first, and triggers a call to the signedStatus callback.
If the signature is valid, we get a good status in the user interface.

It is difficult to prevent sending the positive signature status, because I don't know of a way to peek inside the inner contents.
It seems easier to allow the inner contents to override the earlier signature status.

Consequently, we'll get multiple notifications. That's why the code is enhanced to never allow overriding of a bad status with a good status. Regardless which status came first, the bad status for a message will always prevail.

That's also why we the "extra" variable for the individual tests, the tests must wait until we get the expected amount of notifications.

Comment 83

[Kai Engert [:KaiE:]]

Comment on attachment 9087995 [details] [diff] [review]
1240290-nss-testsuite-v2.patch (will be reviewed in separate NSS bug)

Moving this patch to NSS bug 1577448.

Comment 84

[Kai Engert [:KaiE:]]

Attachment: 1240290-fix-v3.patch

merging v2 and incremental fix into new v3

Comment 85

[Kai Engert [:KaiE:]]

Attachment: 1240290-fix-v4.patch

merged to trunk (comm-central), merging on top of bug 1571481.

Comment 86

[Kai Engert [:KaiE:]]

Comment on attachment 9089016 [details] [diff] [review]
1240290-fix-v4.patch

I think this is ready for review.

Magnus, would you like to review here in bugzilla?
(Let me know if you prefer to review in phabricator. I haven't yet tested the setup for security sensitive patches with phabricator, but it should work.)

Comment 87

[Magnus Melin [:mkmelin]]

Comment on attachment 9089016 [details] [diff] [review]
1240290-fix-v4.patch

Review of attachment 9089016 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/extensions/smime/content/msgHdrViewSMIMEOverlay.js
@@ +38,5 @@
> +  
> +  /**
> +   * Returns NULL if the current window isn't related to a folder, which
> +   * means that comparison by URI is unnecessary.
> +   */

Please use jsdoc style, so something like
@return the URI of the selected message, or null if the current message displayed isn't in a folder

Maybe it should be named getSelectedMessageURI?

@@ +61,5 @@
>        return;
>      }
> +
> +    let selURI = this.getSelectedURI();
> +    if (selURI != null && selURI != aMsgNeckoURL) {

just do a truthy check 

if (selURI && selURI != aMsgNeckoURL)

... or well. you can just do

if (aMsgNeckoURL != this.getSelectedURI())

@@ +67,4 @@
>        return;
>      }
>  
> +    if (gSignatureStatusForURI != null &&

just if (gSignatureStatusForURI

... but the == check would take care of it too, right?

::: mailnews/mime/src/mimecms.cpp
@@ +176,5 @@
> +  }
> +  return false;
> +}
> +
> +

nit: remove double newline

@@ +468,5 @@
> +    // inner encrypted part could have been produced by an attacker who
> +    // stripped away a part containing the signature (S/MIME doesn't
> +    // have integrity protection).
> +    // A sign-then-sign encoding is confusing, too, because
> +    // be an attempt to confuse the which part would be shown as having a signature?

missing an "it" here

@@ +608,5 @@
> +    return -1;
> +  }
> +
> +  if (!data->skip_content && !data->decoder_context) {
> +    /* If we don't skip, we should have a context. */

would use // style for comments

@@ +646,5 @@
>  
> +  if (data->skip_content) {
> +    /* Skipping content means, we detected a forbidden combination
> +     * of CMS objects, so let's make sure we replace the parent status
> +     * with a bad status. */

here too //

::: mailnews/mime/test/unit/test_smime_decrypt.js
@@ +198,5 @@
> + * - match_content: By default (true), we expect that our processing of
> + *                  the message shows the usual message from Alice
> + *                  to Bob. If false, we expect the message isn't
> + *                  shown.
> + *                  (ignored for bad signatures.)

I. 
Don't need the parenthesis

@@ +372,5 @@
>      // dump("contents: " + contents + "\n");
>  
>      if (!msg.sig || msg.sig_good) {
> +      let expected = "This is a test message from Alice to Bob.";
> +      if (!("match_content" in msg) || msg.match_content ) {

Seems you never set match_content to anything?
And is this if clause correct? match_content is true by default

Comment 88

[Kai Engert [:KaiE:]]

Attachment: 1240290-fix-v5.patch

I've addressed all your comments. I've removed the match_content code, that was leftover from an earlier revision.

Comment 89

[Magnus Melin [:mkmelin]]

Comment on attachment 9089345 [details] [diff] [review]
1240290-fix-v5.patch

Review of attachment 9089345 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, r=mkmelin

::: mailnews/mime/src/mimemcms.cpp
@@ +379,5 @@
>  }
>  
> +static void MimeMultCMS_suppressed_child(void *crypto_closure) {
> +  /* I'm a multipart/signed. If one of my cryptographic child elements
> +   * was suppressed, then I want my signature to be shown as invalid. */

nit: // style please

::: mailnews/mime/src/mimemsg.cpp
@@ +681,5 @@
>      return status;
>    }
>  
> +  /* If this is the outermost message, then now is the time to run the
> +   * post_header_html_fn. */

here too //

::: mailnews/mime/src/mimemsig.cpp
@@ +531,5 @@
>    int status = 0;
>    MimeObject *body;
>  
> +  if (!sig->crypto_closure) {
> +    /* We might have decided to skip processing this part. */

//

@@ +544,5 @@
>    if (obj->options && obj->options->headers != MimeHeadersCitation &&
>        obj->options->write_html_p && obj->options->output_fn &&
>        obj->options->headers != MimeHeadersCitation && sig->crypto_closure) {
> +    /* Calling crypto_generate_html may trigger wanted side effects.
> +     * But we're no longer using its results. */

//

Comment 90

[Kai Engert [:KaiE:]]

Attachment: 1240290-fix-v5-plus-nits.patch

fixed the comments

Comment 91

[Kai Engert [:KaiE:]]

Attachment: 1240290-fix-v6-comm-central.patch (merged context changes)

Same patch as before, just merged to trunk, necessary because of context changes (new prettier).

Comment 92

[Kai Engert [:KaiE:]]

Attachment: 1240290-esr68-test-data.patch

test data for ESR 68 branch

Comment 93

[Kai Engert [:KaiE:]]

Attachment: 1240290-backport-esr68.patch

Patch backported to ESR 68 branch.
Excludes new prettier changes.
Includes the small change from bug 1532573 which fixes the @bogus.com email addresses in the test script to @example.com to be compatible with the latest test data.

Comment 94

[Kai Engert [:KaiE:]]

Attachment: 1240290-esr68-backport-v2.patch

formatting changes on tb 68 branch require this updated backport patch

Comment 95

[Jorg K (CEST = GMT+2)]

Checkin-needed? Or you can land after the next M-C merge, perhaps in about 20 minutes from now.

Comment 96

[Jorg K (CEST = GMT+2)]

BTW, I'm doing TB 70 beta 2 today or tomorrow. You want to ship it there?

Comment 97

[Jorg K (CEST = GMT+2)]

Comment on attachment 9090661 [details] [diff] [review]
1240290-fix-v6-comm-central.patch (merged context changes)

That goes for the data patch, too.

Why is the ESR 68 patch different? c-esr68 should be reformatted now, to the trunk patch should (almost) apply.

Comment 98

[Jorg K (CEST = GMT+2)]

Try looks good so far:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=514cb59d06af6043f6ced7b7ef726a79c977a2ed

Comment 99

[Kai Engert [:KaiE:]]

(In reply to Jorg K (GMT+2) (reduced availability 14-19 of Sept.) from comment #97)

Why is the ESR 68 patch different? c-esr68 should be reformatted now, to the
trunk patch should (almost) apply.

That was explained in comment 93.

Resulting test data is the same, it's just that 68 has an older snapshot, and therefore the patch looks different.

Comment 100

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/e17cea8e7c348566ed1c250396c796a9d18d4a00
https://hg.mozilla.org/comm-central/rev/5f3a5f96cc9b1b4f52f652e46fd433c4c08717ec

I took the test data from the try run since it was different from attachment 9087996 [details] [diff] [review].

I forgot to run linting and clang-format, I hope you have, or else, I'll land a follow-up in a minute.

Comment 101

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/6542dc3163386308ccb3bd4db7e914d2eb402844 - reformat C++ code.

Comment 102

[Jorg K (CEST = GMT+2)]

TB 70 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/07b204800f62b72a0543808c980172bae81b2ef0
https://hg.mozilla.org/releases/comm-beta/rev/d38017afcf62c5833a3c01793a8fed474270edc4
https://hg.mozilla.org/releases/comm-beta/rev/4d287273469c94afe9e33a2f7c8414bbc4c6254e

Comment 103

[Jorg K (CEST = GMT+2)]

Comment on attachment 9093925 [details] [diff] [review]
1240290-esr68-backport-v2.patch

Approved for TB 68.2 (or let's discuss whether 68.1.1 doesn't make more sense since we can get feedback from a smaller group of users on the ESR channel first).

Comment 104

[Kai Engert [:KaiE:]]

I'd prefer to let it bake on Beta for 1-2 weeks, prior to make a decision about shipping in 68.x stable.

Comment 105

[Jorg K (CEST = GMT+2)]

My experience shows that beta users don't use any "sophisticated" or uncommon features. Hence my suggestion was to spin it one week on 70 beta 2 and if good, put it on TB 68.1.1 which is not widely distributed so far. Then have it spin there for 1-2 weeks before we ship 68.2 to 25 million users.

Comment 106

[Kai Engert [:KaiE:]]

Why does 68.1.1 have fewer users than 68.2 ? Because we haven't yet automatically upgraded users from 60.x to 68.x ?

Comment 107

[Jorg K (CEST = GMT+2)]

Yes.

Comment 108

[Kai Engert [:KaiE:]]

Ok. Nevertheless, it would be unfortunate if the first 68.x version that 60.x users upgrade to has broken S/MIME support.

But if we cannot realistically expect that anyone uses Beta builds to test S/MIME with real world scenarios, then all we can do is rely on our tests.

Comment 109

[Jorg K (CEST = GMT+2)]

So does that mean we go for 68.1.1 or 68.2? Surely you've viewed some valid S/MIME messages and they're still displayed OK, right? Plus we have some tests that cover the most common scenarios.

I discussed this with Magnus and he and I are cool to ship this in 68.1.1. Could you do an ESR68 try run with your patches. Please backout
https://hg.mozilla.org/releases/comm--esr68/rev/af3c68874f9b040b1751db49e8e041e7330c4651 (EDIT: destroyed link)
as part of the try since it caused Mochitest (bct) failures.

Comment 110

[Kai Engert [:KaiE:]]

(In reply to Jorg K (GMT+2) from comment #109)

Could you do an ESR68 try run with your patches.

Started:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=54e8f5341997ebb515d951fac91df383c44fe381

Comment 111

[Kai Engert [:KaiE:]]

Once ready, I'll run the 68.x try build as my primary client for testing.

Comment 112

[Jorg K (CEST = GMT+2)]

Good move, that's what I've been doing all along with the 68.x series, starting at its beta release :-)

Comment 113

[Wayne Mery (:wsmwk)]

(In reply to Jorg K (GMT+2) from comment #105)

spin it one week on 70 beta 2 and if good, put it on TB 68.1.1 which is not widely distributed so far. Then have it spin there for 1-2 weeks before we ship 68.2 to 25 million users.

Sounds reasonable. But it remains to be seen whether it would be 68.1.2, 68.2.0 (in 4-5 weeks) or a 68.2.1 where we fully unthrottle updates.

Comment 114

[Kai Engert [:KaiE:]]

I haven't seen issues in the limited amount of test message that I could find. If you're ok with the risk, I'm ok to ship with 68.1.1

Comment 115

[Kai Engert [:KaiE:]]

Comment on attachment 9093925 [details] [diff] [review]
1240290-esr68-backport-v2.patch

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: long standing spoofing issue, and waiting one year for a fix is too long
• User impact if declined: continous exposure to spoofing scenarios
• Fix Landed on Version: 71
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): It's uncertain if we have sufficiently tested all S/MIME message scenarios, there's a risk for a regression in treating S/MIME messages correctly.
• String or UUID changes made by this patch: none

Comment 116

[Jorg K (CEST = GMT+2)]

Comment on attachment 9093925 [details] [diff] [review]
1240290-esr68-backport-v2.patch

Wrong flag. Already approved for comm-esr68.

Comment 117

[Jorg K (CEST = GMT+2)]

Comment on attachment 9090665 [details] [diff] [review]
1240290-esr68-test-data.patch

Same here. We don't need to approve all patches individually. A flag on one of them is enough not to miss the bug. But since I'm here, I'll apply it. There's also a reformatting changeset which I'll apply to ESR68 as well.

Comment 118

[Jorg K (CEST = GMT+2)]

TB 68.1.1 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/137371bf8d261c76b13f409303f1a4e586250e99
https://hg.mozilla.org/releases/comm-esr68/rev/75cae72a8bb7499fefb512189202fa12af485dbe
https://hg.mozilla.org/releases/comm-esr68/rev/41c0a3653f95cf6158a2b49ab7cb19327c50e9e8

Comment 119

[Kai Engert [:KaiE:]]

https://www.mozilla.org/en-US/security/advisories/mfsa2019-32/#CVE-2019-11755