Opened 7 years ago

Closed 7 years ago

#2471 closed defect (fixed)

use after free in DNAME processing

Reported by: jinmei Owned by: jinmei
Priority: very high Milestone: Sprint-20121120
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Note the "Sensitive" mark.

There's a use-after-free bug in Query::process. In the DNAME
processing we do this:

            const rdata::generic::DNAME& dname(
                dynamic_cast<const rdata::generic::DNAME&>(
                db_context->rrset->getRdataIterator()->getCurrent()));

and use dname in the code below. But the rdata iterator is already
invalid at that point (the result of getCurrent() itself isn't due to
getting the reference, but it relies on the iterator contents), and
can cause strange behavior. If the RRset is the basic one, this
happens to be safe because it refers to the data stored in rrset,
but in the case of in-memory data source, the rdata iterator is a
completely temporary object, and it causes a real use-after-free
situation.

In the case of my personal server it crashed at this point:

            cname->addRdata(rdata::generic::CNAME(qname_->split(0,
                qname_->getLabelCount() -
                db_context->rrset->getName().getLabelCount()).
                concatenate(dname.getDname())));

The fix is easy: define the rdata iterator as a separate variable, and
keep it sufficiently long:

            RdataIteratorPtr rit = db_context->rrset->getRdataIterator();
            const rdata::generic::DNAME& dname(
                dynamic_cast<const rdata::generic::DNAME&>(rit->getCurrent()));

Note that this can be a remotely exploitable problem. I don't know
whether we want to handle it as a "security" matter (I've marked this
ticket as sensitive, but that's more for practice) but in any case I
believe this should be included in the next release.

Subtickets

Change History (8)

comment:1 Changed 7 years ago by jinmei

  • Milestone changed from New Tasks to Sprint-20121120

pushing to the current sprint as discussed on jabber.

comment:2 Changed 7 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:3 Changed 7 years ago by jinmei

I've committed the fix (with a test case) to a separate private
repository, which is ready for review. It can be cloned by

% git clone ssh://<username>@git.bind10.isc.org/home/jinmei/git/bind10-private

The branch name is sec-trac2471. It's small and should be easy to
understand.

While working on this branch I noticed we only tested the query logic
with a mock data source. I think we should extend the existing
QueryTest so we can use the test cases for the in-memory data source
as much as possible. But that should go to a separate task.

In this ticket I tried to minimize the size of the diff.

This is the proposed changelog entry.

502.?	[security]	jinmei
	Fixed a use-after-free case in handling DNAME record with the
	in-memory data source.  This could lead to a crash of b10-auth
	if it serves a zone containing a DNAME RR from the in-memory
	data source.  This bug was introduced at bind10-devel-20120927.
	(Trac #2471, git TBD)

I'm not sure about the change category, but tentatively specified
"security".

comment:4 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:5 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:6 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Changes look good.

Regarding changelog tag, not sure, do we want to mark them explicitely? (and if so, do we want to merge if we are not releasing just yet)?

Oh and btw, you don't need a separate clone to view the branch; you can either pull it directly, or you can set up your copy as an additional remote (git add remote jinmei-private ssh://<etc> is what I did)

comment:7 in reply to: ↑ 6 Changed 7 years ago by jinmei

Thanks for the review.

Replying to jelte:

Changes look good.

Regarding changelog tag, not sure, do we want to mark them explicitely? (and if so, do we want to merge if we are not releasing just yet)?

I think we should discuss these at the team call tomorrow. I'll
complete this ticket based on the result of it.

Oh and btw, you don't need a separate clone to view the branch; you can either pull it directly, or you can set up your copy as an additional remote (git add remote jinmei-private ssh://<etc> is what I did)

Ah, okay, thanks for the tip.

comment:8 Changed 7 years ago by jreed

  • Resolution set to fixed
  • Sensitive unset
  • Status changed from reviewing to closed

This is public and merged into the master. I unchecked the sensitive checkbox. Also closing the ticket. The continuation of the tests is ticket #2480.

Note: See TracTickets for help on using tickets.