Bug #72579 for Net-DNS-SEC: Net-DNS-SEC bug in sub 'verify' of keyset module

Mon Nov 21 10:16:25 2011 shmulik.bibi [...] gmail.com - Ticket created

CC:	shmulik.bibi [...] gmail.com
Subject:	Net-DNS-SEC bug in sub 'verify' of keyset module
Date:	Mon, 21 Nov 2011 17:16:14 +0200
To:	bug-Net-DNS-SEC [...] rt.cpan.org
From:	שמוליק ביבי <shmulik.bibi [...] gmail.com>

Hello, First of all i wanted to say thank you for contributing to the open source community. I appreciate your effort and time. I've seem to have found a bug in the "Net::DNS::Keyset" module, in the "verify" subroutine. The bug is related to having 2 keys with an identical keytag and name (it is legitimate according to RFC). I saw it when creating a new "Net::DNS::Keyset" object from packet, but i guess it's related to any scenario in which the RRSIG is validated. I'll try to describe the bug and the suggested fix, hopefully it will be clear enough (English is not my native language, sorry). *Module version:* 0.16 (latest ATM). *Keyset revision:* 728 *OS:* WIndows XP (not related to the bug though). *Perl version:* 5.10.1 (not related to the bug though). The simplified flow of "verify" (only the part relevant to the bug) is: - Foreach Key: - Foreach RRSIG: - Check if this RRSIG keytag and signame matches the current key's keytag and name. - If yes - verify the signature, and return an error if it failed. <------ Here is the bug ... The problem is that it's possible that there will be 2 keys with the same keytag and name on the DNSKEY packet. As a result this code can find the first matching RRSIG, that was signed by the second key, and fail (instead of continuing to the correct RRSIG). One possible fix is to continue to loop on all RRSIG until a *verified* match is found, and return an error only if none of the RRSIG matched. I attached the fix I wrote and tested for this function, however note that I'm no expert, so don't expect a perfect code :) I added it just in case it will help somehow. Also note that I changed the code a bit regardless, since I needed to print more info on errors. the basic flow I used is: - Foreach Key: - Foreach RRSIG: - Check if this RRSIG keytag and signame matches the current key's keytag and name. - If yes - verify the signature. - If it verifies raise the flag that says we've found a match, and stop looping on RRSIGs. - If it fails save the error aside and continue to the next RRSIG. - If no RRSIG found (flag is zero) set an error and return 0. ... Please note that I'm reporting this bug as a gratitude to your work, and in order to save other people the time debugging this. I'm not in need for a fix so please don't rush yourself because of me. If you need any more info please don't hesitate to contact me. Thank you for your kind work, Shmulik

Message body is not shown because sender requested not to inline it.

Wed Nov 23 10:24:08 2011 willem [...] cpan.org - Correspondence added

Hi Shmulik, You are absolutely correct. For completeness here is the section from rfc4035 (start of page 29): " It is possible for more than one DNSKEY RR to match the conditions above. In this case, the validator cannot predetermine which DNSKEY RR to use to authenticate the signature, and it MUST try each matching DNSKEY RR until either the signature is validated or the validator has run out of matching public keys to try. " Just out of interest, did you somehow create the keytag ``collision'' on purpose, or did it befell you? Your code looks just fine, but it introduces a bit more complexity in the control flow (via $rrsigFound). I think the most readable modification would be to just remove the explicit early function departure when a keyid is defined and it matches the keytag. I have that modification committed to the repository. Thank you for your contribution. It is much appreciated. -- Willem

Wed Nov 23 10:24:09 2011 The RT System itself - Status changed from 'new' to 'open'

Wed Nov 23 10:27:50 2011 willem [...] cpan.org - Status changed from 'open' to 'resolved'

Tue Apr 17 10:23:08 2012 shmulik.bibi [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #72579] Net-DNS-SEC bug in sub 'verify' of keyset module
Date:	Tue, 17 Apr 2012 17:22:54 +0300
To:	bug-Net-DNS-SEC [...] rt.cpan.org
From:	שמוליק ביבי <shmulik.bibi [...] gmail.com>

Hi Willem, Please forgive me for not responding earlier. I didn't create the collision on purpose, I just bumped into it while using this module to simulate a dnssec client against a dnssec server I wanted to test. Thank you for the fix, Shmulik On Wed, Nov 23, 2011 at 6:24 PM, Willem Toorop via RT < bug-Net-DNS-SEC@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=72579 > > > Hi Shmulik, > > You are absolutely correct. For completeness here is the section from > rfc4035 (start of page 29): > > " It is possible for more than one DNSKEY RR to match the conditions > above. In this case, the validator cannot predetermine which DNSKEY > RR to use to authenticate the signature, and it MUST try each > matching DNSKEY RR until either the signature is validated or the > validator has run out of matching public keys to try. " > > Just out of interest, did you somehow create the keytag ``collision'' on > purpose, or did it befell you? > > Your code looks just fine, but it introduces a bit more complexity in > the control flow (via $rrsigFound). I think the most readable > modification would be to just remove the explicit early function > departure when a keyid is defined and it matches the keytag. > > I have that modification committed to the repository. > > Thank you for your contribution. It is much appreciated. > > -- Willem >

Tue Apr 17 10:23:09 2012 The RT System itself - Status changed from 'resolved' to 'open'

Wed May 02 05:19:23 2012 NLNETLABS [...] cpan.org - Correspondence added

On Tue 17 Apr 2012 10:23:08, shmulik.bibi@gmail.com wrote: Show quoted text

> Thank you for the fix,

You're welcome. Thank you for reporting.

Wed May 02 05:19:25 2012 NLNETLABS [...] cpan.org - Status changed from 'open' to 'resolved'