911396 - (comm-central) TB uses uninitialized data when a date header line contains bogus data.

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: A patch to initialize the data object in question in advance.

When a bogus date is injected on the date line by a (malicious) sender
and the message is retrieved by thunderbird, such e-mail message ends
up being shown as having the time stamp of 1970 Jan 1 00:00
(origin of epoch-time in POSIX-like system)

The date is being shown as such, but beneath the processing
uninitialized memory area is accessed.

valgrind showed this.
(This is recorded on Debian GNU/Linux amd64 (64bits) version.


==24253== Conditional jump or move depends on uninitialised value(s)
==24253==    at 0x40760BE: PR_NormalizeTime (prtime.c:370)
==24253==    by 0x40763E8: PR_ImplodeTime (prtime.c:241)
==24253==    by 0x88510AA: nsMimeBaseEmitter::GenerateDateString(char const*, nsACString_internal&, bool) (nsMimeBaseEmitter.cpp:712)
==24253==    by 0x8854F99: nsMimeHtmlDisplayEmitter::BroadcastHeaders(nsIMsgHeaderSink*, int, bool) (nsMimeHtmlEmitter.cpp:224)
==24253==    by 0x8855793: nsMimeHtmlDisplayEmitter::WriteHTMLHeaders(nsACString_internal const&) (nsMimeHtmlEmitter.cpp:278)
==24253==    by 0x8852D3F: nsMimeHtmlDisplayEmitter::EndHeader(nsACString_internal const&) (nsMimeHtmlEmitter.cpp:316)
==24253==    by 0x88355FB: mimeEmitterEndHeader (mimemoz2.cpp:1968)
==24253==    by 0x8839801: MimeMessage_parse_line(char const*, int, MimeObject*) (mimemsg.cpp:801)
==24253==    by 0x8812464: convert_and_send_buffer(char*, int, bool, int (*)(char*, unsigned int, void*), void*) (mimebuf.cpp:152)
==24253==    by 0x8812775: mime_LineBuffer (mimebuf.cpp:240)
==24253==    by 0x883D646: MimeObject_parse_buffer(char const*, int, MimeObject*) (mimeobj.cpp:246)
==24253==    by 0x8830F7D: mime_display_stream_write (mimemoz2.cpp:986)
==24253==    by 0x884BA15: nsStreamConverter::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (nsStreamConverter.cpp:939)
==24253==    by 0x7F6B9E7: nsDocumentOpenInfo::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (nsURILoader.cpp:302)
==24253==    by 0x8580CEB: nsMailboxProtocol::ReadMessageResponse(nsIInputStream*, unsigned long, unsigned int) (nsMailboxProtocol.cpp:599)
==24253==    by 0x85811A8: nsMailboxProtocol::ProcessProtocolState(nsIURI*, nsIInputStream*, unsigned long, unsigned int) (nsMailboxProtocol.cpp:684)
==24253==    by 0x843B308: nsMsgProtocol::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (nsMsgProtocol.cpp:352)
==24253==    by 0x6CE8736: nsInputStreamPump::OnStateTransfer() (nsInputStreamPump.cpp:529)
==24253==    by 0x6CE9558: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (nsInputStreamPump.cpp:391)
==24253==    by 0x93E3A95: nsInputStreamReadyEvent::Run() (nsStreamUtils.cpp:82)
==24253==    by 0x940293C: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:622)
==24253==    by 0x9393215: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:238)
==24253==    by 0x8947662: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (MessagePump.cpp:81)
==24253==    by 0x948030D: MessageLoop::RunInternal() (message_loop.cc:220)
==24253==    by 0x9480658: MessageLoop::Run() (message_loop.cc:213)
==24253==    by 0x83498AE: nsBaseAppShell::Run() (nsBaseAppShell.cpp:161)
==24253==    by 0x803A1F1: nsAppStartup::Run() (nsAppStartup.cpp:269)
==24253==    by 0x6B95B69: XREMain::XRE_mainRun() (nsAppRunner.cpp:3871)
==24253==    by 0x6B974CA: XREMain::XRE_main(int, char**, nsXREAppData const*) (nsAppRunner.cpp:3939)
==24253==    by 0x6B9786B: XRE_main (nsAppRunner.cpp:4141)
==24253==    by 0x40368F: main (nsMailApp.cpp:111)
==24253==  Uninitialised value was created by a stack allocation
==24253==    at 0x8850BEF: nsMimeBaseEmitter::GenerateDateString(char const*, nsACString_internal&, bool) (nsMimeBaseEmitter.cpp:665)
==24253== 

I think somwhhere below, due to bogus data string,
PR_ParseTimeStringToExplodedTime() left a few fields of
explodedMsgTime uninitialized.

  PRExplodedTime explodedMsgTime;
  // XXX Casting PRStatus to nsresult
  rv = static_cast<nsresult>(
    PR_ParseTimeStringToExplodedTime(dateString, false, &explodedMsgTime));
  /**
   * To determine the date format to use, comparison of current and message
   * time has to be made. If displaying in local time, both timestamps have
   * to be in local time. If displaying in senders time zone, leave the compare
   * time in that time zone.
   * Otherwise in TZ+0100 on 2009-03-12 a message from 2009-03-11T20:49-0700
   * would be displayed as "20:49 -0700" though it in fact is not from the
   * same day.
   */
  PRExplodedTime explodedCompTime;
  if (displaySenderTimezone)
    explodedCompTime = explodedMsgTime;
  else
    PR_ExplodeTime(PR_ImplodeTime(&explodedMsgTime), PR_LocalTimeParameters, &explodedCompTime);



A simple patch attached.

TIA

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

I'm no longer a Thunderbird code reviewer; see <https://wiki.mozilla.org/Modules/Thunderbird> for some other choices.

Comment 2

[ISHIKAWA, Chiaki]

(In reply to Dan Mosedale (:dmose) from comment #1)
> I'm no longer a Thunderbird code reviewer; see
> <https://wiki.mozilla.org/Modules/Thunderbird> for some other choices.

Thank you for the pointer, I put in
[e-mail] as the potential reviewer.

(Incidentally, what do you think of my patch of zeroing out the data?)

Comment 3

[[e-mail]]

Comment on attachment 798084 [details] [diff] [review]
A patch to initialize the data object in question in advance.

>   PRExplodedTime explodedMsgTime;
>   // XXX Casting PRStatus to nsresult
>   rv = static_cast<nsresult>(
>     PR_ParseTimeStringToExplodedTime(dateString, false, &explodedMsgTime));
...
>   PRExplodedTime explodedCompTime;
>   if (displaySenderTimezone)
>     explodedCompTime = explodedMsgTime;
>   else
>     PR_ExplodeTime(PR_ImplodeTime(&explodedMsgTime), PR_LocalTimeParameters, &explodedCompTime);
...
>   if (NS_SUCCEEDED(rv))
>   {

So, the problem here is that we use explodedMsgTime here before checking for a successful return from PR_ParseTimeStringToExplodedTime.

I don't think it's worth moving the existing check earlier, instead we should just return a failure code if PR_ParseTimeStringToExplodedTime fails.

Bonus points for not storing the result of PR_ParseTimeStringToExplodedTime in an nsresult variable (because it's not an nsresult, it's a PRStatus).

Comment 4

[ISHIKAWA, Chiaki]

(In reply to [e-mail] from comment #3)
> Comment on attachment 798084 [details] [diff] [review]
> A patch to initialize the data object in question in advance.
> 
> >   PRExplodedTime explodedMsgTime;
> >   // XXX Casting PRStatus to nsresult
> >   rv = static_cast<nsresult>(
> >     PR_ParseTimeStringToExplodedTime(dateString, false, &explodedMsgTime));
> ...
> >   PRExplodedTime explodedCompTime;
> >   if (displaySenderTimezone)
> >     explodedCompTime = explodedMsgTime;
> >   else
> >     PR_ExplodeTime(PR_ImplodeTime(&explodedMsgTime), PR_LocalTimeParameters, &explodedCompTime);
> ...
> >   if (NS_SUCCEEDED(rv))
> >   {
> 
> So, the problem here is that we use explodedMsgTime here before checking for
> a successful return from PR_ParseTimeStringToExplodedTime.
> 
> I don't think it's worth moving the existing check earlier, instead we
> should just return a failure code if PR_ParseTimeStringToExplodedTime fails.
> 
> Bonus points for not storing the result of PR_ParseTimeStringToExplodedTime
> in an nsresult variable (because it's not an nsresult, it's a PRStatus).

OK, fair enough. However, there is this tiny detail.

What error value should I return? (any non-zero, i.e. a value ( != NS_OK)
will do.

Or should I define something like NS_INVALID_DATE in
mailnews/base/public/msgCore.h and use it?

TIA

Comment 5

[Magnus Melin [:mkmelin]]

(In reply to ISHIKAWA, Chiaki from comment #4)

> What error value should I return? (any non-zero, i.e. a value ( != NS_OK)
> will do.

I think NS_ERROR_FAILURE is probably fine

Comment 6

[:aceman]

Chiaki, this one looks like it could be finished according to the reviewer's comments.

Comment 7

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #6)
> Chiaki, this one looks like it could be finished according to the reviewer's
> comments.

Thank you for the reminder. I will update the patch and upload it very soon now.

Comment 8

[ISHIKAWA, Chiaki]

Attachment: A patch to initialize the data object in question in advance and return early when

> I don't think it's worth moving the existing check earlier, instead we should 
> just return a failure code if PR_ParseTimeStringToExplodedTime fails.
>
> Bonus points for not storing the result of PR_ParseTimeStringToExplodedTime in 
> an nsresult variable (because it's not an nsresult, it's a PRStatus).

Uploading a patch that initializes the data object in question in advance
also addresses the comments above.

I took out one check for rv value |if(...)| and took out unnecessary "{", "}" and re-indented the code there. The diff result looks a little strange.

TIA

Comment 9

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, Chiaki from comment #8)
> Created attachment 8794470 [details] [diff] [review]
> A patch to initialize the data object in question in advance and return
> early when

Oops. This patch breaks compilation under Windows. It seems bzero() is not declared in MSVC.
I will rewrite the patch with memset().

Comment 10

[ISHIKAWA, Chiaki]

Attachment: A patch to initialize the data object in question in advance and return early when |PR_ParseTimeStringToExplodedTime(...)| fails.

(In reply to ISHIKAWA, Chiaki from comment #9)
> (In reply to ISHIKAWA, Chiaki from comment #8)
> > Created attachment 8794470 [details] [diff] [review]
> > A patch to initialize the data object in question in advance and return
> > early when
> 
> Oops. This patch breaks compilation under Windows. It seems bzero() is not
> declared in MSVC.
> I will rewrite the patch with memset().

Using memset(), it compiles on all major platforms (linux, OSX, windows),
try-comm-central submission compiles.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fc9cd96af18df36aa7d0123556e577c1a14d5b8d

I am testing so many patches at the same time, but this patch is in
https://hg.mozilla.org/try-comm-central/rev/753170a2d033

TIA

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 8794531 [details] [diff] [review]
A patch to initialize the data object in question in advance and return early when  |PR_ParseTimeStringToExplodedTime(...)| fails.

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

Please fix the commit message to include bug number
"Bug 911396 - ...."

Looks ok to me, r=mkmelin

::: mailnews/mime/emitters/nsMimeBaseEmitter.cpp
@@ +690,5 @@
>  
>    PRExplodedTime explodedMsgTime;
> +
> +  /* Bogus date string may leave some fields uninitialized, so take precaution.
> +   */

Please refrain from using /* */ comments . 
// style comments are preferred so commenting out blocks for testing and such is easier

Comment 12

[ISHIKAWA, Chiaki]

Attachment: A patch to initialize the data object in question in advance and return early when |PR_ParseTimeStringToExplodedTime(...)| fails. carrying over r=mkmelin+

Thank you for review+.

I took care of the following comments.

(In reply to Magnus Melin from comment #11)
> Comment on attachment 8794531 [details] [diff] [review]
> A patch to initialize the data object in question in advance and return
> early when  |PR_ParseTimeStringToExplodedTime(...)| fails.
> 
> Review of attachment 8794531 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please fix the commit message to include bug number
> "Bug 911396 - ...."

Done. 

> Looks ok to me, r=mkmelin
> 
> ::: mailnews/mime/emitters/nsMimeBaseEmitter.cpp
> @@ +690,5 @@
> >  
> >    PRExplodedTime explodedMsgTime;
> > +
> > +  /* Bogus date string may leave some fields uninitialized, so take precaution.
> > +   */
> 
> Please refrain from using /* */ comments . 
> // style comments are preferred so commenting out blocks for testing and
> such is easier

OK. Done.

TIA

Comment 13

[ISHIKAWA, Chiaki]

I put the checkin-needed keyword.

Comment 14

[:aceman]

Did you upload the old patch? The new one does not have the fixes.

Comment 15

[ISHIKAWA, Chiaki]

Attachment: A patch to initialize the data object in question in advance and return early when |PR_ParseTimeStringToExplodedTime(...)| fails. carrying over r=mkmelin+ (correct patch, previous one was wrong.)

(In reply to :aceman from comment #14)
> Did you upload the old patch? The new one does not have the fixes.

You are right. I uploaded the old patch.
Here is the new one that addresses the comments!

Comment 16

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/33a78eaabf22d2fa35cf8b60039621faf4771964

Please remember: The commit message should read:
 ... r=mkmelin, see comment #11.

Comment 17

[ISHIKAWA, Chiaki]

(In reply to Jorg K (GMT+2) from comment #16)
> https://hg.mozilla.org/comm-central/rev/
> 33a78eaabf22d2fa35cf8b60039621faf4771964
> 
> Please remember: The commit message should read:
>  ... r=mkmelin, see comment #11.

Thank you for fixing that!