Closed Bug 1609607 (CVE-2020-6792) Opened 2 years ago Closed 2 years ago

C-C TB mochitest+valgrind uncovered uninitialized memory access.: MD5 is calculated accessing uninitialized area.

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(thunderbird_esr68+ fixed, thunderbird73 fixed)

RESOLVED FIXED
Thunderbird 74.0
Tracking Status
thunderbird_esr68 + fixed
thunderbird73 --- fixed

People

(Reporter: ishikawa, Assigned: ishikawa)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uninitialized, sec-low)

Attachments

(5 files)

+++ This bug was initially created as a clone of Bug #1608539 +++

That bug is about the following.
--- BEGIN QUOTE ---
The subject says it all
|m_headerstartpos| is not always initialized in nsParseMailMessageState::FinalizeHeaders()

The unitinitialized reference happens at
https://searchfox.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#1223

I found this by running xpcshell-tests under valgrind.

If m_headerstartpos is not properly initialized, literally anything goes.
Header parsing, etc. get all screwed up.

The symptom of strange display of message in a message pane that shows only blank or in the middle of header, etc. could be at least partially explained by this bug. (Maybe not all the cases.)

I detected the issue using valgrind, and then began dumping m_headerstartpos after I set

export MALLOC_CHECK_=3
export MALLOC_PERTURB_=0x5A

in my environment. GNU libc malloc fills the uninitialized malloc'ed area with 0xA5.
Thus it is rather easy to find it in an otherwise normal dump.

cf. From man mallopt.

       M_PERTURB (since glibc 2.4)
              If this parameter is set to a nonzero value, then bytes of allocated memory  (other  than  alloca‐
              tions  via calloc(3)) are initialized to the complement of the value in the least significant byte
              of value, and when allocated memory is released using free(3), the freed  bytes  are  set  to  the
              least  significant  byte  of value.  This can be useful for detecting errors where programs incor‐
              rectly rely on allocated memory being initialized to zero, or reuse values in memory that has  al‐
              ready been freed.
   ...
       MALLOC_PERTURB_
              Controls the same parameter as mallopt() M_PERTURB.

--- END QUOTE ---

From the comment 2 of that bug.
--- BEGIN QUOTE ---
Oh, BTW, I notice that the hash calculation touches uninitialized memory region. The last hunk in the attached diff is for investigating this.
I knew that for a long time. But I let it pass since it is not unusual for some hash functions to
calculate the value using 4 or 8 octets at a time, and thus if we have interesting data which has a length of not multiple of 4, we may end up
accessing the last few uninitialized octets of a word: this happens in zip/unzip operation during |make mozmill| test
and I have suppressed the warning from valgrind after investigation revealed that it is a non-issue.

However, with this uninitialized memory access issue at hand, I investigated a bit more, and I now realized that uninitialized access during hash calcluation MAY end up generating message-ID for a message which does not have it to be attached a non-unique message ID.: that is, the message ID varies each time it is generated because of random data in uninitiailized memory. (Correct me if I am wrong here.)

This may happen when you import a message in an individual file and the message lacks a message ID.
If you include it in multiple folders, the same message will have a different message ID in different folders. So this may not be optimal.
But the practical impact of having the message with different message ID is not that great if I am not mistaken again.

HOWEVER, during investigation, I have noticed a glaring uninitialized memory creation and access in the code which I will report later. I am overwhelmed at what is going on and not sure where the problem begins, even.

--- END QUOTE ---

The code to dump the buffer passed to MD5 creation.
You can see that the hash is calculated using |m_header.GetSize()| as length, but the buffer contains way too much uninitialized garbage at the end...

Typical dump from xpcshell-tests.:

 0:35.12 TEST_START: comm/mailnews/base/test/unit/test_headerFoldingInDatabase.js

  0:35.42 pid:214785 {debug} Dump:tmp_buf: totalsize = 1024 
 0:35.42 pid:214785 0x00:   F  r  o  m     -     S  a  t     A  p  r     1 
 0:35.42 pid:214785 0x10:   4     1  4  :  0  1  :  3  0     2  0  1  8 NU 
 0:35.42 pid:214785 0x20:  LF  D  a  t  e  :     S  a  t  ,     1  4     A 
 0:35.42 pid:214785 0x30:   p  r     2  0  1  8     1  4  :  0  1  :  3  0 
 0:35.42 pid:214785 0x40:      +  0  2  0  0 NU LF  S  u  b  j  e  c  t  : 
 0:35.42 pid:214785 0x50:      B  a  d  l  y     f  o  l  d  e  d     h  e 
 0:35.42 pid:214785 0x60:   a  d  e  r  s  ,     o  n  e     l  i  n  e    
 0:35.42 pid:214785 0x70:   w  i  t  h           s  p  a  c  e           b 
 0:35.42 pid:214785 0x80:   e  t  w  e  e  n           T  o     a  n  d    
 0:35.42 pid:214785 0x90:   F  r  o  m NU NU  p  a  c  e           b  e  t 
 0:35.42 pid:214785 0xA0:   w  e  e  n           T  o     a  n  d     F  r 
 0:35.42 pid:214785 0xB0:   o  m CR LF    NU LF  T  o  :     "  R  e  c  i 
 0:35.42 pid:214785 0xC0:   p  i  e  n  t        w  i  t  h        s  p  a 
 0:35.42 pid:214785 0xD0:   c  e  s  "     <  r  e  c  i  p  i  e  n  t  @ 
 0:35.42 pid:214785 0xE0:   e  x  a  m  p  l  e  .  c  o  m  > NU NU    NU 
 0:35.42 pid:214785 0xF0:  LF  F  r  o  m  :     s  e  n  d  e  r  @  e  x 
 0:35.42 pid:214785 0x00:   a  m  p  l  e  .  c  o  m NU LF a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x10:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x20:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x30:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x40:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x50:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x60:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x70:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x80:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x90:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xA0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xB0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xC0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xD0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xE0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xF0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x00:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x10:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x20:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x30:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x40:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x50:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x60:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x70:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x80:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x90:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xA0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xB0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xC0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xD0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xE0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xF0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x00:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x10:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x20:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x30:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x40:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x50:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x60:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x70:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x80:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0x90:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xA0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xB0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xC0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xD0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xE0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
 0:35.42 pid:214785 0xF0:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 


Assignee: nobody → ishikawa

Note that there are cases where header line's CRLF or LF is completely substituted by NUL (in my dump NUL is printed as NU).

 0:02.86 TEST_START: comm/mailnews/compose/test/unit/test_autoReply.js 
...
 0:03.21 INFO comm/mailnews/compose/test/unit/test_autoReply.js | Starting copy_gIncomingMailFile 
...
 0:03.22 INFO comm/mailnews/compose/test/unit/test_autoReply.js | Starting copy_gIncomingMailFile3 
...
 0:03.23 pid:215945 {debug} Dump:tmp_buf: totalsize = 1024 
 0:03.23 pid:215945 0x00:   F  r  o  m     -     S  u  n     A  p  r     2 
 0:03.23 pid:215945 0x10:   6     2  1  :  3  0  :  5  9     2  0  1  5 NU 
 0:03.23 pid:215945 0x20:   S  u  b  j  e  c  t  :     m  a  i  l     w  i 
 0:03.23 pid:215945 0x30:   t  h     n  o     f  r  o  m NU  T  o  :     f 
 0:03.23 pid:215945 0x40:   r  o  m  @  f  o  o  .  i  n  v  a  l  i  d NU 
 0:03.23 pid:215945 0x50:   C  o  n  t  e  n  t  -  T  y  p  e  :     t  e 
 0:03.23 pid:215945 0x60:   x  t  /  p  l  a  i  n  ;     c  h  a  r  s  e 
 0:03.23 pid:215945 0x70:   t  =  "  U  T  F  -  8  " NU  M  I  M  E  -  V 
 0:03.23 pid:215945 0x80:   e  r  s  i  o  n  :     1  .  0 NU a5 a5 a5 a5 
 0:03.23 pid:215945 0x90:  a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 a5 
...

I thought the whole log from xpcshell-tests could be seen in the try-comm-central job.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=285106307

However, I realized that, in order to get the local log, I had to run the xpcshell-tests with --verbose option since the stderr/stdout output is not shown otherwise AND
serially (not in parallel execution mode) so that the dump is not cluttered up with parallel execution of various tests.
So the dump is not visible in the above job. (BTW, xpcshell-tests log with "--verbose" is so full of suspicious errors/warnings. If we can remove some of them, TB would be more robust, but I digress.)

The dump is close to 60MB. If anyone really wants to see the log, I can upload it somewhere and post a link here.

BTW, it seems that this MD5 calculation is invoked for missing message-ID header case to generate message ID.
This means that A MALICIOUS MAIL SERVER CAN STRIP THE MESSAGE HEADER AND CAUSE THIS UNINITIALIZED MEMORY ACCESS DURING MD5 calculation on TB client. Not sure that could be a security problem albeit non-optimal situation where the same duplicate message (same headers) would end up with different message IDs due to uninitialized garbage input to MD5.

The previous bug in 1608539 of uninitialized m_headerstartpos is worse since the header area manipulation (and the resulting data stored in DB) can be completely bogus although how that is triggered is not clear yet.

Anyway, the area size passed to MD5 calculation is way too large.

From bug 1608539 comment 8 : Slightly edited
I think instead of passing GetSize() of buffer area that stores the header data to md5->Update routine to calculate the MD5 and to generate a missing message ID, we should probably keep track of the ending position (NUL code at the end of the last header line when the headers are parsed and stored.: NEWLINE, CR LF seem to be replaced with NUL in the code. Well, CR LF seems to be replaced with NUL LF ?) and pass the length from the m_headerstartpos to that position.
However, as the forgotten initialization of m_headestartpos shows there seem to a can of worms around here.

The following command identifies the files that trigger the problem.

egrep "(TEST_START|{debug} Dump:tmp_buf: totalsize = |nrun-xpcshell-original.sh|Failed: 1| a5|a5a5a5a5|\| Starting)" log

The attached file is the output.

Kai: does sec-moderate seem right?

Flags: needinfo?(kaie)

I think I found the cause.
We should use GetBufferPos() instead of GetSize() when we pass the valid length of the buffer data.
With this I could eliminate the access to uninitialized memory.

However, I have noticed an anomaly in the log.
There are cases when the header buffer ends with a NUL and no NEWLINE at the end.
Majority of the cases seem to end with NEWLINE.
It seems that the code does not uniformly handle the input data that ends either

  • LF, or
  • CR LF
  • missing the above line-ending sequence at all (?).

I think all the above cases should generate the same MD5 signature and thus the same message ID, but
I am running out of steam. Eliminating the uninitialized memory access is the main target for now.

I have verified my local patch works as far as xpcshell-tests is concerned.

For some reason, I have not been able to run mochitest locally (under Debian GNU/Linux amd64).
Bug 1608532

To verify the patch, I need to run the mochitest somehow.
However, I am not sure if I should submit the patch to try-comm-central since the patch and the log will be visible to anybody.
Is there a way to restrict the access to the job submission and log because of security concern?

This is the local patch I am testing. This passes xpcshell-tests.

I ran xpcshell-tests serially and with --verbose so that the log files are easy to parse (no output from parallel run will get mixed up.).
The command is as follows.:

export MALLOC_CHECK_=3
export MALLOC_PERTURB_=0x5A

TESTDIRS="
comm/chat/components/src/test
comm/chat/modules/test
comm/chat/protocols/irc/test
comm/chat/protocols/skype/test
comm/chat/protocols/xmpp/test
comm/common/saxparser/test/unit
comm/common/test/xpcshell
comm/ldap/xpcom/tests/unit
comm/mail/base/test/unit
comm/mail/components/enterprisepolicies/tests/xpcshell
comm/mail/components/extensions/test/xpcshell
comm/mail/components/test/unit
comm/mailnews/addrbook/test/unit
comm/mailnews/base/test/unit
comm/mailnews/compose/test/unit
comm/mailnews/db/gloda/test/unit
comm/mailnews/db/msgdb/test/unit
comm/mailnews/extensions/bayesian-spam-filter/test/unit
comm/mailnews/extensions/mdn/test/unit
comm/mailnews/extensions/newsblog/test/unit
comm/mailnews/imap/test/unit
comm/mailnews/import/test/unit
comm/mailnews/intl/test/unit
comm/mailnews/jsaccount/test/unit
comm/mailnews/local/test/unit
comm/mailnews/mime/jsmime/test
comm/mailnews/mime/test/unit
comm/mailnews/news/test/unit
"
for dir in $TESTDIRS
do
    bash -vx ~ishikawa/bin/nrun-xpcshell-original.sh --sequential --verbose ${dir}
done

Then I ran the following command on the log to extra the attached file.

egrep "(Failed: |  a5|buffer area|totalsize =|\| Starting|TEST_START)" log1161-xpcshell.txt

You can see that there is no longer an uninitialized memory access (the dumped memory area did not contain the telltale a5.
Also, you may notice that there ARE buffer areas that end in a NUL and not LF as is usually the case.

Oh well, while testing, I already submitted a try-comm-central job, :-(
And honestly speaking, I think big underground organizations and/or nation-state parties would have known the problem for a LOOONG time.

I don't think there's a way to submit "hidden" jobs. But, probably not too much to worry about here - without context someone looking would still have to do a lot of work to take advantage of it.

Sounds like it's best to clean up the patch, and get it reviewed and committed.

Status: NEW → ASSIGNED

(In reply to Magnus Melin [:mkmelin] from comment #10)

I don't think there's a way to submit "hidden" jobs. But, probably not too much to worry about here - without context someone looking would still have to do a lot of work to take advantage of it.

Sounds like it's best to clean up the patch, and get it reviewed and committed.

Will do.

(In reply to Daniel Veditz [:dveditz] from comment #5)

Kai: does sec-moderate seem right?

sec-low seems sufficient.

Flags: needinfo?(kaie)
Keywords: sec-moderatesec-low

(In reply to ISHIKAWA, Chiaki (may be slow to respond until Jan 4.) from comment #11)

(In reply to Magnus Melin [:mkmelin] from comment #10)

I don't think there's a way to submit "hidden" jobs. But, probably not too much to worry about here - without context someone looking would still have to do a lot of work to take advantage of it.

Sounds like it's best to clean up the patch, and get it reviewed and committed.

Will do.

I wanted to test the patch locally but got bitten with bug 1609789 locally which should have been solved by the source tree update.
Once I could run at least xpcshell-tests without ill-effect then I will submit a try server job and ask for an approval.
This patch will include the |m_headerstartpos = 0| from Bug #1608539.

(In reply to ISHIKAWA, Chiaki (may be slow to respond until Jan 4.) from comment #6)

We should use GetBufferPos() instead of GetSize() when we pass the valid length of the buffer data.

Hello Chiaki, thanks a lot for your detective work and finding the cause of this bug!

In bug 1608539 you had included a suggested fix for this bug. You suggested that the buffer should be copied to avoid using garbage. I think that shouldn't be necessary.

I think that simply changing the call should be all we need. The has function will only use the amount of bytes that are passed as a parameter, and ignore anything else in the buffer after that number of bytes.

Attached patch 1609607-v2.patchSplinter Review
Attachment #9122922 - Flags: review?(mkmelin+mozilla)
Attachment #9122922 - Flags: review?(mkmelin+mozilla) → review+
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 74.0
Comment on attachment 9122922 [details] [diff] [review]
1609607-v2.patch

[Approval Request Comment]
User impact if declined: random data used to calculate message id

Although this code reads random data, the data is simply used to calculate a message id. The fix ensures we use only real message properties, making the id calculation stable.

It doesn't seem urgent to merge to stable esr68. It's more of a correctness fix. Letting it bake for a while seems fine.
Attachment #9122922 - Flags: approval-comm-beta?
Attachment #9122922 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 9122922 [details] [diff] [review]
1609607-v2.patch

On the other hand, while not urgent, it's a really obvious fix. I suggest to merge it to esr68, so we can get it off the radar of things we need to track.
Attachment #9122922 - Flags: approval-comm-esr68?
Group: mail-core-security → core-security-release
Attachment #9122922 - Flags: approval-comm-esr68? → approval-comm-esr68+

Rob, can you please land into comm-esr68? Thanks.

Flags: needinfo?(rob)
Flags: needinfo?(rob)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.