Closed Bug 1132358 Opened 5 years ago Closed 5 years ago

possible use after free in nsDNSRecord::GetNextAddr

Categories

(Core :: Networking: DNS, defect)

35 Branch
x86_64
FreeBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 + fixed
firefox37 --- fixed
firefox38 --- fixed
firefox-esr31 37+ fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed
relnote-firefox --- 36+

People

(Reporter: henry.hu.sh, Assigned: henry.hu.sh)

References

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main36+][adv-esr31.6+])

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; FreeBSD amd64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.93 Safari/537.36

Steps to reproduce:

This might be related to bug 955900.

First, check https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsHostResolver.cpp, line 1231-1238. In nsHostResolver::OnLookupComplete() , it says

        AddrInfo  *old_addr_info;
        {
            MutexAutoLock lock(rec->addr_info_lock);
            old_addr_info = rec->addr_info;
            rec->addr_info = result;
            rec->addr_info_gencnt++;
        }
        delete old_addr_info;

so, when addr_info is updated, addr_info_gencnt is increased. Later, old addr_info is deleted.

Next, check https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsDNSService2.cpp. In nsDNSRecord::GetNextAddr(), it says

    mHostRecord->addr_info_lock.Lock();
    if (mHostRecord->addr_info) {
        if (mIterGenCnt != mHostRecord->addr_info_gencnt) {
            // mHostRecord->addr_info has changed, restart the iteration.
            mIter = nullptr;
            mIterGenCnt = mHostRecord->addr_info_gencnt;
        }

        bool startedFresh = !mIter;

        do {
            if (!mIter) {
                mIter = mHostRecord->addr_info->mAddresses.getFirst();
            } else {
                mIter = mIter->getNext();
            }
        }
        while (mIter && mHostRecord->Blacklisted(&mIter->mAddress));

So, when mIterGenCnt != addr_info_gencnt, which means that mIter is invalid now, it starts again from the beginning, updating mIter and mIterGenCnt.
Next, check nsDNSRecord::HasNext(). It says

    NetAddrElement *iterCopy = mIter;

    NetAddr addr;
    *result = NS_SUCCEEDED(GetNextAddr(0, &addr));

    mIter = iterCopy;

It backs up mIter, run GetNextAddr(), and restores mIter.

Now here is the problem.
Consider the case that after mIter is backed up, nsHostResolver::onLookupComplete() is executed on another thread. It updates addr_info_gencnt and frees the old addr_info, invalidates all the old iterators. When GetNextAddr() is executed, it found that addr_info_gencnt is increased, and updates mIterGenCnt and mIter accordingly. However, when GetNextAddr() returns, mIter is restored to the old, invalid value, but mIterGenCnt is not restored.
Next time you execute GetNextAddr(), it would find that mIterGenCnt == addr_info_gencnt, and tries to use mIter directly. But since mIter is invalid, this would cause a use-after-free, and possibly crash the firefox.


Actual results:

firefox randomly crashes with SIGBUS. It crashes at the 
        while (mIter && mHostRecord->Blacklisted(&mIter->mAddress));
line, and mIter's value is 0x5a5a5a5a5a5a5a5a, which means that the source of the value is freed. 


Expected results:

it should not crash. mIterGenCnt also needs to be backed up and restored.
Oh, I mean nsDNSRecord::HasMore(), not HasNext()
Component: Untriaged → Networking: DNS
Product: Firefox → Core
Looks reasonable.
and it should be fixed.

but would not it crash already in mIter->GetNext().
This is probably the reason for some crashes in bug 1132358.

Do you want to submit a patch?
It may not crash in mIter->GetNext() because it just reads the memory mIter points to. Although the memory is freed, it's still accessible, and its content is garbage. When mIter is used next time, it causes a crash.
I will submit a patch which saves and restores mIterGenCnt also.
This patch backs up and restores mIterGenCnt.
If mIterGenCnt is not changed during the call, then nothing changed.
If mIterGenCnt increased, then restoring mIterGenCnt would restore it to the old value. In this case, mIter and mIterGenCnt are still in a consistent state. Next time when GetNext() is called, they would both be updated.
Comment on attachment 8563912 [details] [diff] [review]
backup and restore mIterGenCnt also

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

Looks good.

I will give it too Patrick for review too.

Patrick, can you take a look at this? It look sane to me( but I am not a necko peer)
Attachment #8563912 - Flags: review?(mcmanus)
Comment on attachment 8563912 [details] [diff] [review]
backup and restore mIterGenCnt also

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

thanks for the patch!

this patch applies to a *orig file. Please be careful about forming the correct patch before submitting
Attachment #8563912 - Flags: review?(mcmanus) → review+
should probably backport this to aurora/beta
Group: network-core-security
Group: core-security
Do you want to fix this patch?
Flags: needinfo?(henry.hu.sh)
This patch is generated by "hg diff" against the mozilla-central tree.
Attachment #8563912 - Attachment is obsolete: true
Flags: needinfo?(henry.hu.sh)
Looks good. Only you can add r=reviewer to the commit message (in you case you can put mcmanus).
I have pushed it to try  for you:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=334f57e8e53f
Try will take some time to run.

Try run looks good.
You can mark you patch as review+ (because Patrick already reviewed it and set review+).

After that you can ask for check-in: write in keyworks checkin and somebody will check it in for you.
Flags: needinfo?(henry.hu.sh)
Flags: needinfo?(henry.hu.sh)
Attachment #8566404 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/56e00c7bfa7e

Too late to get this on Fx36, but would still be useful to get this on older affected branches. How far back does this go?
Assignee: nobody → henry.hu.sh
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(mcmanus)
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #14)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/56e00c7bfa7e
> 
> Too late to get this on Fx36, but would still be useful to get this on older
> affected branches. How far back does this go?

much farther than we backport :)
Flags: needinfo?(mcmanus)
Being a sec-moderate, we can at least get it on to Aurora37 (or Beta if we miss Monday's uplift) and b2g34, so I'd say to go ahead and do those nominations. b2g30/b2g32/esr31 are less-likely, but feel free to nominate if you so choose.
https://hg.mozilla.org/mozilla-central/rev/56e00c7bfa7e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8566404 [details] [diff] [review]
3rd version of the patch, save and restore mIterGenCnt

Flags for uplift to Aurora 37 and  b2g34.

Approval Request Comment
[Feature/regressing bug #]:Not a regression. Already there for a long time.
[User impact if declined]:Can cause crash crashes for accessing freed memory. Probably this is the reason for some crashes from bug 955900.
[Describe test coverage new/current, TreeHerder]:none. Hard to reproduce.
[Risks and why]: None. After calling a function we want to roll back to the state before calling it, but a parameter has been forgotten and it has not been returned to the state before calling the function.
[String/UUID change made/needed]: none
Attachment #8566404 - Flags: approval-mozilla-b2g34?
Attachment #8566404 - Flags: approval-mozilla-aurora?
Attachment #8566404 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8566404 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Group: network-core-security
Comment on attachment 8566404 [details] [diff] [review]
3rd version of the patch, save and restore mIterGenCnt

Already in 38 for about a week and fixes a topcrash. Beta+
Attachment #8566404 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Given topcrash status we do want it on ESR.
Blocks: 955900
[Tracking Requested - why for this release]:
Given that this is a topcrash on 36 as well (#7 with 1.3% of the total volume), I wonder if we want to get it into 36.0.1 as well. That said, we may not want to ship a sec-high fix on release when we don't ship it on ESR at the same time, so we need to clear that up before we potentially take it on 36.
It landed on most of the branches for a week, so, I guess potential attackers are already aware of this. it didn't cause any regression... I would like to take it. Any crash fix is good.

Daniel, Al, ok with you?
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Fine by me.
Flags: needinfo?(abillings)
Comment on attachment 8566404 [details] [diff] [review]
3rd version of the patch, save and restore mIterGenCnt

OK, thanks. I am taking it then.
Flags: needinfo?(dveditz)
Attachment #8566404 - Flags: approval-mozilla-release+
https://hg.mozilla.org/releases/mozilla-release/rev/b20bd6385474

Bhavana, are you interested in taking this on b2g30/b2g32 after all? I'm not sure how to look up B2G crash stats to figure out how frequently this hits there.
Flags: needinfo?(bbajaj)
Comment on attachment 8566404 [details] [diff] [review]
3rd version of the patch, save and restore mIterGenCnt

See comment 18 and later for discussion.
Attachment #8566404 - Flags: approval-mozilla-esr31?
Attachment #8566404 - Flags: approval-mozilla-b2g32?
Attachment #8566404 - Flags: approval-mozilla-b2g30?
Attachment #8566404 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Comment on attachment 8566404 [details] [diff] [review]
3rd version of the patch, save and restore mIterGenCnt

This got promoted to sec-high.
Flags: needinfo?(bbajaj)
Attachment #8566404 - Flags: approval-mozilla-b2g32?
Attachment #8566404 - Flags: approval-mozilla-b2g30?
should uaf generally be sec-high? or is it due to intersection with crash-stats? (for my own info)
Flags: needinfo?(dveditz)
Added to the release notes "36.0.1 - Fix a top crash". I haven't added a link to this bug as it is a security issues..
(In reply to Patrick McManus [:mcmanus] from comment #33)
> should uaf generally be sec-high? or is it due to intersection with
> crash-stats? (for my own info)

All the recent Flash 0-day exploits, and at least one of the recent IE 0-day exploits, have been based on use-after-free vulnerabilities. You could even argue for sec-critical unless there's some kind of raciness or unpredictability in triggering the UAF. It's true that some UAFs are unlikely to be exploited, if say there's a deterministic path between the free and the re-use that's short enough for us to be reasonably confident nothing on another thread could reliably reallocate the reclaimed memory in a "useful" way. In general, however, it's more productive to simply assume any given UAF may be one of the exploitable ones.
Flags: needinfo?(dveditz)
Whiteboard: [adv-main36+][adv-esr31.6+]
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.