From: Michal 'vorner' Vaner Date: Thu, 20 Sep 2012 09:27:57 +0000 (+0200) Subject: [1357] Commit only after verifying the last TSIG X-Git-Tag: trac2351_base~49^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7feae4486ab8b722cdf897bcf629391e7c15fcf1;p=thirdparty%2Fkea.git [1357] Commit only after verifying the last TSIG We can't commit during the stream of changes, they might be not signed and that'd be a security risk (if someone stole the TCP connection and pushed a change that'd put bogus data inside before getting to the last message which would be rejected then). Fixed the check: it now happens on the last message, not on each message except the last. Returning true there does not mean to continue, but that the current RR has been handled fully. --- diff --git a/src/bin/xfrin/tests/xfrin_test.py b/src/bin/xfrin/tests/xfrin_test.py index 4ee36b797b..399c7a489c 100644 --- a/src/bin/xfrin/tests/xfrin_test.py +++ b/src/bin/xfrin/tests/xfrin_test.py @@ -571,10 +571,18 @@ class TestXfrinIXFRAdd(TestXfrinState): self.conn._diff.add_data(self.ns_rrset) # put some dummy change self.conn._tsig_ctx = MockTSIGContext(TSIG_KEY) self.conn._tsig_ctx.last_has_signature = lambda: False + # First, push a starting SOA inside. This should be OK, nothing checked + # yet. + self.state.handle_rr(self.conn, self.begin_soa) + end_soa_rdata = Rdata(RRType.SOA(), TEST_RRCLASS, + 'm. r. 1234 0 0 0 0') + end_soa_rrset = RRset(TEST_ZONE_NAME, TEST_RRCLASS, RRType.SOA(), + RRTTL(3600)) + end_soa_rrset.add_rdata(end_soa_rdata) # This would try to finish up. But the TSIG pretends not everything is # signed, rejecting it. self.assertRaises(xfrin.XfrinProtocolError, self.state.handle_rr, - self.conn, self.begin_soa) + self.conn, end_soa_rrset) def test_handle_out_of_sync(self): # getting SOA with an inconsistent serial. This is an error. @@ -1577,16 +1585,18 @@ class TestIXFRResponse(TestXfrinConnection): self.conn._handle_xfrin_responses() self.assertEqual(type(XfrinIXFREnd()), type(self.conn.get_xfrstate())) self.assertEqual([], self.conn._datasrc_client.diffs) + # Everything is committed as one bunch, currently we commit at the very + # end. check_diffs(self.assertEqual, [[('delete', begin_soa_rrset), ('delete', self._create_a('192.0.2.1')), ('add', self._create_soa('1231')), - ('add', self._create_a('192.0.2.2'))], - [('delete', self._create_soa('1231')), + ('add', self._create_a('192.0.2.2')), + ('delete', self._create_soa('1231')), ('delete', self._create_a('192.0.2.3')), ('add', self._create_soa('1232')), - ('add', self._create_a('192.0.2.4'))], - [('delete', self._create_soa('1232')), + ('add', self._create_a('192.0.2.4')), + ('delete', self._create_soa('1232')), ('delete', self._create_a('192.0.2.5')), ('add', soa_rrset), ('add', self._create_a('192.0.2.6'))]], diff --git a/src/bin/xfrin/xfrin.py.in b/src/bin/xfrin/xfrin.py.in index 2613307402..88411ed7ac 100755 --- a/src/bin/xfrin/xfrin.py.in +++ b/src/bin/xfrin/xfrin.py.in @@ -420,9 +420,9 @@ class XfrinIXFRAdd(XfrinState): conn.get_transfer_stats().ixfr_changeset_count += 1 soa_serial = get_soa_serial(rr.get_rdata()[0]) if soa_serial == conn._end_serial: - # FIXME: It's insecure to do a full commit here, due - # to the fact that the message we're working on might not - # be signed right now. + # The final part is there. Check all was signed + # and commit it to the database. + conn._check_response_tsig_last() conn._diff.commit() self.set_xfrstate(conn, XfrinIXFREnd()) return True @@ -432,8 +432,10 @@ class XfrinIXFRAdd(XfrinState): str(conn._current_serial) + ', got ' + str(soa_serial)) else: - conn._check_response_tsig_last() - conn._diff.commit() + # Apply a change to the database. But don't commit it yet, + # we can't know if the message is/will be properly signed. + # A complete commit will happen after the last bit. + conn._diff.apply() self.set_xfrstate(conn, XfrinIXFRDeleteSOA()) return False conn._diff.add_data(rr)