#3 S/MIME broken reading signed-then-encrypted messages

[Joshua Miller]

Viewing S/MIME signed emails works fine.
Viewing S/MIME encrypted only emails works fine.
If a message is both signed and encrypted, it fails with an error ("Couldn't load PKCS7
object: unknown error")

I've tracked down the cause of the error. There is another error message that is being discarded from detach.c detach: "Can't find body for requested message".

I've included a patch with fixes this issue. There's a chance it may break handling of multipart mime messages with signed or encrypted parts that are buried down in the structure. However, it fixes signed-then-encrypted, and just signed or just encrypted still work.

Here's what is happening:

In alpine/mailview.c, mail_view_screen(), if it's an smime message, it
calls: fiddle_simime_message(body, msgno)

fiddle_smime_message is in pith/smime.c, and just calls
do_fiddle_smime_message (which can call itself recursively).

Most of the rest of this happens in pith/smime.c:

do_fiddle_smime_message:
* on first pass, section=Top (null).
* is_pkcs7_body is true
* do_decoding is called (section=Top)
** there is no sparep (spare pointer) holding previous pk7 data
** newSec is made into "1" from emptystring.
** get_pkcs7_from_part(msgno, section) (section=1) is called
*** get_part_contents(msgno, section) (section=1) is called
**** detach(ps_global->mail_stream, msgno, section, ...) (section=1) is called
***** mail_body(stream, msgno, section) (section=1) is called
*** store (from get_part_contents) ->src == CharStar
*** BIO is taken from that
*** p7=d2i_PKCS7_bio on the BIO
*** returns p7
** body is decrypted in do_decoding
* body is multipart, signed once...
* hits the "else" part, loops over parts recursively calling
do_fiddle_smime_message
* uses current section (""=Top) and partNum to make newSec
# (my message only has one part here)
* newSec = 1
* calls do_fiddle_smime_message(part->body, msgno, newSec) where newSec = 1

do_fiddle_smime_message:
* second pass - from the loop over decrypted multi-part message parts
* is_pkcs7_body is true
* do_decoding is called (section=1)
** there is no sparep (spare pointer) holding previous pk7 data
** newSec is made into "1.1" from "1".
** get_pkcs7_from_part(msgno, newSec) (section=1.1) is called
*** get_part_contents(msgno, section) (section=1.1) is called
**** detach.c - detach(ps_global->mail_stream, msgno, section, ...) (section=1.1) is called
***** c-client/mail.c - mail_body(stream, msgno, section) (section=1.1) is called
----
---- This is where it breaks---- The "stream" is the global stream
---- The first call to do_decoding has updated the message in the stream.
---- It replaces the existing body with a multipart body that has one part.
---- So, no section 1.1 exists.
---- mail_body returns NIL
----

**** detach.c - detach returns error: "Can't find body for requested message"
*** get_part_contents doesn't do anything with the error (should probably report it)
** get_pkcs7_from_part doesn't do anything if the part isn't found,just returns NULL
* do_fiddle_smime_message: updates status with "Couldn't load PKCS7 object: unknown error"
---- the error is unknown here because it's passing the openssl_error_string(),
---- but this isn't an openssl error. It's an error getting that body part that
---- propogates down to this part of the code.

I first verified this works, and sent my initial (more verbose) email to the group, and attached a patch with all the debug stuff and a hack of a fix. That temp fix was to do the following:

In pith/smime.c, get_part_contents, where it calls "detach", I changed
it from:

err = detach(ps_global->mail_stream, msgno, (char*) section, 0L,
&len, pc, NULL, 0L);

to:
if(strcmp((char*) section, (char*) "1.1") == 0) {
dprint((9, "JOSH: get_part_contents - forcing section to 1
from 1.1 for detach()"));
err = detach(ps_global->mail_stream, msgno, "1", 0L, &len, pc,
NULL, 0L);
}
else {
dprint((9, "JOSH: get_part_contents - using default section
for detach()"));
err = detach(ps_global->mail_stream, msgno, (char*) section,
0L, &len, pc, NULL, 0L);
}

That worked for all types of messages I have on hand (normal, signed, encrypted, signed-then-encrypted).

I found a more-appropriate place to put the fix, and that's included in the attached patch.

In smime.c, do_decoding is only called from do_fiddle_smime_message, and only in one place after it checks that the body "is_pkcs7_body". The loop over parts just recursively calls do_fiddle_smime_message.

When a signed-then-encrypted message is it, it's first decrypted fine, and replaces the message body with a type=TYPEMULTIPART and subtype of "x-pkcs7-enclosure". When do_fiddle_smime_message goes past that part, it sees the message is multipart, but then does not hit the MIME_MULTI_SIGNED part (because subpart is not "signed"), and does not hit MIME_MSG (because it's MULTIPART), so then it loops over the parts.

Looping over the parts, it must increment the section (cause they're separate parts), so that has to stay there.

When it calls do_decoding the first time, the "section" is not defined. When it tries to call "get_pkcs7_from_part", it must pass a section, or it gets no results. However, on the second call, the previously mentioned loop is passing the partNum. If that get's fiddled with in do_decoding, then get_pkcs7_from_part will fail.

So, replace this (in do_decoding):

snprintf(newSec, sizeof(newSec), "%s%s1", section ? section : "", (section && *section) ? "." : "");
p7 = get_pkcs7_from_part(msgno, newSec);
if(!p7){

With this:

p7 = get_pkcs7_from_part(msgno, (section && *section) ? section : "1");
if(!p7){

That'll pass along the section if it's defined, and otherwise (Top part) sets it to 1.

[Joshua Miller]

Patch for S/MIME

[Andraž Levstik]

Patch applied.