232515 - S/MIME support not RFC2633 compliant: transfer-encoding problem - no transfer encoding applied when ascii msg body

Status: RESOLVED FIXED

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

Description

[julien.stern]

User-Agent:       
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5) Gecko/20031107 Debian/1.5-3

When sending an s/mime multipart/signed email containing 8bit characters,
mozilla does NOT use a transfer-encoding which has a 7bit representation, as
mandatory in the RFC.

The relevant snippet of the RFC (RFC2633) follows:

3.1.3 Transfer Encoding for Signing Using multipart/signed
                                                                                
   If a multipart/signed entity is EVER to be transmitted over the
   standard Internet SMTP infrastructure or other transport that is
   constrained to 7-bit text, it MUST have transfer encoding applied so
   that it is represented as 7-bit text. MIME entities that are 7-bit
   data already need no transfer encoding. Entities such as 8-bit text
   and binary data can be encoded with quoted-printable or base-64
   transfer encoding.



Reproducible: Always
Steps to Reproduce:
1. Send an signed (but not encrypted) s/mime email containing non ascii characters
2.
3.

Actual Results:  
The mail is sent with a transfer encoding of Content-Transfer-Encoding: 8bit
This makes the signature INVALID in many many cases (either because the path is
not 8bit clean or because an agent transcodes the mail).

Expected Results:  
The mail should have been sent with a 8 bit clear transfer encoding, such as
base64 or quoted-printable.

Comment 1

[John G. Myers]

This can be enabled using the "quoted printable" preference on the
Mail&Newsgroups/Composition preference pane.

7bit-only transports are exceedingly rare to nonexistent.  MTAs are prohibited
from transcoding multipart/signed.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I'm reopening this bug.  About half the signed emails I receive from other
mozilla (Seamonkey, THunderbord) users show invalid signatures when they
are received, precisely because MTAs alter them in transit, whether it is
prohibited or not.  Common corruptions induced by MTAs include:
- changing line endings
- removing trailing spaces from lines
- converting from 8-bit to 7-bit enocdings.  
- Appending a mailing list blurb to the end of message text.

One can argue that all those MTAs are broken, violating RFCs, but the 
simple fact is that if mozilla always base64 encoded all signed emails,
these problems would completely disappear.   Our objective is to make
signed emails work reliably for our users, not to play finger-pointing 
games.

mozilla's mail/news code has huge problems with replying to quoted-printable
messages, so I would not recommend that we rely on quoted-printable as a 
solution to *ANYTHING*.  

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Dan,
Here's a 4 year old bug that makes S/MIME pretty close to useless for 
many users whose mail servers tend to mangle bare unencoded mail.
Base64 encoding would solve these problems, and we clearly already 
implement it.  So, the solution seems pretty simple: just treat all 
signed emails as if they were binary and base64 encode them. 

I'd nominate this bug as blocking TB 3, but since it's a "core" bug,
I can't.  There are a LOT of serious bugs that have that problem. 
They should be blocking TB, but they can't because they're core.

Comment 4

[Magnus Melin [:mkmelin]]

Seems to get properly base64 encoded in my testing. This looks like a dupe of the fixed bug 256851. Are you really still seeing it?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

I see this every day with SM trunk.
Understand that the problem is NOT only limited to "8-bit" email.
It's a problem for pure-ASCII email too.  I can show you simple plain text 
ASCII emails that arrive mangled, resulting in invalid signatures.
Basically, all parts of signed emails need to always be sent base64 encoded.

Comment 6

[Magnus Melin [:mkmelin]]

But are those from mozilla products? If they are, how do I reproduce it? I tried sending a signed mail, and the content part was base64 encoded.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

> But are those from mozilla products? 
Yes, the signed messages I send every day using SM trunk are sent without
any base64 encoding of the content.  

> If they are, how do I reproduce it?
Compose a plain text email and send it signed.  That's all I do.
I don't know any way to get such messages to be sent using base64 encoding.
I have NEVER seen SM send such an email using base 64 encoding. 
I'll be happy to send you an example. 

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: signed text/plain email I just sent to Magnus

Here's a copy of a signed plain text email I just sent. 
It is entirely typical of the signed plain text messages I send daily.
Notice that the main message body part is not base64 encoded.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

I'd mark this bug as requesting blocking TB3, if it were possible to do so.

Comment 10

[Magnus Melin [:mkmelin]]

Ah, did another test and apparently it gets quoted-printable/base64 encoded *only* if there is non-ascii in the content. (Which it doesn't normally.)

Comment 11

[Magnus Melin [:mkmelin]]

Taking for now.

Comment 12

[Magnus Melin [:mkmelin]]

Bug 281252 also did some work for this.

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

This makes sure signed/encrypted messages always get transfer encoding applied. 
Which one (quoted printable/base64) is determined as before.

Notes:
 - AnalyzeSnarfedFile is already called in PickEncoding
 - InitCompositionFields needed to get called a bit later so the field values really get set

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 330455 [details] [diff] [review]
proposed fix


>@@ -3286,30 +3281,43 @@ nsMsgComposeAndSend::Init(
>   //
>   PRBool strictly_mime = PR_TRUE;
>   nsCOMPtr<nsIPrefBranch> pPrefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID));
>   if (pPrefBranch)
>   {
>     rv = pPrefBranch->GetBoolPref(PREF_MAIL_STRICTLY_MIME, &strictly_mime);
>     rv = pPrefBranch->GetIntPref(PREF_MAIL_MESSAGE_WARNING_SIZE, (PRInt32 *) &mMessageWarningSize);
>   }
> 
>-  if (!strictly_mime)
>+  nsCOMPtr<nsIMsgComposeSecure> secureCompose;
>+  secureCompose = do_CreateInstance(NS_MSGCOMPOSESECURE_CONTRACTID, &rv);
>+  if (NS_SUCCEEDED(rv) && secureCompose)
>   {

secureCompose should be initialised in its constructor or via = on its construction line, otherwise we may be causing an initialising to null, then setting it up.

If you don't like the line length, put the = on the end of the first line, and two-space the do_CreateInstance on the next line.

You shouldn't need to check for NS_SUCCEEDED(rv) and secureCompose there either. I think you can just turn that if into NS_ENSURE_SUCCESS(rv, rv); and then you don't need to keep the next section indented.

r=me with that fixed.

Comment 15

[Magnus Melin [:mkmelin]]

Actually, those shouldn't be errors (= just missing s/mime extension). At least there are such comments e.g. in BeginCryptoEncapsulation.

Comment 16

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Fixed the first review comment. Carrying forward r=standard8.

Comment 17

[David :Bienvenu]

Comment on attachment 334076 [details] [diff] [review]
proposed fix, v2

thx for the patch, and for the notes (very helpful).

sr=bienvenu, with one nit:


+        {
+          PR_FREEIF(attachments[i].encoding);
+          attachments[i].encoding = PL_strdup(ENCODING_8BIT);
+        }

You can use PR_Free instead of PR_FREEIF since we're assigning attachments[i].encoding on the next line, and PR_Free already checks for null.

Comment 18

[Magnus Melin [:mkmelin]]

changeset:   119:a4294d175bf6
http://hg.mozilla.org/comm-central/index.cgi/rev/6e8b065f2b6c
->FIXED

Comment 19

[Mark Banner (:standard8)]

I've just backed this out due to unit test failures (all platforms). Unfortunately it wasn't just tinderbox weirdness (as some of the oranges have been recently).

From my local debug build I was getting:

TEST-UNEXPECTED-FAIL | ../../../mozilla/_tests/xpcshell-simple/test_compose/unit/test_sendMessageFile.js | test failed, see log
../../../mozilla/_tests/xpcshell-simple/test_compose/unit/test_sendMessageFile.js.log:
>>>>>>>
*** registering jsconsole-clhandler.js: [ jsConsoleHandler ]
*** Registering components in: nsCookieModule
*** Registering components in: embedcomponents
*** Registering components in: nsImportServiceModule
*** Registering components in: nsMailModule
*** Registering components in: nsLDAPProtocolModule
*** registering nsAddonRepository.js: [ Addon Repository ]
*** registering nsBlocklistService.js: [ Blocklist Service ]
*** registering nsSpotlightIntegration.js: [ Spotlight Integration ]
*** registering nsURLFormatter.js: [ Application URL Formatter Service ]
*** test pending
Wants directory: MailD
Wants directory: MFCaF
Wants directory: DefRt
###!!! ASSERTION: You can't dereference a NULL nsRefPtr with operator->().: 'mRawPtr != 0', file ../../../mozilla/dist/include/xpcom/nsAutoPtr.h, line 1082
nsGetServiceByCID::nsGetServiceByCID(nsID const&)+0x00000894 [/Users/moztest/comm/main/tb/mozilla/dist/bin/components/libmail.dylib +0x001B67E4]
nsVoidArray::operator[](int) const+0x00060D67 [/Users/moztest/comm/main/tb/mozilla/dist/bin/components/libmail.dylib +0x001B4DE9]
nsVoidArray::operator[](int) const+0x0005E36C [/Users/moztest/comm/main/tb/mozilla/dist/bin/components/libmail.dylib +0x001B23EE]
NS_InvokeByIndex_P+0x00000063 [/Users/moztest/comm/main/tb/mozilla/xpcom/build/libxpcom_core.dylib +0x000A0B07]
nsSupportsArray::AppendElement(nsISupports*)+0x0000544D [/Users/moztest/comm/main/tb/mozilla/dist/bin/components/libxpconnect.dylib +0x0004ECE7]
nsCRT::free(unsigned short*)+0x00005135 [/Users/moztest/comm/main/tb/mozilla/dist/bin/components/libxpconnect.dylib +0x000595B1]
js_Invoke+0x000008B8 [/Users/moztest/comm/main/tb/mozilla/js/src/libmozjs.dylib +0x000793A2]
JS_CompareValues+0x00010024 [/Users/moztest/comm/main/tb/mozilla/js/src/libmozjs.dylib +0x0006C0C4]
js_FreeStack+0x0000104B [/Users/moztest/comm/main/tb/mozilla/js/src/libmozjs.dylib +0x00077F8D]
JS_ExecuteScript+0x00000083 [/Users/moztest/comm/main/tb/mozilla/js/src/libmozjs.dylib +0x00013B39]
start+0x0000178A [/Users/moztest/comm/main/tb/mailnews/compose/test/../../../mozilla/dist/bin/xpcshell +0x00002BFA]
start+0x00001ABF [/Users/moztest/comm/main/tb/mailnews/compose/test/../../../mozilla/dist/bin/xpcshell +0x00002F2F]
start+0x00002119 [/Users/moztest/comm/main/tb/mailnews/compose/test/../../../mozilla/dist/bin/xpcshell +0x00003589]
start+0x00003376 [/Users/moztest/comm/main/tb/mailnews/compose/test/../../../mozilla/dist/bin/xpcshell +0x000047E6]
start+0x000000FC [/Users/moztest/comm/main/tb/mailnews/compose/test/../../../mozilla/dist/bin/xpcshell +0x0000156C]
start+0x00000029 [/Users/moztest/comm/main/tb/mailnews/compose/test/../../../mozilla/dist/bin/xpcshell +0x00001499]

<<<<<<<

I couldn't see anything obvious at a quick glance.

Magnus, if you need any help running unit tests, see https://wiki.mozilla.org/MailNews:Xpcshell_tests or poke me on irc.

Comment 20

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v4

So the unit test failure was because I hadn't moved the if (sendFile) clause down, only the InitCompositionFields call => InitCompositionFields was never called when sending a file from disk.

Comment 21

[Magnus Melin [:mkmelin]]

Relanded. changeset:   178:0ab76ee87645
http://hg.mozilla.org/index.cgi/comm-central/rev/0ab76ee87645