222179 - User preferences should control ciphers used when sending encrypted S/MIME messages

Status: NEW

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

Description

[nischkaa]

User-Agent:       I want NO ads NO scripts ONLY plain HTML
Build Identifier: Mozilla Thunderbird 0.2 (20031011)

The used encryption and signature algorithms (such as "RC2 (128 bit)" or
"RSA/SHA-1") should be choosable when composing a mail, and should be displayed
when reading a mail, at least in the existing "Message Security" dialog.

Reproducible: Always

Steps to Reproduce:
1. Create and import your own X.509 certificate
2. Send an email to yourself, both encrypted and signed
3. Get that email into your inbox and open it

Actual Results:  
1. You could not choose the cipher (e.g. RC2-128 or TripleDES) and signature
algorithm (e.g. RSA/MD5 or RSA/SHA-1) before sending the message. You do not
even know which algorithms were used. They may be too weak for some people, or
not supported by the recipient.

2. There is absolutely NO way to find out which cipher and signature algorithm
protected the message when it was transmitted. Even looking at the headers in
the message source does not help.


Expected Results:  
1. The dialog box "Message Security", invoked either from icon "lock"/"View
Security Info" or menu "View"/"Message Security Info" in the "Compose" window,
should have two drop-down lists for each recipient of the message to select the
algorithms:
   a) "Encryption" with choices "RC2 (40 bit)", "RC2 (64 bit)", "RC2 (128 bit)",
"DES (56 bit)", "Triple-DES (168 bit)"
   b) "Signature" with choices "RSA/MD5" and "RSA/SHA-1"

2. The "security" preferences panel for each email-account should have the same
drop-down lists to choose the default settings for each newly composed mail,
used for recipients for whom only the certificate is known but not the supported
and preferred algorithms.

3. The address book should
   a) display the applicable S/MIME certificates of the person,
   b) store and display the supported and preferred encryption and signature
algorithms,
   c) get/update both a) and b) from each received S/MIME message,
   d) allow selecting the supported and preferred algorithms manually
per email-address (a person can have multiple email addresses)

4. The dialog box "Message Security", invoked from the icon "Pen" or icon "Key"
in the message header bar or menu "View"/"Message Security Info" in the main
window where you read messages, should display which encryption and signature
algorithm were used *for each recipient*.
Personally, I would love to have the only or weakest algorithms used for any
recipient displayed beneath the "Pen" and "Key" icons, and in the message table,
as short strings like "RC2 (128 bit)" and "RSA/SHA-1".

5. Since the dialog box "Message Security" is conservative (saying "Encryption
makes it very difficult for other people to view information while it is
travelling...", it should also say "A digital signature makes it very difficult
for other people to alter information while it is travelling..." (instead of
"The message has not been altered...") and "The subject line is never encrypted
or signed". Maybe the text should indicate how secure the weakest used algorithm
is believed to be, e.g. "RC2 (40 bit) may be broken by a single individual with
a number of workstations within weeks".

The problem also applies to Mozilla 1.4, so I guess it is in the common Mail
component of mozilla.org.

Netscape, on the other hand, allows to choose and display the encryption and
signature algorithms since version 4.x or so.

Comment 1

[Gervase Markham [:gerv]]

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 2

[Gervase Markham [:gerv]]

This bug has been automatically resolved after a period of inactivity (see above
comment). If anyone thinks this is incorrect, they should feel free to reopen it.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

The feature being requested here is an extremely old request.  Requests for 
it began much earlier than the date of this bug.  It was part of bug 31799,
for example.  

Long ago, Netscape and Mozilla email clients had a UI by which a user could
choose ciphers to be used for encrypted email that the user RECEIVED.  When 
the user chose (say) 3DES encryption only, he was setting a preference that 
was thereafter conveyed to other correspondents in any signed S/MIME emails 
he sent.  When his correspondents read his new signed messages, they would 
thereafter send him only strongly-encrypted messages.  

That UI, and the prefs it set, had no effect at all on the ciphers that were 
used when that user's own email client sent out encrypted messages.   They 
only affected the preferences for what that user received, not what he sent.  
This violated the "principle of least astonishment" for most users.  Many 
users were outraged to discover that they were still sending messages 
encrypted with low grade weak encryption after they had specified only 
strong encryption in their prefs.  

That was clearly a failure of UI to explain the prefs.  When Thunderbird was created, the UI for those prefs was completely removed (IINM).  But the users'
desire to be able to control the ciphers they use when sending out encrypted
message has not diminished any.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

This may be a duplicate of bug 136289.

Comment 6

[Ludovic Hirlimann [:Usul]]

(In reply to comment #5)
> This may be a duplicate of bug 136289.

Nelson can you explain why you think it's a "may" and not an "is" ?

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

> can you explain why you think it's a "may" and not an "is" ?
Difference of scope.  This bug is clearly and solely about prefs that affect
the cipher used when sending.  The other bug also includes prefs for ciphers
used when receiving.

Comment 9

[David Cooper]

This bug seems to be very closely related to an issue that I have.  NIST is recommending moving away from the use of SHA-1 to compute digital signatures (http://csrc.nist.gov/publications/PubsDrafts.html#800-131).  Beginning with v3.1, Thunderbird can verify signatures on email messages that were signed using SHA-256, SHA-384, and SHA-512, but it is will only sign messages using SHA-1.  I have been able to modify Thunderbird to sign emails using SHA-256 by making a few simple changes to mailnews/extensions/smime/src/msMsgComposeSecure.cpp.  However, even with these changes, the choice of hash algorithm cannot be changed without recompiling the program from source, which is not a viable option for most users.

Ideally, users would be able to select the signature algorithm to use when signing an email.  My suggestion would be a selection menu that appeared within the "Digital Signing" area of the "Security" tab in the "Account Settings" window.  The menu would be enabled when the user had specified a certificate to digitally sign messages, and the choices in the menu would depend on the subject public key in the certificate.  For example, if the public key were an ECC key on curve P-256, then the only option in the menu would be ECC with SHA-256.  If the public key were an RSA key then the options would be RSA with SHA-1, RSA with SHA-256, RSA with SHA-384, and RSA with SHA-512.  (Later, once NSS supports RSASSA-PSS, the list of options would be expanded to include both RSA PKCS #1 v1.5 and RSASSA-PSS.)  While this would be a lot of options to choose from, one of the options could be labeled as "Recommended" and be chosen by default.  I might be able to help out some with the implementation of this, but my knowledge of the source code is very limited, and I have no idea how to do the programming to create the user interface.

If modifying the user interface to permit the user to select a signature algorithm is not considered a viable option, then an alternative could be to use the signer's certificate as the basis for choosing an algorithm.  So, if the certificate is signed using SHA-256, then the email will be signed using SHA-256.  If the certificate is signed using RSASSA-PSS, the the email will be signed using RSASSA-PSS (once RSASSA-PSS is supported by NSS).

Comment 10

[David Cooper]

Attachment: Patch to allow user to specify choice of hash algorithm when digitally signing email

I have created a patch to address one aspect of this bug report.  The attached patch adds a drop-down menu to the "Security" preferences panel of Account Settings to allow the user to specify which hash algorithm should be used when digitally signing an email.  The choices in the drop-down menu are "default", "SHA1", "SHA256", "SHA384", and "SHA512", with "default" being the default choice.  If the user chooses an option other than "default" then the message is signed using the hash algorithm specified by the user.  If the user chooses the "default" option, then Thunderbird chooses which hash algorithm to use.

When the "default" option is chosen, the code in this patch chooses a hash algorithm based on the subject public key in the certificate being used to sign the email.  If the subject public key is an RSA or DSA key, then SHA-1 is used.  If the subject public key is an elliptic curve key, then the hash algorithm is chosen based on the size of the subject public key.

One aspect to this patch that I would like to point out is that if the user chooses a hash algorithm other than "default" then the signing function will always be called with that hash function specified, even if that hash function is not appropriate.  For example, for DSA keys, NSS currently only supports DSA with SHA-1, so if any other hash algorithm is specified the attempt to sign the message will fail.  Similarly, with elliptic curve keys, the attempt to sign the message will fail is the output of the hash is larger than the key (e.g., for a public key over curve P-256, signature generation will fail if SHA-384 or SHA-512 is chosen).  If an unacceptable hash function is chosen, the result of signature generation failing will be a dialog window that says:

    Unable to sign message. Please check that the certificates
    specified in Mail & Newsgroups Account Settings for this
    mail account are valid and trusted

In most cases this will not be a problem since (1) any of the hash algorithms will work with an RSA key and (2) most users will likely not change the choice of hash algorithm from "default", in which case Thunderbird will always choose an appropriate hash function.

I had originally wanted to customize the drop-down menu based on the user's certificate, but I discovered while working on that patch that in general only the certificate's nickname may be known and not the contents of the certificate itself.

One way to avoid the signature generation problem would be to override the user's hash algorithm selection if the hash algorithm is inconsistent with the user's key (e.g., always use SHA-1 with a DSA key even if the user specifies SHA-256, SHA-384, or SHA-512), but I didn't know if it would be appropriate to silently override a user's preference even if the user's preference would not work.

I would appreciate any comments on how this patch could be improved and I hope that it will be considered for inclusion in Thunderbird.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 535647 [details] [diff] [review]
Patch to allow user to specify choice of hash algorithm when digitally signing email

The one thing I'm not really fond of is the word "default", since it's confusing to me, as I don't know what the default is, nor how it differs from any of the SHA algorithms.

So, if you can remove that and just make one of the SHA algorithms the default, then ui-r=me.  (Otherwise, explain how the default differs, and we can try to come up with a better word for it.)

Thanks,
Blake.

Comment 12

[David Cooper]

(In reply to comment #11)

Blake,

Thanks for reviewing this patch.

When the "default" option is chosen, the hash algorithm is selected by getHashFunction() in nsMsgComposeSecure.cpp.  At the moment, this function chooses SHA-1 if the user has an RSA or DSA public key, but if the user has an ECDSA public key then if chooses a hash function based on the length of the user's key.  So, for a 256-bit elliptic curve key it returns SHA-256 and for a 384-bit elliptic curve key it returns SHA-384.

I could just make SHA-1 the default, but I see a few problems with this.  I'd really like to see the use of SHA-1 for digital signatures phased out over the next few years.  Right now, I have designed the patch so that if someone has an RSA key, SHA-1 is used as the hash algorithm unless the user specifically chooses a different hash algorithm.  Within the next few years, I would like to see this changed so that SHA-256 is used unless the user specifically chooses a different hash algorithm.  However, if SHA-1 is made the default, then SHA-1 would appear in users' configurations as their chosen hash algorithm even if they never actually choose SHA-1 but simply left the configuration unchanged from its default value.  Also, if the default were simply set to SHA-256 this wouldn't work for user's with DSA public keys or with elliptic curve public keys shorter than 256 bits.  My solution to this was to leave Thunderbird free to choose the most appropriate hash algorithm in cases where the user has not specifically made a choice of hash algorithm.  Do you think there is a way to accomplish this that would be less confusing than "default"?  Something like replacing the word "default" with a blank line (i.e., empty string) or a longer phrase such as "Allow Thunderbird to choose".

Comment 13

[Konstantin Andreev]

David, I am maintaining a private TB build, which also manages hash algorithms for s/mime (but w/o GUI). You are going in the right direction, but I see an unpleasant issues with your approach.

1) A specific hash algorithm in "account settings" (hereinafter: UHASH) is tightly coupled with sign key.  UHASH, relatively to sign key, may be:

    incompatible : old DSA => SHA-1 only, GOST 34.10 => GOST 34.11 only, etc.

    suboptimal: ECC 256bit => 256-bit hash is optimal

As a matter of fact, UHASH becomes a property of sign key, and must be changed together with sign key.

This coupling may become very annoying, if a user has multiple sign keys with different sign algorithms (e.g. RSA key for USA correspondents and GOST key for Russian ones).

2) "default" setting is ambiguous from the user POV. Will the weakest hash be chosen ? Or an "optimal" (in some sense) one ?

IMO, what you are really trying to achieve, is the ability for user to state:

    - "Please, sign with strong hash, always/if possible."

Given that, let me propose alternative approach for specifying sign hash. Here is a "text drawing" of UHASH controls on the "Account settings/Security" tab.

|
| "Sign hash, bits" >= {256}. [X] mandatory
|

{} - drop-down list (or even text box), [] - checkbox

MUA tries to choose a hash not weaker than 256 bit. If it can't, and "mandatory" is not checked, MUA chooses biggest supported strength (e.g. sha-1 for old DSA). If MUA can't choose strong hash and "mandatory" is checked, sign operation fails.

Please, note: 

  - exact hash type (SHA,GOST,etc.) is not indicated, it is implied by sign key.

  - strength range lets to cover sign keys with different hash capabilities.

  - if "mandatory" is checked, you have a guarantee of strong hash

  - if "mandatory" is not checked, you get strong hash if sign key supports it, but may use sign keys which support weaker hashes.

Thus sign key gets decoupled from UHASH.

Comment 14

[David Cooper]

(In reply to comment #13)

> IMO, what you are really trying to achieve, is the ability for user to state:
> 
>     - "Please, sign with strong hash, always/if possible."

What I am really trying to achieve at the moment is the ability to sign emails using RSA with SHA-256.  It would be infeasible at the moment, however, to change Thunderbird to always use SHA-256 with RSA since Microsoft Outlook on Windows XP (and Thunderbird prior to version 3.1) cannot process emails signed using SHA-256.  So, the only way I see to achieve my goal is to allow the user to sign the hash algorithm.  In solving the short-term problem, though, I hope to find a solution that will work in the long-term as well.

I don't believe that there is any perfect solution.  I tried to address problem (1) by having a "default" (or "Allow Thunderbird to choose") option.  I also thought this would help the vast majority of users who don't even know what a hash algorithm is since it makes clear that they can just ignore the configure setting (i.e., leave it at "default" or "Allow Thunderbird to choose") if they don't understand what it means.  It also makes it easier to migrate users who simply leave the configuration setting unchanged.  In a few years, when it is deemed to be no longer necessary to support email recipients who cannot process SHA-256, getHashFunction() can be changed to return SHA-256 for RSA rather than SHA-1, and those who haven't specifically chosen a hash algorithm will be migrated to using SHA-256.

At the moment, it would be sufficient to only allow the user to choose the hash algorithm if signature algorithm is RSA [e.g., change "Signature Hash Algorithm:" to "Signature Hash Algorithm (for RSA only):"].  Then Thunderbird would use the chosen hash algorithm for RSA, but it would always use SHA-1 for DSA and for ECDSA it would choose a hash algorithm based on the key length.  Even if NSS added support for DSA with hash algorithms other than SHA-1, Thunderbird could choose the correct hash algorithm based on the size of the key.  According to RFC 4491 this would continue to work even if support for GOST were added since there is only one hash function defined for use with GOST.

In the future, however, it would not be sufficient to only allow the user to choose the hash algorithm for RSA signatures.  If NSS adds support for SHA-3 in a few years, then it would be appropriate to allow a user with a 256-bit ECC key to choose between SHA-256 (from SHA-2) and the 256-bit hash from SHA-3.  The problem is that if the user is allowed to choose a "hash family" (e.g., GOST, SHA-2, or SHA-3) there is a risk that the user will choose one that is incompatible with the key type.  But, I don't know how to allow the user to choose the hash algorithm without creating a risk that the user will choose a hash algorithm that is incompatible with the signature algorithm (except in the short-term by only allowing the user to choose a hash algorithm if the signature algorithm is RSA).

I think that your proposal has merit, but it is unclear how it would be extended once NSS added support for SHA-3 and it became necessary to allow the user to choose between SHA-2 and SHA-3.  I am also unclear of the meaning of the drop-down list.  If the user had an RSA key and 160 was the chosen value, could Thunderbird sign using SHA-256 rather than SHA-1?  What if the user really wanted to choose SHA-1 since the recipient's client didn't support SHA-256?

Comment 15

[Konstantin Andreev]

(In reply to comment #14)
> What I am really trying to achieve at the moment is the ability to sign
> emails using RSA with SHA-256.

I understand your point. But tuning sign hash "for RSA only" is not fair. At least ECC signatures work great in TB/NSS, and ought to be addressed.

So, we should either gradually augment the list of hash selectors (by algorithm):

| Sign hash,
|   RSA: {SHA256}
|   ECC: {SHA224}
|   ...

or establish something more universal / long-term.

Ideally, hash selector should not mention any particular algorithm. This way PSM GUI and even code could avoid continious adding new algorithm names.

> it is unclear how [your proposal] would be extended [...] to allow the user to
> choose between SHA-2 and SHA-3.

That's right, my proposal becomes ambiguous when SHA-3 goes in use. I thought about this. This may be addressed by an ordered list of "check-boxed" of "hash families", e.g.:

| [x] sha-3   ^
| [ ] sha-2   |
| [x] gost    v

This is a typical appoach for algorithm selection, you may have seen it in many GUI SSH clients.

> I am also unclear [...] If the user had an RSA key and 160 was the chosen
> value, could Thunderbird sign using SHA-256 rather than SHA-1?

No, TB must sign with SHA-1. Sorry, this wasn't fully explained. The rule

|
| "Sign hash, bits" >= {N}
|

is unambiguous only if TB chooses the weakest hash, satisfying ">= N" condition. If we want for TB to be able to choose stronger hashes, there should be an upper boundary "<= M". That'is, IMO, a complication of UI without benefits.

> MS Outlook on Win XP (and TB < 3.1) can't process emails signed using SHA-256.

There always will be exceptions. If recipient provided us with S/MIME caps, hash selection may take them into account. An alternative (or supplement) is per-recipient tuning: an overriding hash selector on hypothetical Security tab of AddressBook/Contact, near hypothetical r/o "S/MIME caps" viewing tab :)

Comment 16

[David Cooper]

Attachment: Patch to allow user to specify preferred hash algorithm when digitally signing email

Comment 17

[David Cooper]

Attachment 542157 [details] [diff] is a revised version of the patch in attachment 535647 [details] [diff] [review].  In this version of the patch, the user may specify a preferred signature hash algorithm.  The preferred hash algorithm will be used to sign the message if possible.  However, unlike in the previous version of the patch, if NSS cannot use the user's preferred hash algorithm then a different hash algorithm will be used.  So, for example, if the user hash a DSA key pair, then messages will always be signed using SHA-1 regardless of the user's preferred hash algorithm.  For ECDSA, if the user specifies a preferred hash algorithm whose output is longer than the length of the key then the hash algorithm with the longest output that is no longer than the length of the key will be chosen.

As before, I included an option for the majority of users, who have no interest in select which hash algorithm will be used to sign messages:

    Preferred Signature Hash Algorithm: {No Preference}

"Preferred Signature Hash Algorithm" will default to "No Preference" and if "No Preference" is the value then Thunderbird will choose the most appropriate hash algorithm given the user's signature key.  I hope that "No Preference" will be considered less confusing than the "default" option from the previous patch.

So, the new proposed user interface is a drop-down menu as follows:


    Preferred Signature Hash Algorithm: {No Preference}
                                        {SHA1}
                                        {SHA256}
                                        {SHA384}
                                        {SHA512}

In this version of the patch, I also cleaned up the implementation of getHashFunction (now nsCMSMessage_getSigningHashFunction) by moving it to mozilla/security/manager/ssl/src/nsCMS.cpp and making use of the SECKEY_* functions to obtain the type and length of the user's public key.

Comment 18

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 542157 [details] [diff] [review]
Patch to allow user to specify preferred hash algorithm when digitally signing email

It didn't apply, but based on manual inspection of the code ui-r=me, with the reservation that I suspect most users won't really care about this.  On the other hand, it's kind of buried in the preferences, so most users probably won't see it either…  ;)

Comment 19

[Ian Neal]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #18)
> Comment on attachment 542157 [details] [diff] [review]
> Patch to allow user to specify preferred hash algorithm when digitally
> signing email
> 
> It didn't apply, but based on manual inspection of the code ui-r=me, with
> the reservation that I suspect most users won't really care about this.  On
> the other hand, it's kind of buried in the preferences, so most users
> probably won't see it either…  ;)

Don't forget to add the equivalent entities for SeaMonkey too:
http://mxr.mozilla.org/comm-central/source/suite/locales/en-US/chrome/mailnews/smime/am-smime.dtd

Comment 20

[David Cooper]

Attachment: Patch to allow user to specify preferred hash algorithm when digitall signing email

(In reply to Ian Neal from comment #19)
> (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #18)
> > Comment on attachment 542157 [details] [diff] [review]
> > Patch to allow user to specify preferred hash algorithm when digitally
> > signing email
> > 
> > It didn't apply, but based on manual inspection of the code ui-r=me, with
> > the reservation that I suspect most users won't really care about this.  On
> > the other hand, it's kind of buried in the preferences, so most users
> > probably won't see it either…  ;)
> 
> Don't forget to add the equivalent entities for SeaMonkey too:
> http://mxr.mozilla.org/comm-central/source/suite/locales/en-US/chrome/
> mailnews/smime/am-smime.dtd

I updated the patch so that it works cleanly with the current (comm-central) code, adding the string information to am-smime-dtd for SeaMonkey, and tested the code with both Thunderbird and SeaMonkey.  (I hope that I set the review flags correctly.)

Comment 21

[Ludovic Hirlimann [:Usul]]

(In reply to David Cooper from comment #20)

> tested the code with both Thunderbird and SeaMonkey.  (I hope that I set the
> review flags correctly.)

Looks like it. If you want faster UI review posting a screenshot might help.

Comment 22

[David Cooper]

Attachment: Screenshot of hash algorithm selection drop-down menu

Comment 23

[Kai Engert [:KaiE:]]

Comment on attachment 602083 [details] [diff] [review]
Patch to allow user to specify preferred hash algorithm when digitall signing email

sorry, I don't have time to focus on this.
And you probably should have automated tests for new features.
Please try to find another reviewer.

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 602083 [details] [diff] [review]
Patch to allow user to specify preferred hash algorithm when digitall signing email

Dropping r since the patch no longer applies and it is then hard to review.  Please provide an updated patch that applies to current comm-central and create it with Mercurial (headers are not correct in this patch to show in splinter).

Few notes:
- nsICMSMessage needs change to its IID
- the new argument should IMO be added as a last arg for better compatibility

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 602083 [details] [diff] [review]
Patch to allow user to specify preferred hash algorithm when digitall signing email

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

::: ./mailnews/extensions/smime/content/am-smime.js
@@ +97,5 @@
>  
>      gEncryptAlways.setAttribute("disabled", true);
>      gNeverEncrypt.setAttribute("disabled", true);
>      gSignMessages.setAttribute("disabled", true);
> +    gPreferredSigningHashAlgorithm.value = "NoPreference";

No preference is usually just ""

Comment 26

[George Maschke]

Thunderbird automatically chooses the 168-bit 3DES cipher for S/MIME encryption, with no user option to select stronger ciphers that Thunderbird nonetheless can support, like 256-bit AES. This is a security shortcoming that really needs to be promptly addressed in view of recent revelations about government eavesdropping efforts and capabilities. Users should be able to choose the strongest ciphers available. In fact, perhaps 256-bit AES should be made the default in Thunderbird, with the option to choose alternative (and possibly weaker) ciphers.

Comment 27

[David Cooper]

Attachment: Part 1 of patch to allow user to specify preferred hash algorithm when digitally signing email

I updated the patch to allow the user to select a preferred hash algorithm to work with the currernt comm-central and tried to address the comments that were raised, although I haven't yet learned how to create automated tests for this.

Kai: If you don't have time to review this patch, can you suggest someone who could?

As an alternative to this patch, given the general move away from SHA-1 (see, for example, bug 942515), would a patch with no user interface that selected the hash algorithm based on the key size be of more interest? The idea would be that the patch would use the nsCMSMessage_getSigningHashFunction function in this proposed patch to select the hash algorithm, but there would be no user interface or "preferred" hash algorithm and the hash algorithm selected for an RSA public key would be SHA-256 unless the public key was 1024 bits or less in length.

Comment 28

[David Cooper]

Attachment: Part 2 of patch to allow user to specify preferred hash algorithm when digitally signing email

When I used Mercurial in the comm-central directory to create the patch it did not include the changes to code in the comm-central/mozilla directory, so the patch is broken into two separate files. Part 1 includes the patch for comm-central/mozilla and Part 2 includes the patch for comm-central (which includes modifications to files in comm-central/mail, comm-central/mailnews, and comm-central/suite).

Comment 29

[David Cooper]

Attachment: Screenshot of hash algorithm selection drop-down menu

As requested I changed to "No Preference" option to "", so this is a revised screenshot showing the drop-down menu with a blank line indicating no preference.

Comment 30

[Ludovic Hirlimann [:Usul]]

(In reply to David Cooper from comment #28)
> Created attachment 8361735 [details] [diff] [review]
> Part 2 of patch to allow user to specify preferred hash algorithm when
> digitally signing email
> 
> When I used Mercurial in the comm-central directory to create the patch it
> did not include the changes to code in the comm-central/mozilla directory,
> so the patch is broken into two separate files. Part 1 includes the patch
> for comm-central/mozilla and Part 2 includes the patch for comm-central
> (which includes modifications to files in comm-central/mail,
> comm-central/mailnews, and comm-central/suite).

comm-central/mozilla is indeed mozilla-central as psm is shared code. 

you should ask review to :bsmith and he'll probably ask for a mozilla-central patch

Comment 31

[David Cooper]

Attachment: Part 1 of patch to allow user to specify preferred hash algorithm when digitally signing email

No change. Just updating so patch works with current development code.

Comment 32

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8427859 [details] [diff] [review]
Part 1 of patch to allow user to specify preferred hash algorithm when digitally signing email

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

Requesting review from a PSM peer.

Comment 33

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8427859 [details] [diff] [review]
Part 1 of patch to allow user to specify preferred hash algorithm when digitally signing email

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

Looks mostly good if the unnecessary function is removed. I would like to see an updated patch, however, so r- for now.

::: security/manager/ssl/public/nsICMSMessage.idl
@@ +29,5 @@
>    void getEncryptionCert(out nsIX509Cert ecert);
>    void verifySignature();
>    void verifyDetachedSignature(in UnsignedCharPtr aDigestData, in unsigned long aDigestDataLen);
>    void CreateEncrypted(in nsIArray aRecipientCerts);
> +  void CreateSigned(in nsIX509Cert scert, in nsIX509Cert ecert, in UnsignedCharPtr aDigestData, in unsigned long aDigestDataLen, in int16_t aDigestType);

Add a comment that aDigestType is one of the values in nsICryptoHash

::: security/manager/ssl/src/nsCMS.cpp
@@ +16,5 @@
>  #include "nsIArray.h"
>  #include "nsArrayUtils.h"
>  #include "nsCertVerificationThread.h"
> +#include "nsICryptoHash.h"
> +#include "keyhi.h"

nit: arrange #includes according to the guidelines in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices

@@ +577,5 @@
>    return rv;
>  }
>  
> +/* Select a hash algorithm to sign message based on subject public key type and size. */
> +NS_IMETHODIMP nsCMSMessage_getSigningHashFunction(PRInt16 *mHashType, nsIX509Cert *aSigningCert,

This isn't implementing an interface function, so NS_IMETHODIMP shouldn't be used. It returns nsresult. Also, the return value should be on a separate line. Additionally, it should be static so it won't be visible to other files. Furthermore, prepending nsCMSMessage_ to the function name doesn't make much sense (this is C++, not C). If necessary, put it in the nsCMSMessage class.

Although, this function isn't called altogether, so I'm not sure why it's here. Is this left over from a previous patch?

@@ +583,5 @@
>  {
> +  CERTCertificate *scert = nullptr;
> +  nsCOMPtr<nsIX509Cert2> aSigningCert2 = do_QueryInterface(aSigningCert);
> +  SECKEYPublicKey *scert_subjectPublicKey;
> +  KeyType subjectPublicKey_type;

nit: declare variables as close as possible to where they're used

@@ +587,5 @@
> +  KeyType subjectPublicKey_type;
> +  unsigned siglen, keylen;
> +  bool preferred_hash_alg_selected;
> +  PRInt16 preferred_hash_alg;
> +  

nit: remove all trailing whitespace

@@ +667,5 @@
> +  }
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP nsCMSMessage::CreateSigned(nsIX509Cert* aSigningCert, nsIX509Cert* aEncryptCert, unsigned char* aDigestData, uint32_t aDigestDataLen, int16_t aDigestType)

nit: keep lines < 80 chars, return value (i.e. NS_IMETHODIMP) on previous line by itself

@@ +669,5 @@
> +}
> +
> +NS_IMETHODIMP nsCMSMessage::CreateSigned(nsIX509Cert* aSigningCert, nsIX509Cert* aEncryptCert, unsigned char* aDigestData, uint32_t aDigestDataLen, int16_t aDigestType)
> +{
> +  SECOidTag digest_type;

nit: digestType
Also, declare this closer to where it's used.

@@ +698,5 @@
>        ecert = aEncryptCert2->GetCert();
>      }
>    }
>  
> +  if (aDigestType == nsICryptoHash::SHA1)

nit: curly braces around conditional bodies:
if (...) {
  ...
} else if (...) {
  ...
} else {
  ...
}

(this could also be a switch statement)

@@ +745,5 @@
>  
>    /* 
>     * create & attach signer information
>     */
> +  if ((signerinfo = NSS_CMSSignerInfo_Create(m_cmsMsg, scert.get(), digest_type)) 

nit: remove trailing whitespace while you're here

Comment 34

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8427859 [details] [diff] [review]
Part 1 of patch to allow user to specify preferred hash algorithm when digitally signing email

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

Some thoughts about this bug overall:

S/MIME already defines a mechanism by which every user can indicate what signature algorithms he/she prefers and/or supports: http://tools.ietf.org/html/rfc5751#section-2.5.2.

1. Is Thunderbird correctly advertising its SHA-2 support in the S/MIME capabilities extension of the S/MIME email it sends? If not, we should fix that in a separate bug.

2. Thunderbird should cache the S/MIME capabilities in its address book entries and the S/MIME capabilities should override the default value indicated by the preference being added here.

3. Thunderbird, and not Gecko, should interpret the preference. More generally, Thunderbird, and not Gecko, should decide which algorithm to use. (Counterpoint: We should move nsICMS* from Gecko to Thunderbird so it's a moot point.)

4. It's up to the Thunderbird team to decide whether they want to optimize the default value for legacy compatibility or security or compliance with regulations. My own personal opinion is that we should investigate which clients are sending useful S/MIME capabilities; if most other SHA-2-capable clients are sending S/MIME capabilities indicating SHA-2 support then we could leave the default as SHA-1 but override that when we know the S/MIME capabilities of the receivers. And/or we could assume that a SHA-2-signed certificate implies SHA-2 CMS support (even though we know that isn't 100% true). If there are a lot of clients that aren't sending S/MIME capabilities then the decision is tougher.

5. I suggest that we put the Gecko changes (the changes to nsCMS.cpp and nsICMSMessage) in a separate bug from the Thunderbird changes.

::: security/manager/ssl/src/nsCMS.cpp
@@ +576,5 @@
>  
>    return rv;
>  }
>  
> +/* Select a hash algorithm to sign message based on subject public key type and size. */

Please don't use C-style comments in C++ code.

@@ +577,5 @@
>    return rv;
>  }
>  
> +/* Select a hash algorithm to sign message based on subject public key type and size. */
> +NS_IMETHODIMP nsCMSMessage_getSigningHashFunction(PRInt16 *mHashType, nsIX509Cert *aSigningCert,

IMO, this function is better put in the Thunderbird S/MIME code instead of here. The platform code should just require that the caller always supply a hash algorithm.

@@ +578,5 @@
>  }
>  
> +/* Select a hash algorithm to sign message based on subject public key type and size. */
> +NS_IMETHODIMP nsCMSMessage_getSigningHashFunction(PRInt16 *mHashType, nsIX509Cert *aSigningCert,
> +                nsString &preferred_hash_alg_str)

Please mark out parameters as with /*out*/ and if preferred_hash_alg_str is not modified, please make it a const reference.

Please use the camelCase naming convention used in Gecko instead of the under_score naming convention you are using here.

@@ +614,5 @@
> +  if (!scert) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  scert_subjectPublicKey = CERT_ExtractPublicKey(scert);

Please use the ScopedSECKEYPublicKey type and MapSECStatus from ScopedNSSTypes.h as follows:

ScopedSECKEYPublicKey scertPublicKey(CERT_ExtractPublicKey(scert));
if (!scertPublicKey) {
  return MapSECStatus(SECFailure);
}

ScopedSECKeyPublicKey will automatically destroy the public key when it goes out of scope. MapSECStatus will automatically map the NSS/NSPR PRErrorCode to an nsresult that is more specific than NS_ERROR_FAILURE.

@@ +630,5 @@
> +      (subjectPublicKey_type == rsaPssKey)) {
> +    keylen = 8 * siglen;
> +    /* SHA-1 chosen for compatibility with older systems.
> +     * Default for RSA should be changed to SHA-256. */
> +    if (preferred_hash_alg_selected == false) {

Please do not use "==" or "!=" for boolean comparisons to constants "true" or "false" or for pointer comparisons to nullptr.

Bad: if (x == nullptr)
Good: if (!x)

Bad: if (x == false)
Good: if (!x)

Bad: if (x != nullptr)
Good: if (x)

Bad: if (x == true)
Good: if (x)

@@ +745,5 @@
>  
>    /* 
>     * create & attach signer information
>     */
> +  if ((signerinfo = NSS_CMSSignerInfo_Create(m_cmsMsg, scert.get(), digest_type)) 

Please do not use the assignment operator in conditions. Instead, please rewrite to:

signerinfo = NSS_CMSSignerInfo_Create(...);
if (!signerinfo) {
   ...
}

@@ +808,5 @@
>  
>      digest.data = aDigestData;
>      digest.len = aDigestDataLen;
>  
> +    if (NSS_CMSSignedData_SetDigestValue(sigd, digest_type, &digest)) {

Please use camelCase: digestType, not digest_type.

For SECStatus, please *don't* use implicit conversion from SECStatus to boolean in comparisons. Instead, always write explicitly "!= SECSuccess". (Note that "== SECFailure" and "!= SECSuccess" are not the same thing since SECStatus also has the usually-not-used value "SECWouldBlock"; this is one of the reasons we prefer explicit conversion to SECSuccess.)

Comment 35

[David Cooper]

Attachment: Part 1 of patch to allow user to specify preferred hash algorithm when digitally signing email

David, Brian,

Thanks for reviewing this patch. I took the code for selecting the hash algorithm to use to sign the email and moved it into the other patch, so that function is now in a more appropriate part of the code base.

I tried to address the remaining nits that the two of you raised, however, I wanted to avoid making changes to the code that I did not modify. I understand that the developer guidelines specify a preferred ordering for #includes, but it didn't seem that any of the files in security/manager/ssl/src were following that ordering or any scheme that I could make out. As I am just adding one #include to the 13 #includes that are already in nsCMS.cpp, it didn't seem appropriate to try to reorder all of the existing #includes, and there didn't seem to be a more appropriate place to add the new one than at the end of the list.

There were also a couple of nits related to the calls to NSS_CMSSignerInfo_Create and NSS_CMSSignedData_SetDigestValue in  nsCMSMessage::CreateSigned. However, those issued were related to the current code, not to the changes that I made, so I did not address those issues.

Comment 36

[David Cooper]

Attachment: Part 2 of patch to allow user to specify preferred hash algorithm when digitally signing email

Moved function to select the hash function to use to sign the email from mozilla/security/manager/ssl/src/nsCMS.cpp to mailnews/extensions/smime/src/nsMsgComposeSecure.cpp and tried to address nits that were raised for that function.

It may be preferable at this point to just have a patch that moves to signing hash algorithm from SHA-1 to SHA-2 without allowing for any user input. That would make this patch much shorter, since most of the code is related to the user interface and to processing the user's selection of the preferred hash. I could imagine the code would be similar to what is in getSigningHashFunction in this patch, but with a hash being selected being as follows based on key type and size:

    RSA or DSA (less than 2048 bits): SHA-1
    RSA or (2048 bits or more): SHA-256

    ECDSA (less than 256 bits): SHA-1
    ECDSA (256 to 383 bits): SHA-256
    ECDSA (384 to 511 bits): SHA-384
    ECDSA (more than 512 bits): SHA-512

Of course, if this is the preferred approach then a new bug should be opened, since this bug is about allowing the user to specify preferences.

If this is someone who would be willing to review either this or the simplied (no user preference) version, please let me know.

Thanks,

Dave

Comment 37

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8429579 [details] [diff] [review]
Part 1 of patch to allow user to specify preferred hash algorithm when digitally signing email

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

::: security/manager/ssl/src/nsCMS.cpp
@@ +608,5 @@
>      }
>    }
>  
> +  SECOidTag digestType;
> +  if (aDigestType == nsICryptoHash::SHA1) {

nit: Please use switch (aDigestType) instead of if...else if...else.

@@ +617,5 @@
> +    digestType = SEC_OID_SHA384;
> +  } else if (aDigestType == nsICryptoHash::SHA512) {
> +    digestType = SEC_OID_SHA512;
> +  } else {
> +    return NS_ERROR_FAILURE;

return NS_ERROR_INVALID_ARG;

Comment 38

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8429579 [details] [diff] [review]
Part 1 of patch to allow user to specify preferred hash algorithm when digitally signing email

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

Looks good to me with nits addressed.

::: security/manager/ssl/public/nsICMSMessage.idl
@@ +31,5 @@
>    void verifyDetachedSignature(in UnsignedCharPtr aDigestData, in unsigned long aDigestDataLen);
>    void CreateEncrypted(in nsIArray aRecipientCerts);
> +
> +  /* The parameter aDigestType must be one of the values in nsICryptoHash */
> +  void CreateSigned(in nsIX509Cert scert, in nsIX509Cert ecert, in UnsignedCharPtr aDigestData, in unsigned long aDigestDataLen, in int16_t aDigestType);

nit: wrap lines longer than 80 chars

::: security/manager/ssl/src/nsCMS.cpp
@@ +15,5 @@
>  #include "nsICMSMessageErrors.h"
>  #include "nsIArray.h"
>  #include "nsArrayUtils.h"
>  #include "nsCertVerificationThread.h"
> +#include "nsICryptoHash.h"

Here's what the guidelines for #includes are:

Includes are split into three blocks and are sorted alphabetically in each block:

    The main header: Foo.h in Foo.cpp
    Standard library includes: #include <map>
    Mozilla includes: #include "mozilla/dom/Element.h"

In this file, the first point is already taken care of ("nsCMS.h" is by itself). There aren't any of the second type, so we don't have to worry about that. The third is where the issue is. It's true that many of the files in security/manager don't follow these guidelines, but if we address them as we touch those files, we can make progress towards a unified style that will make the code easier to maintain and develop.

@@ +577,5 @@
>  }
>  
> +NS_IMETHODIMP
> +nsCMSMessage::CreateSigned(nsIX509Cert* aSigningCert, nsIX509Cert* aEncryptCert,
> +       unsigned char* aDigestData, uint32_t aDigestDataLen, int16_t aDigestType)

nit: align this line with the first argument on the previous line (you'll probably have to wrap to another line, which is fine - align that one to the same)

Comment 39

[David Cooper]

Attachment: Part 1 of patch to allow user to specify preferred hash algorithm when digitally signing email

David, Brian,

Thanks for the review. This should address all of the issues that you raised in the previous reviews.

Thanks,

Dave

Comment 40

[David Cooper]

For those who may be interested, I just submitted a new bug report (bug 1018259) with a patch that provides an alternative to Part 2 of the patch here. The patch in bug 1018259 automatically selects the most appropriate hash function based on the size and type of the key without allowing the user to specify a preference.

Comment 41

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8431595 [details] [diff] [review]
Part 1 of patch to allow user to specify preferred hash algorithm when digitally signing email

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

Great - r=me with nit addressed.

::: security/manager/ssl/src/nsCMS.cpp
@@ +609,5 @@
>    }
>  
> +  SECOidTag digestType;
> +  switch (aDigestType) {
> +  case nsICryptoHash::SHA1:

nit: indent the entire body of the switch block two spaces:

switch (aDigestType) {
  case nsICryptoHash::SHA1:
    digestType = SEC_OID_SHA1;
    break;
  case nsICryptoHash::SHA256:
...

Comment 42

[David Cooper]

Attachment: Part 1 of patch to allow user to specify preferred hash algorithm when digitally signing email

Adjusted indentation of switch block.

Comment 43

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8431818 [details] [diff] [review]
Part 1 of patch to allow user to specify preferred hash algorithm when digitally signing email

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

Looks good to me.

Comment 44

[Joshua Cranmer [:jcranmer]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #34)
> S/MIME already defines a mechanism by which every user can indicate what
> signature algorithms he/she prefers and/or supports:
> http://tools.ietf.org/html/rfc5751#section-2.5.2.

[I know very little about the crypto side of things. My knowledge of S/MIME basically stops at "give me a block box for CMS and certificate validation, and I can go from there"]

> 1. Is Thunderbird correctly advertising its SHA-2 support in the S/MIME
> capabilities extension of the S/MIME email it sends? If not, we should fix
> that in a separate bug.

Judging from asn1dump and grepping through an mbox archive of recent Kitten wg messages, it appears that the CMS code as of 24 only outputs the symmetric ciphers in the smimeCapabilities dump.

> My own personal opinion is that we should investigate which
> clients are sending useful S/MIME capabilities; if most other SHA-2-capable
> clients are sending S/MIME capabilities indicating SHA-2 support then we
> could leave the default as SHA-1 but override that when we know the S/MIME
> capabilities of the receivers. And/or we could assume that a SHA-2-signed
> certificate implies SHA-2 CMS support (even though we know that isn't 100%
> true). If there are a lot of clients that aren't sending S/MIME capabilities
> then the decision is tougher.

I grepped through the mailing list archives of Kitten, but I only found messages generated by Apple and Thunderbird. I also found one from Alpine on the EAI working group. Alpine's output looks suspiciously similar to Thunderbird's, but it appears to use OpenSSL and not NSS (they both only list symmetric ciphers). Apple Mail doesn't appear to bother with the S/MIME capabilities at all.

So it sounds like S/MIME capabilities are quite useless for making these decisions.

Comment 45

[Kaspar]

(In reply to Joshua Cranmer [:jcranmer] from comment #44)
> Judging from asn1dump and grepping through an mbox archive of recent Kitten
> wg messages, it appears that the CMS code as of 24 only outputs the
> symmetric ciphers in the smimeCapabilities dump.

This observation is correct, yes. See

http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/smime/smimeutil.c#574
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/smime/smimeutil.c#626

(NSS_SMIMEUtil_CreateSMIMECapabilities)

Comment 46

[Mark Banner (:standard8)]

Comment on attachment 8429594 [details] [diff] [review]
Part 2 of patch to allow user to specify preferred hash algorithm when digitally signing email

I believe Joshua knows this code best.

Comment 47

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8429594 [details] [diff] [review]
Part 2 of patch to allow user to specify preferred hash algorithm when digitally signing email

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

The changes to nsMsgComposeSecure here conflict with bug 1018259.

::: mail/locales/en-US/chrome/messenger/am-smime.dtd
@@ +25,5 @@
>  <!ENTITY signMessage.accesskey "D">
>  <!ENTITY signingCert.message "Use this certificate to digitally sign messages you send:">
> +<!ENTITY digitalSign.preferred_hash_algorithm.label "Preferred Signature Hash Algorithm:">
> +<!ENTITY digitalSign.preferred_hash_algorithm.NoPreference.label "">
> +<!ENTITY digitalSign.preferred_hash_algorithm.SHA1.label "SHA1">

Is there really any need to localize the labels of "SHA1" (SHA-1? I don't know what the preferred spelling is here, but I think it's SHA-1)? If not, there's no need to have the labels here.

::: mailnews/extensions/smime/content/am-smime.js
@@ +90,5 @@
>      gSignMessages.checked = gIdentity.getBoolAttribute("sign_mail");
>      if (!gSignCertName.value)
>      {
>        gSignMessages.setAttribute("disabled", true);
> +      gPreferredSigningHashAlgorithm.value = gIdentity.getUnicharAttribute("preferred_signature_hash_algorithm");

You generally want Unicode attributes only when you expect non-ASCII characters to be used. In this case, after looking over the uses, I'm minded to make this an int-based pref that represents the value of nsICryptoHash::SHA* prefs.

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ +360,5 @@
> +  }
> +
> +  unsigned keylen;
> +  if ((subjectPublicKeyType == rsaKey) ||
> +      (subjectPublicKeyType == rsaPssKey)) {

I've made lots of comments on this code in relation to bug 1018259. Bug 1018259 is basically responsible for setting a sensible default hash if none is permitted. The code in that patch then gets reworked in this one to also check the validity of the preferred hash algorithm set in the dialog. I would say that I'd like to see this check done in the dialog, but we don't expose anything scriptable for JS to use (hmm....).

If the hash algorithm indicated by the preference is too strong, I think I'd prefer the attempt to sign a message to fail rather than silently use a weaker algorithm. This would, however, require a test to make sure that using too strong a hash causes the code to fail.

Comment 48

[David Cooper]

Attachment: Patch to allow user to specify preferred hash algorithm when digitally signing email

This patch should address all of the issues raised in comment 47. The patch for bug 1018259 needs to be applied first in order for this patch to work.

As suggested, if the user specifies a preferred hash algorithm then the code always tries to sign using that hash algorithm and signing fails if NSS cannot create a signature using the specified hash. Fortunately, at the moment signing will only fail if the user has a DSA key and selects either SHA-384 or SHA-512 as the hash algorithm, so users will be very unlikely to encounter this problem.

Comment 49

[Magnus Melin [:mkmelin]]

Comment on attachment 8462822 [details] [diff] [review]
Patch to allow user to specify preferred hash algorithm when digitally signing email

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

On a general level, why should users have do choose this? It's so not obvious what the best choice would be.

::: mailnews/extensions/smime/content/am-smimeOverlay.xul
@@ +53,5 @@
> +      <hbox align="center">
> +        <label id="identity.preferred_signature_hash_algorithm_label"
> +		      value = "&digitalSign.preferred_hash_algorithm.label;"/>
> +	<menulist id="identity.preferred_signature_hash_algorithm"
> +		prefstring="mail.identity.%identitykey%.preferred_signature_hash_algorithm">

please align attributes
 id="....
 value="....


 id="...
 prefstring="...

@@ +54,5 @@
> +        <label id="identity.preferred_signature_hash_algorithm_label"
> +		      value = "&digitalSign.preferred_hash_algorithm.label;"/>
> +	<menulist id="identity.preferred_signature_hash_algorithm"
> +		prefstring="mail.identity.%identitykey%.preferred_signature_hash_algorithm">
> +          <menupopup>

and give the popup an id

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ +441,5 @@
> +    }
> +
> +    if (preferredHashAlg) {
> +      mHashType = (int16_t)preferredHashAlg;
> +    } else {

nit: please follow the brace style in this file, 

if
{

}

here and elsewhere

Comment 50

[David Cooper]

Attachment: Patch to allow user to specify preferred hash algorithm when digitally signing email

> On a general level, why should users have do choose this? It's so not obvious
> what the best choice would be.

As I noted in comment 14, my goal all along has simply been to make it possible to sign emails using a stronger hash algorithm than SHA-1. Now that the patch in bug 1018259 has been checked in, my needs are addressed. However, as seen in bug 1111578, the problem that I noted in comment 14 about Outlook on Windows XP being unable to process emails signed using SHA-2 is still an issue for some people. So, I have addressed the comments that were provided in comment 49 in case there is interest in using this patch as a means of addressing bug 1111578.

Comment 51

[Magnus Melin [:mkmelin]]

Comment on attachment 8540934 [details] [diff] [review]
Patch to allow user to specify preferred hash algorithm when digitally signing email

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

Given bug 1111578 is Win XP only, why wouldn't a hidden pref (if anything) be enough?

I think my question still stands: why would users have to choose this. If they don't, we can (and do?) choose the strongest algorithm we think will work. So allowing it also degrades strength in the future as you limit it to whatever the user chose. And if we offer the choice, the first choice should be something like "Auto". 

Also add a comment about what the 0 and other values represents for the pref (in mailnews.js)

Comment 52

[Onno Ekker [:nONoNonO UTC+1]]

(In reply to Magnus Melin from comment #51)
> Given bug 1111578 is Win XP only, why wouldn't a hidden pref (if anything)
> be enough?
> 
> I think my question still stands: why would users have to choose this. If
> they don't, we can (and do?) choose the strongest algorithm we think will
> work. So allowing it also degrades strength in the future as you limit it to
> whatever the user chose. And if we offer the choice, the first choice should
> be something like "Auto". 

I'm not sure which hashing Thunderbird uses at the moment (or will use in version 38), but I can imagine that for performance reasons you want to sometimes use lower security (e.g. for e-mail that stays inside your own company) and sometimes you want to use higher security (e.g. if you know that the other party can read the message).

That would mean not only make the algorithm configurable, but also make at configurable per recipient!

Anyway, I sure hope this bug makes it into TB38, because otherwise i have to disable signing altogether... Please let me know if i can do anything to help.

Comment 53

[Ralf Hauser]

see also Bug 949564

Comment 54

[Onno Ekker [:nONoNonO UTC+1]]

@David: are you still working on this?

Comment 55

[David Cooper]

(In reply to Onno Ekker [:nONoNonO UTC+1] from comment #54)
> @David: are you still working on this?

No, I haven't been working on this. As I noted in comment 50, my goal has been to enable the use of strong hash functions, and that is addressed with bug 1018259. Since I understand that moving away from SHA-1 (for those signing with RSA keys longer than 1024 bits) causes problems for those who need to send signed emails to people still using Outlook on Windows XP I tried to get the patch I developed for this bug accepted as well. However, given that it isn't clear that a patch allowing users control over which hash algorithm is used to sign would be approved, it is difficult for me to justify putting more work into this patch.

I believe that it wouldn't be very difficult to develop a patch that would make selection of the hash algorithm a hidden preference, but even if such a patch were to be accepted, I don't see what the point would be. How many people would actually know about the existence of the hidden preference and be able to make use of it?

I don't think there is any reason to make the hash algorithm selectable for performance reasons. If you look at http://www.cryptopp.com/benchmarks.html, you'll see that hash computations are very fast. On a 64-bit 2.2 GHz processor, even the slowest hash algorithm (SHA-512) can hash 154 MiB/s. Given that the hash is computed just once for each email, regardless of the number of recipients, I think the only reason to support a weak hash algorithm would be to support recipients using very old software (e.g., Outlook on Windows XP) that cannot process signatures created using SHA-256.

Making the algorithm configurable per recipient (presumably with the idea of signing the message with the strongest algorithm that is supported by all recipients) would be very difficult to implement, and it would be a feature that almost no one would use. Perhaps it would be used if the selection could be automated, but that could only be done if the S/MIME capabilities that was included in messages received previously from the intended recipients of the message included their capabilities in the S/MIME capabilities attribute. But, as was discussed earlier, this doesn't happen.

So, I think making this configurable per recipient would be very difficult to implement and nearly impossible to use. Making it configurable, but not per recipient, via a hidden preference would be easy to implement, but would still be so difficult to use that almost no one would use it.

If someone else wants to work on this patch, it is fine with me. But, I can't justify continuing to work on a patch that may never be accepted.

Comment 56

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 58

[Fr3ddie]

Hello,

I have reached this bug report while trying to understand which type of symmetric encryption and hash algorithm (and the related key/hash sizes) Thunderbird is using for S/MIME support.

I would like to highlight that the status of Thunderbird S/MIME support is *not acceptable* at the moment.
It should be either completely *dropped* in future releases or David's patch should be integrated in the main tree and further extended to allow the user to select the used encryption algorithm too, along with the hashing algorithm.

This is because I believe it is unacceptable that a software in 2018 uses 3DES for encryption purposes and SHA1 for cryptographic signature without any warning or notification to the user. Moreover, at the moment it is not possible in any screen/window for the user to see and understand this behaviour, without the use of a different software that analyzes the S/MIME e-mail. No indication about the used symmetric and hash algorithms (and the related keys) is provided in any place of the GUI for S/MIME emails.

This behaviour is *worst* than not providing S/MIME functionality in any case, since it gives the user a *false sense of security* using old and weak encryption/hashing algorithms, that are deprecated everywhere and only left for retro-compatibility in any other software/platform I have seen to date.

Moreover, this behaviour is also in contrast with RFC5751 that, in section 2.7 ( https://tools.ietf.org/html/rfc5751#section-2.7 ), states that an S/MIME compliant software MUST support AES-128-CBC and SHOULD+ support AES-192-CBC and AES-256-CBC. The same paragraph also states that *SHOULD-* support DES-EDE3-CBC, clearly for retro-compatibility purposes.

I believe many would agree with this position about current S/MIME support in Thunderbird and I really hope it will be dropped as soon as possible, of course if Mozilla has no plans to correct such wrong behaviours.

Thank you

Comment 59

[David Cooper]

While it is true that this aspect of Thunderbird's S/MIME support could use an update, the information you cite above is out of date (see comment 55 above). Thunderbird moved away from signing with SHA-1 about 4 years ago (bug 1018259). Now, if the message is being signed with an RSA key, the message will only be signed using SHA-1 if the signer's key is 1024 bits or less in length. So, if the signer uses a key with an acceptable key length, then the message will be signed using SHA-256 or stronger.

Thunderbird does still use 3DES for encryption. As far as I know the code for selecting the symmetric encryption algorithm was last updated 6 years ago (bug 676108). It was acknowledged even back then that at some point a move should be made from 3DES to AES (https://bugzilla.mozilla.org/show_bug.cgi?id=676108#c4), but it would have been inappropriate to make the switch at that time, since there was still many clients in use that didn't support AES. Hopefully it would be agreed that at this point there is no longer a need for interoperability with clients that don't support AES decryption. So, I agree that it should be changed, and while I haven't looked at the Mozilla code in several years, I might be able to develop a patch for it. I do not think there is any need to drop the S/MIME functionality from Thunderbird.

At the current time I don't think there is a compelling need to allow users to select encryption and signing algorithms. For signer's the code automatically chooses a hash algorithm (SHA-1, SHA-256, SHA-384, or SHA-512) that provides comparable strength to the private key being used to sign the message. The code for selecting an encryption algorithm tries to do the same thing. For the moment it only chooses between AES128 and AES256 if the recipients have elliptic curve keys, but the same could be done in the case that recipients have RSA keys: use AES128 in most cases, but use AES256 if all the recipients have very large RSA keys.

In the past there may be been a security vs. interoperability trade-off (use SHA-1 for interoperability or SHA-2 for security), and so it may have made sense to allow the user to choose between those two options. Now, however, there doesn't seem to be such a trade-off as we can probably assume that all clients will support the stronger algorithms, so there doesn't seem to be a need to allow for user selection.

I can't rule out the possibility that something like this will happen in the future. There may at some point be a transition to yet another set of algorithms, and there will be a need to choose between the current algorithms (supported by all clients) and the newer, stronger algorithms (not yet supported by everyone), but I don't see anything like that happening anytime soon.

Comment 60

[Fr3ddie]

David,

of course I trust you about the signature behaviour (I haven't checked, encryption with 3DES was enough for me...) and I have the best respect for you and your past work.

But please let me argue that on the rest you're fiddling around.
Let's stick to the facts:

- Thunderbird is non-compliant to the S/MIME RFC (RFC5751), dated *January 2010*;
- Thunderbird is happily using a (long!) outdated and weak (read: *cracked*) encryption algorithm for a security feature;
- Thunderbird is doing that absolutely *silently* (there is no notification/label anywhere to the user!).

Considering the facts, Thunderbird S/MIME implementation is currently broken.

For such a situation, in my opinion, there are two possible quick&simple actions to be taken *as soon as possible* if there is no plan to update the feature to be in line with standards and with *real* security (independently from the solution chosen, if it is leaving the control of the algorithms to the user or if it's better to provide an automatic procedure):
- drop S/MIME support (at least for encryption, based on what you say about signatures);
- insert, at least, a *very-big* warning message about the use of 3DES as cipher by Thunderbird.

Security folks (including me) *horrify* thinking that there are still softwares that happily use 3DES in 2018.
*3DES has to be considered cracked* and it could have been possibly considered cracked also 6 years ago.
It is unacceptable to use it anymore outside of the scope of interfacing legacy systems to provide a "security feature" whatsoever.

I guess this topic would deserve a dedicated bug report.

Comment 61

[David Cooper]

As I mentioned in my previous message, I have not looked at the Mozilla code or at Bugzilla for several years. However, I just did a quick search and found the following two bug reports related to this issue: bug 1167857 and bug 1364568.

Comment 62

[Wayne Mery (:wsmwk)]

Kaie, can you offer thoughts on comment 51, to which so far there have been no feedback. Thanks

Comment 63

[Kai Engert [:KaiE:]]

(In reply to Wayne Mery (:wsmwk) from comment #62)

Kaie, can you offer thoughts on comment 51, to which so far there have been no feedback.

Which part of comment 51 hasn't gotten feedback yet?

Comment 64

[Wayne Mery (:wsmwk)]

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

(In reply to Wayne Mery (:wsmwk) from comment #62)

Kaie, can you offer thoughts on comment 51, to which so far there have been no feedback.

Which part of comment 51 hasn't gotten feedback yet?

I guess there has been feedback, but no developer decision on whether a pref would be accepted and so with comment 59, cooper is out. And there is comment 58/comment 60 - how relevant is it to this bug?

Given bug 1111578 is Win XP only, why wouldn't a hidden pref (if anything) be enough?

Note bug 1111578 was wontfix two years ago, and XP is long defunct.

Comment 65

[Adriano Santoni]

I'd like very much TB to let me choose the algorithms to use for S/MIME. Outlook has been allowing this for years.
At least I'd want to be able to choose between 3DES and AES256 for encryption, and the hashing function (SHA256/SHA384/SHA512) for signing.

Comment 66

[Kai Engert [:KaiE:]]

So I've skimmed this bug and would like to ask that we summarize, and also give my opinion, based on impression.

The original request is to introduce user preferences for the algorithms that are used when creating emails.
Several people have argued it should be unnecessary.
I also agree that we should avoid this additional complexity in user visible prefs.
At most, we could consider a hidden preference.
(But I'd like to hear a compelling argument why this would still be a significant and necessary improvement).

There were some claims that TB uses outdated algorithms.
As I understand it, that is no longer correct?
It seems to have been confirmed that we no longer 3DES for encryption, and at least SHA256 for signing?

In bug 1167857 David suggests we should support AES-256 GCM for better complicance with RFC 5751.

See also bug 1847703 comment 13 which has my attempted summary of current TODO items for S/MIME.