[PATCH] DNS TKEY/TSIG handling, bug 11520

[Jeremy Allison]

On Mon, Jun 13, 2016 at 10:19:59AM +0200, Ralph Boehme wrote:
> On Thu, Jun 09, 2016 at 11:29:16AM +0200, Ralph Boehme wrote:
> > On Sat, Jun 04, 2016 at 12:09:14AM +1200, garming at catalyst.net.nz wrote:
> > > I think that addresses everything on my end.
> > >
> > > You can put my review on the last patch.
> > 
> > attached is the final patchset with a minor cleanup in the test. The
> > change from the previously reviewed version is:
> > 
> > --- a/python/samba/tests/dns_tkey.py
> > +++ b/python/samba/tests/dns_tkey.py
> > @@ -365,10 +365,6 @@ class DNSTest(tests.TestCase):
> >          packet.additional = additional
> >          packet.arcount = 1
> >  
> > -    def remove_tsig_rec(self, packet):
> > -        packet.additional = []
> > -        packet.arcount = 0
> > -
> >      def search_record(self, name):
> >          p = self.make_name_packet(dns.DNS_OPCODE_QUERY)
> >          questions = []
> > @@ -472,7 +468,6 @@ class TestDNSUpdates(DNSTest):
> >          (response, response_p) = self.dns_transaction_udp(p,
> >          self.server_ip)
> >          self.assert_dns_rcode_equals(response, dns.DNS_RCODE_OK)
> >          self.verify_packet(response, response_p, mac)
> > -        self.remove_tsig_rec(p)
> >  
> >          # Check the record is around
> >          rcode = self.search_record(self.newrecname)
> > 
> > The call to remove_tsig_rec wasn't needed anymore and so the function
> > was obsolete.
> > 
> > Please review and push if ok. Thanks!
> 
> just in case this got lost: ping. :)

I'm OK with this, but there's one check I'd like to
add:

In:

s4/dns_server: include request MAC in TSIG response MAC calculation

we have:

-       buffer_len = packet_blob.length + tsig_blob.length;
+       if (state->tsig != NULL) {
+               mac_size = state->tsig->rdata.tsig_record.mac_size;
+       }
+       buffer_len = mac_size + packet_blob.length + tsig_blob.length;

As packet_blob.length is copied in -> out, I'd really like
to see some integer wrap checks on this. I know the original
code didn't have them, but as we modify code I'd like to
raise the bar on things.

Can you add checks like:

	buffer_len = mac_size;
	buffer_len += packet_blob.length;
	if (buffer_len < packet_blob.length) {
		return WERR_INVALID_PARAM
	}
	buffer_len += tsig_blob.length;
	if ((buffer_len < tsig_blob.length) {
		return WERR_INVALID_PARAM
	}

I'm happy for these to be added as an additional
fix after this has gone in though.

What do you think Ralph - am I being too paranoid
here ?

Cheers,

	Jeremy.