1132358 - possible use after free in nsDNSRecord::GetNextAddr

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect)

Description

[Henry Hu]

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.

Comment 1

[Henry Hu]

Oh, I mean nsDNSRecord::HasMore(), not HasNext()

Comment 2

[Dragana Damjanovic [:dragana]]

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?

Comment 3

[Henry Hu]

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.

Comment 4

[Henry Hu]

Attachment: backup and restore 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 5

[Dragana Damjanovic [:dragana]]

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)

Comment 6

[Patrick McManus [: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

Comment 7

[Patrick McManus [:mcmanus]]

should probably backport this to aurora/beta

Comment 8

[Dragana Damjanovic [:dragana]]

Do you want to fix this patch?

Comment 9

[Henry Hu]

Attachment: 2nd version, generated with "hg diff"

This patch is generated by "hg diff" against the mozilla-central tree.

Comment 10

[Dragana Damjanovic [:dragana]]

Here are some information about submitting a patch:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 11

[Henry Hu]

Attachment: 3rd version of the patch, save and restore mIterGenCnt

Here is a new version of the patch, generated according to 
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 12

[Dragana Damjanovic [:dragana]]

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.

Comment 13

[Dragana Damjanovic [:dragana]]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 15

[Patrick McManus [:mcmanus]]

(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 :)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/56e00c7bfa7e

Comment 18

[Dragana Damjanovic [:dragana]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/9bbcf4fd73ca

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/386a573b4fc0

Not sure if we want this on esr31 or not.

Comment 22

[Daniel Veditz [:dveditz]]

Given topcrash status we do want it on ESR.

Comment 23

[Robert Kaiser]

[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.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

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?

Comment 25

[Al Billings [:abillings - ex-MoCo]]

Fine by me.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8566404 [details] [diff] [review]
3rd version of the patch, save and restore mIterGenCnt

OK, thanks. I am taking it then.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8566404 [details] [diff] [review]
3rd version of the patch, save and restore mIterGenCnt

See comment 18 and later for discussion.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/386a573b4fc0

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/6a3e3165c377

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8566404 [details] [diff] [review]
3rd version of the patch, save and restore mIterGenCnt

This got promoted to sec-high.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e1055be6944e
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/e6eabddd46af

Comment 33

[Patrick McManus [:mcmanus]]

should uaf generally be sec-high? or is it due to intersection with crash-stats? (for my own info)

Comment 34

[Sylvestre Ledru [:Sylvestre]]

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..

Comment 35

[Daniel Veditz [:dveditz]]

(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.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/9bbcf4fd73ca
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/e1055be6944e