Add recognition of SHA-2 hashes when verifying S/MIME messages with detached signatures (application/pkcs7-signature)

RESOLVED FIXED in Thunderbird 3.1b2

Status

MailNews Core
MIME
--
major
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: Scout, Assigned: Kaspar Brand)

Tracking

({testcase})

Trunk
Thunderbird 3.1b2
testcase

Firefox Tracking Flags

(blocking-thunderbird3.1 beta2+, thunderbird3.1 beta2-fixed)

Details

(Whiteboard: oe-parity)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.1.7) Gecko/20091221 Firefox/3.5.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.1.5) Gecko/20091204 Thunderbird/3.0

Only at this VeriSign signed mail the indicator not apear and no Message appear.
If i got mail from another buddy the indicator appears.

Reproducible: Always

Steps to Reproduce:
1. download my attached zip
2. import public certs if you want (not needed usually)
3. open Testmail5_source.eml > signing indicator not appear
(4. verify Mail with OpenSSL with "openssl smime -CAfile VeriSign_RootCA.crt -verify -in Testmail5_source.txt") > Verification successful
Actual Results:  
the indicator not appear

Expected Results:  
show the indicator with result "signature ok" or not
(Reporter)

Comment 1

8 years ago
Created attachment 422940 [details]
Mailbody and Certs
(Reporter)

Updated

8 years ago
(Reporter)

Updated

8 years ago
Summary: I got signed mail - open it but digital signing indicator not appear → I got smime signed mail - open it but digital signing indicator not appear
(Reporter)

Comment 2

8 years ago
Do not work on Thunderbird/3.0.1 too.
Severity: normal → major
Status: UNCONFIRMED → NEW
Component: Mail Window Front End → Security
Ever confirmed: true
Flags: blocking-thunderbird3.1?
Keywords: testcase
OS: Windows 7 → All
QA Contact: front-end → thunderbird
Hardware: x86 → All
Scout, thanks for the testcase, that's a big help.

Ludo, I don't think we'd block on this without knowing more about the scope here.  Which is to say, if this happens with all S/MIME messages, we'd certainly block on it.  If it happens with a tiny percentage, we probably wouldn't.  Adding qawanted, and setting to blocking- for now.
blocking-thunderbird3.1: --- → -
Flags: blocking-thunderbird3.1?
Keywords: qawanted
SO it does work with some certs and messages. Bob could you dig into the testcase to figure out what could be triggering the issue ?

Comment 5

8 years ago
Scout, thank you so much for the detailed report.  I was able to confirm that the command line tools verify the message, and that TB does not show any indicator at all. That's strange! I'm surprised I have not seen this issue before!

Interestingly, the Apple Mail app does just what TB does, which is to show no indication of S/MIME. 

I looked at the MIME boundaries and they appear to be OK, though perhaps a MIME expert could look at them to see if there is something wrong there. 
==>Ludo: can you have someone peek at the MIME setup to see if there is something going on there?  Maybe it's a MIME issue and not an S/MIME issue.  

It looks like you are using Office Outlook 2007. Is that right? Are you able to send signed emails to other Outlook 2007 users with success? Are there other mail clients that successfully parse your emails?

If you would like, you can try to send emails to me to test a little more.  I wonder if you send a signed email to bob.lord at gmail what would happen.
(Assignee)

Comment 6

8 years ago
Created attachment 427536 [details] [diff] [review]
Patch for recognizing SHA-2 micalgs in libmime (c-c part)

It's the micalg parameter in the Content-Type header which is the culprit.

libmime currently only recognizes the SHA-1 (plus MD2/MD5) hashes, probably also due to the fact that even the S/MIME 3.1 spec (RFC 3851, July 2004) does not yet recommend its use. With S/MIME 3.2 (RFC 5751), however, at least SHA-256 became a MUST, so I think it's definitely useful to add recognition of the SHA-2 hashes.

The attached path (comm-central part) adds the "offical" values from RFCs 5751 and 3851, and additionally also its OID "counterparts"... as Microsoft apparently uses this format with Outlook (the OID format isn't really covered in the S/MIME spec, but it isn't forbidden either).

The sample message in attachment 422940 [details] uses SHA-512 for the signature. With this patch applied - and an additional one for mozilla-central, which I will attach in a few seconds -, Thunderbird will correctly detect and verify the signature. Note that this fix only addresses the case of detached S/MIME signatures, I did not yet look at opaque signatures. Also, I guess it would be a good idea to extend nsMsgComposeSecure::MimeInitMultipartSigned with support for SHA-2 hashes (plus a preference and possibly a UI for picking a particular hash).

Martin, can you try to configure Outlook to use SHA-256 and SHA-384, respectively, and create two test messages? It would be interesting to see if it also puts OIDs into the micalg parameter in these cases.
(Assignee)

Comment 7

8 years ago
Created attachment 427537 [details] [diff] [review]
Patch for recognizing SHA-2 micalgs in libmime (m-c part)
[Checkin: Comment 22]
Component: Security → MIME
Keywords: qawanted
Product: Thunderbird → MailNews Core
QA Contact: thunderbird → mime
Whiteboard: oe-parity
Version: unspecified → Trunk
(In reply to comment #6)
> Created an attachment (id=427536) [details]
> Patch for recognizing SHA-2 micalgs in libmime (c-c part)

Thank you very much for the patch, according to https://developer.mozilla.org/en/comm-central#Requirements you should set a review request in order to be able to get your patch pushed in the tree. We also recently added a policy for new patches to come with unit tests (see http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/c316a9cd94199e16/b5ba6be4b90eaa6c?lnk=gst&q=test+policy#b5ba6be4b90eaa6c).
Comment on attachment 427536 [details] [diff] [review]
Patch for recognizing SHA-2 micalgs in libmime (c-c part)

setting Review flags
Attachment #427536 - Flags: superreview?(bugzilla)
Attachment #427536 - Flags: review?(bienvenu)
Whiteboard: oe-parity → oe-parity [needs r bienvenu][needs sr standard8]
Attachment #427537 - Flags: review?(cbiesinger)
Assignee: nobody → mozbugzilla
Attachment #427537 - Flags: review?(cbiesinger) → review+
Comment on attachment 427537 [details] [diff] [review]
Patch for recognizing SHA-2 micalgs in libmime (m-c part)
[Checkin: Comment 22]

Ain't sure sr is needed but asking anyhow
Attachment #427537 - Flags: superreview?(bzbarsky)
(Reporter)

Comment 11

8 years ago
(In reply to comment #6)
> Martin, can you try to configure Outlook to use SHA-256 and SHA-384,
> respectively, and create two test messages? It would be interesting to see if
> it also puts OIDs into the micalg parameter in these cases.

I don#t know where you find the micalg parameter. But i think will you find them themself ;-)

Yes, I use Outlook Prof. Plus 2007 with SP2 to send the signed mails.

As desired in your comment i attach some testmails with different hash values.

Only at mails with MD5 hash or SHA1 hash algorithm the signing indicator appears.
(Reporter)

Comment 12

8 years ago
Created attachment 427598 [details]
Testmails with different Hash values

The signing indicator appears at md5 and sha1 hash only!

Updated

8 years ago
blocking-thunderbird3.1: - → rc1+
(Assignee)

Comment 13

8 years ago
(In reply to comment #12)
> Created an attachment (id=427598) [details]
> Testmails with different Hash values

Danke. This confirms that Outlook puts in OIDs for all SHA-2 hashes (not just for SHA-512):

Testmail Hashalg-SHA-256.eml:   micalg=2.16.840.1.101.3.4.2.1;
Testmail Hashalg-SHA-384.eml:   micalg=2.16.840.1.101.3.4.2.2;
Testmail Hashalg-SHA-512.eml:   micalg=2.16.840.1.101.3.4.2.3;

Which means that the patches I attached above can be kept as-is (three variants for each of the three SHA-2 hashes - one according to RFC 5751, one according to RFC 3851, and one dealing with the "Outlook" format).

> The signing indicator appears at md5 and sha1 hash only!

Yes, this is expected.

(In reply to comment #8)
> We also recently added a policy for new patches to come with unit tests (see
> http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/c316a9cd94199e16/b5ba6be4b90eaa6c?lnk=gst&q=test+policy#b5ba6be4b90eaa6c).

Well, then I'd like to ask for an exception, because "it's specifically impractical and the cost/benefit tradeoff is sufficiently poor" :-) No, seriously: the code changes really look straightforward/trivial to me, and an automated test would actually have to check for the presence of the UI indicator... is this something which is doable with any of the currently available test frameworks? Apart from that, I should add that I can't really spend much time for getting familiar with a new test framework right now. But if someone else is willing to jump in, then he is certainly welcome to do so.
Comment on attachment 427537 [details] [diff] [review]
Patch for recognizing SHA-2 micalgs in libmime (m-c part)
[Checkin: Comment 22]

sr=bzbarsky
Attachment #427537 - Flags: superreview?(bzbarsky) → superreview+
Keywords: checkin-needed
(Assignee)

Comment 15

8 years ago
(In reply to comment #6)
> Note that this fix only addresses the case of detached S/MIME
> signatures, I did not yet look at opaque signatures.

For the sake of completeness: opaquely signed S/MIME messages with SHA-2 hashes already work properly in Tb 3.0 (and earlier versions, too), as I have verified in the meantime. I'm therefore changing the summary to better reflect what the focus of this bug is.
Summary: I got smime signed mail - open it but digital signing indicator not appear → Add recognition of SHA-2 hashes when verifying S/MIME messages with detached signatures (application/pkcs7-signature)

Comment 16

8 years ago
Comment on attachment 427536 [details] [diff] [review]
Patch for recognizing SHA-2 micalgs in libmime (c-c part)

this looks really straightforward, and it builds and runs, as long as the moz-central patch is also applied.

I think the mozmill test test-message-header.js could probably be extended to test s/mime though I'm not an expert on that code or test. dmose could probably say more.
Attachment #427536 - Flags: review?(bienvenu) → review+
(In reply to comment #13)

> Well, then I'd like to ask for an exception, because "it's specifically
> impractical and the cost/benefit tradeoff is sufficiently poor" :-)

Completely reasonable to ask!

> No, seriously: the code changes really look straightforward/trivial to me,

Very true.  There are not complexity reasons to want to tests here, but just having an exceedingly basic automated functional test so that we notice if this breaks in the future would be very valuable, particularly as we try to make the message header code better here.

> and an automated test would actually have to check for the presence of the UI
> indicator... is this something which is doable with any of the currently
> available test frameworks? 

Yep, MozMill is exactly targeted towards this sort of testign!

> Apart from that, I should add that I can't really
> spend much time for getting familiar with a new test framework right now. But
> if someone else is willing to jump in, then he is certainly welcome to do so.

As David says, I think we've actually got most of the infrastructure work already done.  Would you be willing to spend a little time with test-message-header.js to see if you can figure out how to add a single test to that file?
(Assignee)

Comment 18

8 years ago
(In reply to comment #17)
> Would you be willing to spend a little time with test-message-header.js
> to see if you can figure out how to add a single test to that file?

I had a short look at it, and I don't think it's simply a matter of adding a few more lines to headersToTest or so, I'm afraid. What the test would have to do is roughly:

1) load a message with a known good S/MIME signature from a file (with one of the SHA-2 hashes, and a detached signature)

2) open the message window (or load it into the message pane), and test a) if the "signedHdrIcon" element is not collapsed and b) the value of its "signed" attribute is equal to "ok".

While using the test messages from attachment 427598 [details] might be tempting, it should be noted that they will fail to produce "ok" after 2010-11-21, as the signer's certificate expires at that date. I.e., a careful preparation of suitable S/MIME test messages (possibly with an own trust anchor, which would have to be added to the cert DB first) is an additional requirement.

As mentioned before, I can't really spend much more time on this bug, so I would hope that the "awesome test engineer" picks this up once he has been hired - and if the cost/benefit tradeoff is still not considered poor enough...
Status: NEW → ASSIGNED
Whiteboard: oe-parity [needs r bienvenu][needs sr standard8] → oe-parity [needs sr standard8]
Thanks for taking the time to look into this in more detail.  Agreed that steps 1 and 2 sound like what's needed.

You make a very good point about cert expirations here.  Bob, KaiE, Jonath, do we have a general way of addressing this in any other S/MIME (or even SSL) tests?
I believe m-c generates some sort of certs (possibly for use with ssl) for unit tests. I haven't looked at the details.

Comment 21

8 years ago
(In reply to comment #19)
> Thanks for taking the time to look into this in more detail.  Agreed that steps
> 1 and 2 sound like what's needed.
> 
> You make a very good point about cert expirations here.  Bob, KaiE, Jonath, do
> we have a general way of addressing this in any other S/MIME (or even SSL)
> tests?

Expiration of S/MIME certs can be tricky as Kaspar points out. To control variables, it would be best to have an internal CA issue the certs, and to have the test messages regenerated on a regular basis. 

If you point me to an appropriate QE wiki page, I'd be happy to dump my thoughts of what we might want to do.
Comment on attachment 427537 [details] [diff] [review]
Patch for recognizing SHA-2 micalgs in libmime (m-c part)
[Checkin: Comment 22]


http://hg.mozilla.org/mozilla-central/rev/375c5041aff8
Attachment #427537 - Attachment description: Patch for recognizing SHA-2 micalgs in libmime (m-c part) → Patch for recognizing SHA-2 micalgs in libmime (m-c part) [Checkin: Comment 22]
URL: http://
Keywords: checkin-needed
We have a pre-built cert db for Mochitests (along with some of the specific certs that make it up, the rest are generated as-needed) :
http://mxr.mozilla.org/mozilla-central/source/build/pgo/certs/

There's a Makefile target "genservercert" in the build/pgo directory that regenerates the db:
http://mxr.mozilla.org/mozilla-central/source/build/pgo/Makefile.in#83

Currently Mochitest sets up its profile to trust the testing CA listed there as well as some client certs, and ssltunnel uses those certs for proxying SSL connections:
http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#375
(In reply to comment #14)
> (From update of attachment 427537 [details] [diff] [review])
> sr=bzbarsky

Requesting wanted for 1.9.2 for the core part of the patch. This is low risk and would make TB interact with newer version of OE much better wrt to cert signed/encrypted emails.
status1.9.2: --- → ?
(In reply to comment #24)
> (In reply to comment #14)
> > (From update of attachment 427537 [details] [diff] [review] [details])
> > sr=bzbarsky
> 
> Requesting wanted for 1.9.2 for the core part of the patch. This is low risk
> and would make TB interact with newer version of OE much better wrt to cert
> signed/encrypted emails.

As that attachment is #defines only, and therefore doesn't affect the gecko build, I'm reluctant to add additional load on the 1.9.2 drivers to get this in (they already have many bugs awaiting approval). Instead I think we can just do this via an ifdef in comm-central; it will get dropped when we branch comm-1.9.2 from comm-central so going forward we'll get the right solution anyway.
status1.9.2: ? → ---
Created attachment 435583 [details] [diff] [review]
Patch for recognizing SHA-2 micalgs in libmime (c-c part) v2

This patch updates the c-c part to include in an 1.9.2 ifdef the defines from attachment 427537 [details] [diff] [review] and a note where they are defined on trunk and is what I'll land on behalf of Kaspar in a few minutes.

Kaspar, sorry for the delay in review. The patch looks good and works well.
Attachment #427536 - Attachment is obsolete: true
Attachment #435583 - Flags: superreview+
Attachment #435583 - Flags: review+
Attachment #427536 - Flags: superreview?(bugzilla)
Checked in: http://hg.mozilla.org/comm-central/rev/4e9d373dfab1

I'll mark this bug as fixed (as it is now fixed), but I'll also mark as in-testsuite? as we'd really like to get a test implemented for this. If guidance is needed, feel free to ask me.
Status: ASSIGNED → RESOLVED
blocking-thunderbird3.1: rc1+ → beta2+
Last Resolved: 8 years ago
status-thunderbird3.1: --- → beta2-fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: oe-parity [needs sr standard8] → oe-parity
Target Milestone: --- → Thunderbird 3.1b2
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.