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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dns resolution skipping over nameservers with valid responses #18662

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

dwelch-r7
Copy link
Contributor

@dwelch-r7 dwelch-r7 commented Jan 4, 2024

Continuation of #18660

Recommend turning whitespace changes off for this one https://github.com/rapid7/metasploit-framework/pull/18662/files?diff=unified&w=1

There have been a few issues reported with DNS resolution recently this PR aims to resolve (馃榿) those

Noticed a couple of issues:

  1. Only a single nameserver was ever being pulled out of /etc/resolv.conf with the last nameserver overwriting the previous one(s)
  2. Looping over the nameservers until we get a valid response was not correctly returning the answer from DNS instead moving on to the next nameserver until it ran out and saying the dns resolution was failing

@@ -1093,7 +1093,7 @@ def parse_config_file
when /^\s*search\s+(.*)/
self.searchlist = $1.split(" ")
when /^\s*nameserver\s+(.*)/
self.nameservers = $1.split(" ")
self.nameservers += $1.split(" ")
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the windows codepath get handled correctly? Probably worth a check after the unix-y path works 馃憖

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to this random issue report (I couldn't find a better example of what gets returned) it should be correct for windows
https://bugs.ruby-lang.org/issues/12604

but I'll be sure to verify this on windows myself too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a windows dev env set up too, let me know if you've got replication steps you want me to follow 馃憤

@dwelch-r7 dwelch-r7 marked this pull request as ready for review January 5, 2024 10:49
@dwelch-r7 dwelch-r7 changed the title [WIP] Fix dns resolution skipping over nameservers with valid responses Fix dns resolution skipping over nameservers with valid responses Jan 5, 2024
@adfoster-r7
Copy link
Contributor

adfoster-r7 commented Jan 8, 2024

Can we verify with the docs/schema that there isn't more edegcases to consider in the /etc/resolv.conf config format? 馃

Edit: Looks like there's some options that we could potentially support, but not needed in this PR

@dwelch-r7
Copy link
Contributor Author

Can we verify with the docs/schema that there isn't more edegcases to consider in the /etc/resolv.conf config format? 馃

Edit: Looks like there's some options that we could potentially support, but not needed in this PR

Forgot to reply to this earlier apologies
yea was looking through these docs, only thing that stands out like you said are the options
https://www.ibm.com/docs/en/aix/7.3?topic=formats-resolvconf-file-format-tcpip

@adfoster-r7
Copy link
Contributor

adfoster-r7 commented Jan 19, 2024

Not a blocker: This would be easier to test if there was a top level resolve command, similar to the one in Meterpreter 馃槃

@adfoster-r7
Copy link
Contributor

Worked against my custom /etc/resolv.conf file 馃憤

search foo.bar.com localdomain
nameserver x.x.x.x
nameserver x.x.x.x

@adfoster-r7 adfoster-r7 merged commit f56c9fc into rapid7:master Jan 19, 2024
34 checks passed
@adfoster-r7 adfoster-r7 added the rn-fix release notes fix label Jan 19, 2024
@adfoster-r7
Copy link
Contributor

Release Notes

Fixes an edgecase where features set dns_feature true did not correctly parse a user's /etc/resolv.conf file if there were multiple nameservers present

@adfoster-r7
Copy link
Contributor

Not a blocker: This would be easier to test if there was a top level resolve command, similar to the one in Meterpreter 馃槃

I think @zeroSteiner has got us covered here now with the work he's doing in the same area馃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rn-fix release notes fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants