Uninitialized value usage in ./mailnews/base/src/nsMsgDBView.cpp

RESOLVED FIXED in Thunderbird 20.0

Status

defect
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: ishikawa, Assigned: ishikawa)

Tracking

(Blocks 1 bug, {sec-high})

Thunderbird 20.0
All
Linux
Dependency tree / graph
Bug Flags:
sec-bounty +

Thunderbird Tracking Flags

(thunderbird18+ fixed, thunderbird19+ fixed, thunderbird20 fixed, thunderbird-esr1018+ fixed, thunderbird-esr1718+ fixed)

Details

(Whiteboard: [adv-main18+][adv-esr17+][adv-esr10+])

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

7 years ago
Uninitialized value usage in ./mailnews/base/src/nsMsgDBView.cpp

Found by valgrind run. See bug 803816 about mozmill run of TB.

Source version:
comm-central thunderbird.
$ hg identify
1016cef82fd8+ tip
ishikawa@debian-vm:~/TB-NEW/TB-3HG/new-src$ cd mozilla
ishikawa@debian-vm:~/TB-NEW/TB-3HG/new-src/mozilla$ hg identify
a517f7ea5bef+ tip

Valgrind log

==30518== Conditional jump or move depends on uninitialised value(s)
==30518==    at 0x5B8F829: nsMsgDBView::GetInsertIndexHelper(nsIMsgDBHdr*, nsTArray<unsigned int, nsTArrayDefaultAllocator>&, nsCOMArray<nsIMsgFolder>*, int, int) (nsMsgDBView.cpp:5227)
==30518==    by 0x5B908A1: nsMsgDBView::GetInsertIndex(nsIMsgDBHdr*) (nsMsgDBView.cpp:5301)
==30518==    by 0x5B9D103: nsMsgSearchDBView::InsertHdrFromFolder(nsIMsgDBHdr*, nsIMsgFolder*) (nsMsgSearchDBView.cpp:655)
==30518==    by 0x5B9DDCD: nsMsgSearchDBView::OnSearchHit(nsIMsgDBHdr*, nsIMsgFolder*) (nsMsgSearchDBView.cpp:691)
==30518==    by 0x5BCC450: nsMsgSearchSession::AddSearchHit(nsIMsgDBHdr*, nsIMsgFolder*) (nsMsgSearchSession.cpp:549)
==30518==    by 0x5BC5EBC: nsMsgSearchOfflineMail::AddResultElement(nsIMsgDBHdr*) (nsMsgLocalSearch.cpp:788)
==30518==    by 0x5BC7D24: nsMsgSearchOfflineMail::Search(bool*) (nsMsgLocalSearch.cpp:741)
==30518==    by 0x5BC49A1: nsMsgSearchScopeTerm::TimeSlice(bool*) (nsMsgSearchTerm.cpp:1881)
==30518==    by 0x5766379: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:3132)
==30518==    by 0x576DC97: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1488)
==30518==    by 0x66D2880: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:364)
==30518==    by 0x66C4414: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2369)
==30518==    by 0x66D18AC: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:324)
==30518==    by 0x66D296F: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:379)
==30518==    by 0x66D3037: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==30518==    by 0x6712C1A: js::IndirectProxyHandler::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jsproxy.cpp:456)
==30518==    by 0x67820EB: js::DirectWrapper::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jswrapper.cpp:390)
==30518==    by 0x6784837: js::CrossCompartmentWrapper::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jswrapper.cpp:722)
==30518==    by 0x67184C3: js::Proxy::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jsproxy.cpp:2478)
==30518==    by 0x671851A: proxy_Call(JSContext*, unsigned int, JS::Value*) (jsproxy.cpp:3038)
==30518==    by 0x66D2AB8: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:364)
==30518==    by 0x66C4414: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2369)
==30518==    by 0x66D18AC: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:324)
==30518==    by 0x66D296F: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:379)
==30518==    by 0x66D3037: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==30518==    by 0x6712C1A: js::IndirectProxyHandler::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jsproxy.cpp:456)
==30518==    by 0x67820EB: js::DirectWrapper::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jswrapper.cpp:390)
==30518==    by 0x6784837: js::CrossCompartmentWrapper::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jswrapper.cpp:722)
==30518==    by 0x67184C3: js::Proxy::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jsproxy.cpp:2478)
==30518==    by 0x671851A: proxy_Call(JSContext*, unsigned int, JS::Value*) (jsproxy.cpp:3038)
==30518==    by 0x66D2AB8: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:364)
==30518==    by 0x66C4414: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2369)
==30518==    by 0x66D18AC: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:324)
==30518==    by 0x66D296F: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:379)
==30518==    by 0x66D3037: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==30518==    by 0x6649E05: JS_CallFunctionValue (jsapi.cpp:5901)
==30518==    by 0x538E6B2: nsJSContext::CallEventHandler(nsISupports*, JSObject*, JSObject*, nsIArray*, nsIVariant**) (nsJSEnvironment.cpp:1914)
==30518==    by 0x542F123: nsJSEventListener::HandleEvent(nsIDOMEvent*) (nsJSEventListener.cpp:212)
==30518==    by 0x51DC9B6: nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsIDOMEventTarget*, unsigned int, nsCxPusher*) (nsEventListenerManager.cpp:868)
==30518==    by 0x51DD09A: nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) (nsEventListenerManager.cpp:941)
==30518==    by 0x51FF9A0: nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*, bool, nsCxPusher*) (nsEventListenerManager.h:144)
==30518==    by 0x137161F7: ???
==30518==  Uninitialised value was created by a stack allocation
==30518==    at 0x5B8F520: nsMsgDBView::GetInsertIndexHelper(nsIMsgDBHdr*, nsTArray<unsigned int, nsTArrayDefaultAllocator>&, nsCOMArray<nsIMsgFolder>*, int, int) (nsMsgDBView.cpp:5198)
==30518==



Observation:

Local variable fieldType is not initialized, but is supposed to be set
by GetFieldTypeAndLenForSort();

However, checking GetFieldTypeAndLenForSort() in the same file reveals
that there is an error path in which the third argument is not set and
the function returns NS_ERROR_UNEXPECTED.

In the code below quoted from nsMsgDBView.cpp, the return value is not
checked at all, and thus the third argument fieldType is undefined,
but unnoticed. So the uninitialized fieldType is referenced in the
switch() statement that follows.

Clearly something is wrong here.
  

 5195	nsMsgViewIndex nsMsgDBView::GetInsertIndexHelper(nsIMsgDBHdr *msgHdr, nsTArray<nsMsgKey> &keys,
 5196	                                                 nsCOMArray<nsIMsgFolder> *folders,
 5197	                                                 nsMsgViewSortOrderValue sortOrder, nsMsgViewSortTypeValue sortType)
 5198	{
 5199	  nsMsgViewIndex highIndex = keys.Length();
 5200	  nsMsgViewIndex lowIndex = 0;
 5201	  IdKeyPtr EntryInfo1, EntryInfo2;
 5202	  EntryInfo1.key = nullptr;
 5203	  EntryInfo2.key = nullptr;
 5204	
 5205	  nsresult rv;
 5206	  uint16_t  maxLen;
 5207	  eFieldType fieldType;
 5208	  rv = GetFieldTypeAndLenForSort(sortType, &maxLen, &fieldType);
 5209	  const void *pValue1 = &EntryInfo1, *pValue2 = &EntryInfo2;
 5210	
 5211	  int (* comparisonFun) (const void *pItem1, const void *pItem2, void *privateData) = nullptr;
 5212	  int retStatus = 0;
 5213	  msgHdr->GetMessageKey(&EntryInfo1.id);
 5214	  msgHdr->GetFolder(&EntryInfo1.folder);
 5215	  EntryInfo1.folder->Release();
 5216	  //check if a custom column handler exists. If it does then grab it and pass it in
 5217	  //to either GetCollationKey or GetLongField
 5218	  nsIMsgCustomColumnHandler* colHandler = GetCurColumnHandlerFromDBInfo();
 5219	
 5220	  viewSortInfo comparisonContext;
 5221	  comparisonContext.view = this;
 5222	  comparisonContext.isSecondarySort = false;
 5223	  comparisonContext.ascendingSort = (sortOrder == nsMsgViewSortOrder::ascending);
 5224	  rv = EntryInfo1.folder->GetMsgDatabase(&comparisonContext.db);
 5225	  NS_ENSURE_SUCCESS(rv, highIndex);
 5226	  comparisonContext.db->Release();
 5227	  switch (fieldType)
 5228	  {


The full log is in bug 803816.

(There could be a security implication since use of uninitialized variable led to many exploits in other software packages before. So I am setting Security status just in case. Please reset it is fixed or is not quite security-related.)

Comment 1

7 years ago
Strange, I've done a sweep on unchecked rv, I wonder why gcc didn't report this one (line 5208).

I think Ischikawa can fix this sort of stuff, this just needs a decision if it is OK to bailout on failure (NS_ENSURE_SUCCESS(rv)) in this function.
Assignee

Comment 2

7 years ago
Thank you for the comment.
I have no idea if it is OK or not (since NS_ERROR_UNEXPECTED sounds a little
difficult to deal with easily), but
several lines below, I see the following bailout for a different 
error:
line 5258:
NS_ASSERTION(NS_SUCCEEDED(rv),"failed to create collation key");

I will adopt this as 
NS_ASSERTION(NS_SUCCEEDED(rv), "failed to obtain field type");
just below the line where rv is set and see how it goes.

TIA

Comment 3

7 years ago
Actually I think line 5225 (NS_ENSURE_SUCCESS(rv, highIndex);) would be the pattern to copy here. But you'd have to see what returning "highIndex" means for the callers.
I think you can't report any nsresult values like (NS_ERROR_UNEXPECTED) as the function is defined to return nsMsgViewIndex .
It sounds like you are working on this, Ishikawa, so I'll assign it to you.

That sounds kind of sketchy, so I'm going to set sec-high. Feel free to adjust it if you think it should be changed.
https://wiki.mozilla.org/Security_Severity_Ratings
Assignee: nobody → ishikawa
Blocks: 803816
Keywords: sec-high
Assignee

Comment 5

7 years ago
I created a patch to take care of the particular problematic place, but then
I realized that similar usage without checking is found in a few places in the same file.

Unfortunately, the processing that follows the call to the function, GetFieldTypeAndLenForSort()  is not quite uniform.

To begin with switch() is used to select the processing in some places where as series of if() is used to select the processing (and forgets about the undefined case!).

I tried to return the reasonable value immediately if that is possible.

But in one place, it is not entirely clear what we should do.
So I bomb out.

But, I can certainly say that the change does not break the ordinary cases, and
this patch eliminated the undefined value usage warning from valgrind log.

The case where GetFieldTypeAndLenForSort() returns error value must be
really an error situation that requires an early bail out.

I think someone familiar with the code should check for the bomb-out case,.

All I can say that the patched code is much safer than the current code is, and I am more comfortable to run a safer code than some code that runs with uninitialized variable (which can mean it is non-deterministic from the ordinary 
user's point of view.)

TIA
Attachment #679645 - Flags: review?(continuation)
Comment on attachment 679645 [details] [diff] [review]
A proposed patch to take care of the undefined value usage issue.

Sorry, I'm not actually familiar with Thunderbird code. :)
Attachment #679645 - Flags: review?(continuation) → review?(mbanner)
Comment on attachment 679645 [details] [diff] [review]
A proposed patch to take care of the undefined value usage issue.

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

Thanks for taking this on.

I don't think we need to add the kUnknown alternative to the enum; by my reading, it works to just take the early exit any time a call to GetFieldTypeAndLenForSort() fails.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +3833,5 @@
> +//
> +// TODO/FIXME:
> +// There is a case when pFieldType is not set, and the function
> +// returns NS_ERROR_UNEXPECTED;
> +//

Keep the body of the comment, but remove the TODO/FIXME.

@@ +4291,1 @@
>    rv = GetFieldTypeAndLenForSort(sortType, &maxLen, &fieldType);

NS_ENSURE_SUCCESS(rv, 0); and update the comment

@@ +4469,5 @@
>      // could be a problem here if the ones that appear here are different than the ones already in the array
>      uint32_t actualFieldLen = 0;
> +
> +    // TODO/FIXME 
> +    // fieldType 

We exited early (by NS_ENSURE_SUCCESS) if fieldType was not set, so I don't think we need this comment

@@ +5794,3 @@
>    rv = GetFieldTypeAndLenForSort(m_sortType, &maxLen, &fieldType);
> +  // if we fail to obtain fieldType here, the default in switch
> +  // below should catch it. (It will result in error.)

This is debug code, so just fail by assertion like you do for ValidateSort(), and remove the TODO/FIXME line from the comment

@@ +5835,5 @@
> +  // TODO/FIXME
> +  // It is not entirely clear what we should do since
> +  // if fieldType is not available, there is no way to know
> +  // how to compare the field to check for sorting.
> +  // So we bomb out for now. 

Trailing white space. Since this is debug code, excluded by #ifdef, failing by ASSERTION is fine.

@@ +5836,5 @@
> +  // It is not entirely clear what we should do since
> +  // if fieldType is not available, there is no way to know
> +  // how to compare the field to check for sorting.
> +  // So we bomb out for now. 
> +  rv = GetFieldTypeAndLenForSort(m_sortType, &maxLen, &fieldType);

Need to declare nsresult rv?

@@ +5837,5 @@
> +  // if fieldType is not available, there is no way to know
> +  // how to compare the field to check for sorting.
> +  // So we bomb out for now. 
> +  rv = GetFieldTypeAndLenForSort(m_sortType, &maxLen, &fieldType);
> +  NS_ASSERTION(NS_SUCCEEDED(rv),"failed to obtain fieldType");      

Trailing white space

::: mailnews/base/src/nsMsgDBView.h
@@ +39,5 @@
>  
>  enum eFieldType {
>      kCollationKey,
> +    kU32,
> +    kUnknown                    // error case

Don't need to add this
Attachment #679645 - Flags: review?(mbanner)
Attachment #679645 - Flags: review?(continuation)
Attachment #679645 - Flags: feedback-
Attachment #679645 - Flags: review?(continuation)
One more thing - it would be nice to have an NS_WARNING that tells us what case caused us to return NS_ERROR_UNEXPECTED, so we have a chance of catching latent bugs where people add values to the enum without adding cases to the switch.

Apparently bugzilla let me stomp on mccr8's re-assignment of the reviewer; sorry about that. Feel free to assign review to either me or :standard8 when you respond to my feedback.
Assignee

Comment 9

7 years ago
(In reply to Irving Reid (:irving) from comment #8)
> One more thing - it would be nice to have an NS_WARNING that tells us what
> case caused us to return NS_ERROR_UNEXPECTED, so we have a chance of
> catching latent bugs where people add values to the enum without adding
> cases to the switch.

OK, I will upload a modified patch tomorrow.
I am afraid that my diff got a little mixed up about the undeclared rv. I thought I recreated a new diff after a successful compilation, 
but seems to obtain a copy from a stale file and used it :-(

Actualy, I am not that familiar with NS_WARNING, but I take that
I can put this statement next to where we bomb out (or
where we return by failure in GetFieldTypeAndLenForSort()?)

> Apparently bugzilla let me stomp on mccr8's re-assignment of the reviewer;
> sorry about that. Feel free to assign review to either me or :standard8 when
> you respond to my feedback.
Will do.
http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#1728 has an example of building up a message and passing it to NS_ERROR(); NS_WARNING() works the same but doesn't trigger a breakpoint for people debugging the code. I'd put it right before the "return NS_ERROR_UNEXPECTED" in GetFieldTypeAndLenForSort().
(In reply to Irving Reid (:irving) from comment #8)
> Apparently bugzilla let me stomp on mccr8's re-assignment of the reviewer;
> sorry about that.
It is no problem, I just wanted somebody who knew this code to look at the patch, and it looks like that has happened. :)
Assignee

Comment 12

7 years ago
Posted patch rev 02 of proposed patch (obsolete) — Splinter Review
Tried to incoporate the comment on the first patch.

A note: when I tried to insert NS_WARNING before return NS_ERROR_UNEXPECTED, I realized a grave error/problem or oversight in the original code. (Maybe it is a never-happens case.)
When colHandler is null and *pFieldType is NOT set, the function
GetFieldTypeAndLenForSort() returns NS_OK (!)
So I put another NS_WARNING there.

Header file was restored to the original.

Now, regarding trailing whitespace at the end of a line. I use emacs, and it has "delete-trailing-space" function to remove such extra whitespace characters.

I ran delete-trailing-space. Voila, it removed several other unnoticed trailing whitespaces, it seems. So you see seemingly unnecessary changes. They are because the removal of trailing whitespaces.

One other thing. I let the test run of make mozmill run overnight.
Unfortunately, this happens sometimes, but depending on the load on my PC,
the operation under valgrind takes a looong time, and
the timeout function in the testing harness kicks in.
This happened last time, and I am afraid that the particular test which
caused the printing of the original error got aborted early and
so the printing from NS_WARNING is not found in the latest log.

I left another test run running, and hopefully (depending on the phase of the Moon, etc.), the particular test would run to completion and the warning is printed there.
Attachment #679934 - Flags: review?(irving)
Comment on attachment 679934 [details] [diff] [review]
rev 02 of proposed patch

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

r=irving after you add the error return I suggest. Thank you for doing this valgrind testing, it is difficult but very helpful.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +3895,5 @@
>                *pMaxLen = 0;
>              }
>            }
> +          else
> +            NS_WARNING("colHandler is null. *pFieldType is not set.");

We should return NS_ERROR_NULL_POINTER here
Attachment #679934 - Flags: review?(irving) → review+
Assignee

Comment 14

7 years ago
(In reply to Irving Reid (:irving) from comment #13)
> r=irving after you add the error return I suggest. Thank you for doing this
> valgrind testing, it is difficult but very helpful.
 
You are welcome.

> ::: mailnews/base/src/nsMsgDBView.cpp
> @@ +3895,5 @@
> >                *pMaxLen = 0;
> >              }
> >            }
> > +          else
> > +            NS_WARNING("colHandler is null. *pFieldType is not set.");
> 
> We should return NS_ERROR_NULL_POINTER here

I re-created the patch and am uploading it in the next post.
(If the patch doesn't apply cleanly, please let me know. By my manual operation mistake, I may have currupted my source tree, but I don't have the time to clean everything and download the whole source tree and start over...(
Assignee

Comment 15

7 years ago
Posted patch rev 03 of proposed patch (obsolete) — Splinter Review
>>            }
>> +          else
>> +            NS_WARNING("colHandler is null. *pFieldType is not set.");
>
>We should return NS_ERROR_NULL_POINTER here

Addressed the above issue.

TIA
Attachment #683973 - Flags: review?(irving)
Comment on attachment 683973 [details] [diff] [review]
rev 03 of proposed patch

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

Looks good, compiles clean. You can carry forward r=irving from this comment when you fix the white space, no need to request another review.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +3833,5 @@
> +//
> +// There are cases when pFieldType is not set:
> +// one case returns NS_ERROR_UNEXPECTED;
> +// the other case now return NS_ERROR_NULL_POINTER (this is only when
> +// colhandler below is null, but is very unlikely). 

Trailing white space
Attachment #683973 - Flags: review?(irving) → review+
Assignee

Comment 17

7 years ago
The final patch taking care of the trailing blank issue.
(I am not sure what to do now. Mozilla web page suggests that I add commit-needed keyword somewhere, but uploading page does not allow that operation?)
The keyword is checkin-needed. Remove it if you aren't yet ready for checkin. :)
Assignee

Comment 19

7 years ago
(In reply to Andrew McCreight [:mccr8] from comment #18)
> The keyword is checkin-needed. Remove it if you aren't yet ready for
> checkin. :)

Thanks!
This is rated sec-high (and AFAICT, it affects more than just tb20), which means that the patch requires security approval to land, correct?
Attachment #679645 - Attachment is obsolete: true
Attachment #679934 - Attachment is obsolete: true
Attachment #683973 - Attachment is obsolete: true
(In reply to Ryan VanderMeulen from comment #20)
> This is rated sec-high (and AFAICT, it affects more than just tb20), which
> means that the patch requires security approval to land, correct?

Irving can you do that , too much question I can't answer.
The security level was set because of uninitialized memory use, and nobody has done the analysis to determine whether the problem is exploitable or not. I can't speak to what review is necessary; I'll leave that decision to Standard8.
Comment on attachment 684029 [details] [diff] [review]
rev 04 of proposed patch

Note: I'm not altogether convinced this is a sec-high issue. There are some cases where we could go down random branches in the code due to this, but I suspect that in general all the default cases are handled, and realistically we'd only hit this in an add-on situation (though I've not checked all these routes myself, so there may be something internally to TB that could affect this still).

[Security approval request comment]
How easily can the security issue be deduced from the patch? Quite easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The comments would help.

Which older supported branches are affected by this flaw? Probably all including 10.0.x.

If not all supported branches, which bug introduced the flaw? N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? No, small differences probably just to account for type name changes & a few other things.

How likely is this patch to cause regressions; how much testing does it need? Its possible, could do with a bit of time on beta before pushing to release.
Attachment #684029 - Flags: sec-approval?
Comment on attachment 684029 [details] [diff] [review]
rev 04 of proposed patch

sec-approval+.
Attachment #684029 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/comm-central/rev/d8c4fc70e423
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
Comment on attachment 684029 [details] [diff] [review]
rev 04 of proposed patch

[Triage Comment]
Approving for aurora/beta now so that we can get this out for testing in the next beta. Flagging for 17 as well. Looks like we'll need a back-ported patch for ESR 10.
Attachment #684029 - Flags: approval-comm-esr17?
Attachment #684029 - Flags: approval-comm-beta+
Attachment #684029 - Flags: approval-comm-aurora+
Attachment #684029 - Flags: approval-comm-esr17? → approval-comm-esr17+
(In reply to Mark Banner (:standard8) from comment #28)
> https://hg.mozilla.org/releases/comm-esr17/rev/3cc88679f0b7

And a minor bustage fix due to the recent nsCAutoString -> nsAutoCString transition:

https://hg.mozilla.org/releases/comm-esr17/rev/c96629272e7f
This is the fixed up version for ESR 10. There were no functional changes in the patch between 10 & 17, just bitrot issues.
Attachment #697392 - Flags: approval-comm-esr10+
Did this make it into the current ESR builds for next week?
Whiteboard: [adv-main18+][adv-esr17+][adv-esr10+]
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Assignee

Comment 35

7 years ago
(In reply to Al Billings [:abillings] from comment #32)
> Did this make it into the current ESR builds for next week?

Hi,

I got a notice from Chris Hoffmann that I have won the security bounty award(!) for reporting and fixing the problem.

Without Gary's designating this bugzilla entry as a security issue, I have never thought of being part of the security bounty efforts.

I would like to thank everybody  who pitched in to fix this bug as well as Gary.

Now, I have a request. Can I take a look at the draft of the announcement that you will put regarding the security issues: I am curious how my name will appear
in it and might want to ask a few modifications if possible.

TIA and what a surprise to receive a bounty in new year.
In Japan, we have a practice of giving a monetary allowance during new year holidays to children (like a Christmas gift in North America, but usually given as some amount of money so that children can go out and spend it as they see fit. Buy a toy, buy some sweet, use it to go to amusement park, save it for the future like buying an expensive book later, etc. I used to buy plastic models like Tamiya's when I was young.).
Receiving the bounty at this time of the year is like receiving this new year's monetary allowance, called "Otoshidama" in
Japan. [Except that I have been on the giver's side for a quite some time now :-) ]

Thank you again.
 This is for bug 809064, which was fixed in the release that went live yesterday. The bug was mentioned in the roll up advisory at http://www.mozilla.org/security/announce/2013/mfsa2013-01.html. It did not get a separate advisory, but was included in the "Miscellaneous memory safety hazards" advisory.

Updated

6 years ago
Component: General → Backend
Product: Thunderbird → MailNews Core

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.