Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dyn dns update fix #13958

Merged
merged 3 commits into from
Aug 7, 2020
Merged

Dyn dns update fix #13958

merged 3 commits into from
Aug 7, 2020

Conversation

digininja
Copy link
Contributor

The resolve function used to always return true, regardless whether the record it was asked to resolve existed or not, this broke some onward calls when they tried to update things that didn't really exist.

Verification

List the steps needed to make sure this thing works

  • Try to update a record that doesn't exist:
msf6 auxiliary(admin/dns/dyn_dns_update) > set ACTION UPDATE
ACTION => UPDATE
msf6 auxiliary(admin/dns/dyn_dns_update) > set HOSTNAME new2
HOSTNAME => new2
msf6 auxiliary(admin/dns/dyn_dns_update) > run

[+] Did not find an existing TXT record for new2.digi.int
[*] Sending dynamic DNS add message...
[+] The record 'new2.digi.int => aa' has been added!
[*] Auxiliary module execution completed

It gets added

  • Now run it again and it will get updated
msf6 auxiliary(admin/dns/dyn_dns_update) > run

[+] Found existing TXT record for new2.digi.int
[*] Sending dynamic DNS delete message...
[+] The record 'new2.digi.int => aa' has been deleted!
[*] Sending dynamic DNS add message...
[+] The record 'new2.digi.int => aa' has been added!
[*] Auxiliary module execution completed
msf6 auxiliary(admin/dns/dyn_dns_update) > 

@cnotin
Copy link
Contributor

cnotin commented Aug 7, 2020

Psst looks like the branch also contains the commit of #13957 ;)

@digininja
Copy link
Contributor Author

Ye, I needed that fix to do this one.

Do I remove that and resubmit? I don't know the correct way to do these.

removed commit from other PR
@digininja
Copy link
Contributor Author

I've removed the lines from the other PR

@cnotin
Copy link
Contributor

cnotin commented Aug 7, 2020

That's one way to do it. Or you could have amended your commit and force pushed your PR branch. Github handles it fine and as long as no one has started to look into it it shouldn't bother.

@digininja
Copy link
Contributor Author

digininja commented Aug 7, 2020 via email

@cnotin
Copy link
Contributor

cnotin commented Aug 7, 2020

Yes if you want to add a commit to an existing PR you can indeed edit the file in your fork and in the branch 😉

@smcintyre-r7 smcintyre-r7 self-assigned this Aug 7, 2020
@smcintyre-r7
Copy link
Contributor

Well it looks like it would return false if a Dnsruby::ResolveError exception was raised, however these changes are still beneficial because they'll account for scenarios in which there's a response with no answers. I tested this against both my BIND and Microsoft DNS servers and that doesn't seem to be the case with either but it sounds reasonable enough that other DNS server implementations may exhibit this behavior.

I did confirm that the module functions as intended both before and after this update so I'm ready to merge it in thanks!

@smcintyre-r7 smcintyre-r7 merged commit 178ec83 into rapid7:master Aug 7, 2020
@smcintyre-r7
Copy link
Contributor

smcintyre-r7 commented Aug 7, 2020

Release Notes

Updated the auxiliary/admin/dns/dyn_dns_update module to properly identify that a record is missing when a valid DNS response is returned with no answers.

@digininja
Copy link
Contributor Author

digininja commented Aug 7, 2020 via email

@gwillcox-r7 gwillcox-r7 added bug module rn-fix release notes fix labels Aug 7, 2020
@digininja digininja deleted the dyn_dns_update_fix branch August 7, 2020 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug module rn-fix release notes fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants