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

Add DNS rebinding protection #2397

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

juniorz
Copy link

@juniorz juniorz commented Dec 5, 2020

The implementation of IP blocking is based on dnsmasq in regard to which CIDRs to block.

Closes #102

@codecov-io
Copy link

codecov-io commented Dec 5, 2020

Codecov Report

Merging #2397 (3a4ff66) into master (2298a9e) will increase coverage by 0.52%.
The diff coverage is 80.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2397      +/-   ##
==========================================
+ Coverage   38.14%   38.67%   +0.52%     
==========================================
  Files          84       85       +1     
  Lines        9593     9723     +130     
==========================================
+ Hits         3659     3760     +101     
- Misses       5475     5498      +23     
- Partials      459      465       +6     
Impacted Files Coverage Δ
internal/dnsfilter/dnsfilter.go 59.23% <ø> (ø)
internal/dnsforward/config.go 46.95% <ø> (ø)
internal/dnsforward/stats.go 18.96% <0.00%> (-0.68%) ⬇️
internal/querylog/searchcriteria.go 35.13% <0.00%> (-1.49%) ⬇️
internal/dnsforward/dnsforward.go 50.92% <25.00%> (-1.00%) ⬇️
internal/dnsforward/rebind.go 82.45% <82.45%> (ø)
internal/dnsforward/dns.go 62.10% <100.00%> (-0.12%) ⬇️
internal/dnsforward/filter.go 68.88% <100.00%> (ø)
internal/dnsforward/http.go 61.42% <100.00%> (+0.70%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2298a9e...3a4ff66. Read the comment docs.

@ameshkov
Copy link
Member

ameshkov commented Dec 6, 2020

Wow that's massive, thank you!

@ArtemBaskal
Copy link
Contributor

Great, but there is a need to add some logic to handle the new DNS rebinding protection request in the query log correctly

@juniorz
Copy link
Author

juniorz commented Dec 23, 2020

@ArtemBaskal could you give some pointers on what's missing specifically? Thanks for reviewing.

@ameshkov
Copy link
Member

@juniorz the problem is that this new status is not handled by the query log.

  1. There's no human-readable name for it.
  2. There's no way I can find it using any search criteria. I suggest reusing "filteringStatusBlockedSafebrowsing" for it -- see searchcriteria.go. Let it find both queries blocked by Safe Browsing web service AND by DNS rebinding protection (and in the future, we may extend it with other security features).
  3. I also suggest reusing the same color indication as for FilteredSafeBrowsing.

@juniorz
Copy link
Author

juniorz commented Jan 4, 2021

Hi @ameshkov , thank you for the feedback. I believe I have found my way through the stats and logging subsystems and I was able to make the requests blocked to be displayed to the end-user.

Could you please review and provide feedback on the choice of colors/categorization?

Responses blocked due to DNS rebinding attempts will contribute to "Blocked by Filters" stats, and in the logs, they are in the "Filtered" category - but I have also added a "Blocked rebinding" category.

@juniorz
Copy link
Author

juniorz commented Jan 4, 2021

Also, lint failed but the Github check does not give any meaningful output so I don't really know what needs to be fixed. And running make go-deps go-tools go-lint VERBOSE=2 does not show anything either.

@ameshkov
Copy link
Member

ameshkov commented Jan 6, 2021

@juniorz got it, thank you! We'll take care of it

@ameshkov ameshkov added this to the v0.106.0 milestone Jan 26, 2021
@ameshkov ameshkov modified the milestones: v0.106.0, v0.107.0 Apr 19, 2021
@ameshkov ameshkov removed the request for review from ArtemBaskal April 19, 2021 13:16
@ainar-g ainar-g removed this from the v0.107.0 milestone Dec 9, 2021
@ainar-g ainar-g added this to the v0.108.0 milestone Dec 9, 2021
@Dynasty-Dev
Copy link

When will this be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide an option for blocking internal IP addresses
6 participants