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

bogus_nxdomain does not block responses when at least one IP address is not listed as bogus #2394

Closed
PussAzuki opened this issue Dec 4, 2020 · 9 comments
Assignees
Milestone

Comments

@PussAzuki
Copy link

PussAzuki commented Dec 4, 2020

Prerequisites

Please answer the following questions for yourself before submitting an issue. YOU MAY DELETE THE PREREQUISITES SECTION.

  • [✔] I am running the latest version
  • [✔] I checked the documentation and found no answer
  • [✔] I checked to make sure that this issue has not already been filed

Issue Details

It's easy to reproduce.

  1. Select a domain name with at least two IP addresses and write down its IP address
  2. Add one or more of the above IP addresses to the software that blocks IP access, and confirm that the blocking is successful and at least one IP address can be accessed successfully.
  3. Confirm that at least two DNS servers that do not belong to the same provider have been added to the upstream part of the AGH and confirm that AGH uses the fastest IP mode.
  4. Attempt to request the above domain name on any host in the network segment monitored by AGH.

Expected Behavior

Only IP that is not blocked in the above operation is returned.

Actual Behavior

As if the above blocking operation is invalid, it is possible to return an unblocked IP address and a blocked IP address.

Additional Information

bug_example
bug_example2

@ameshkov ameshkov added this to the v0.106.0 milestone Dec 9, 2020
@ameshkov ameshkov changed the title Logic loophole exists in AGH bogus_nxdomain does not block responses when at least one IP address is not listed as bogus Dec 15, 2020
@ameshkov
Copy link
Member

ameshkov commented Dec 15, 2020

Moved it to v0.105.0 since the only that's required is updating dnsproxy to v0.33.5.
Changeset: AdguardTeam/dnsproxy@f0c00d2

@ainar-g ainar-g modified the milestones: v0.106.0, v0.105.0 Dec 16, 2020
adguard pushed a commit that referenced this issue Dec 16, 2020
Merge in DNS/adguard-home from 2394-update-dnsproxy to master

Updates #2394.

Squashed commit of the following:

commit 57526d1
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Dec 16 14:43:29 2020 +0300

    all: document changes

commit acdfd6c
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Dec 16 14:28:23 2020 +0300

    all: update dnsproxy
@ainar-g
Copy link
Contributor

ainar-g commented Dec 17, 2020

Should be fixed as of snapshot fd7b061. Can you please check if our solution fixes the issue for you?

@PussAzuki
Copy link
Author

2020/12/17 21:24:09 8250#1 [debug] Upstream 0: 192.168.7.1:5353
2020/12/17 21:24:09 8250#1 [info] Starting the DNS proxy server
2020/12/17 21:24:09 8250#1 [info] 1 bogus-nxdomain IP specified
2020/12/17 21:24:09 8250#1 [info] Creating the UDP server socket
2020/12/17 21:24:09 8250#1 [info] Listening to udp://127.0.0.1:53535
2020/12/17 21:24:09 8250#1 [info] Creating a TCP server socket
2020/12/17 21:24:09 8250#1 [info] Listening to tcp://127.0.0.1:53535
2020/12/17 21:24:09 8250#6 [info] Entering the UDP listener loop on 127.0.0.1:53535
2020/12/17 21:24:09 8250#7 [info] Entering the tcp listener loop on 127.0.0.1:53535
2020/12/17 21:24:16 8250#19 [debug] github.com/AdguardTeam/dnsproxy/proxy.(*Proxy).udpHandlePacket(): Start handling new UDP packet from 127.0.0.1:39241
2020/12/17 21:24:16 8250#19 [debug] github.com/AdguardTeam/dnsproxy/proxy.(*Proxy).logDNSMessage(): IN: ;; opcode: QUERY, status: NOERROR, id: 49853
;; flags: rd ad; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1

;; QUESTION SECTION:
;avatars0.githubusercontent.com.	IN	 A

;; ADDITIONAL SECTION:

;; OPT PSEUDOSECTION:
; EDNS: version 0; flags: ; udp: 4096
; COOKIE: 95e58acf3d43b655

2020/12/17 21:24:16 8250#19 [debug] 192.168.7.1:5353: sending request A avatars0.githubusercontent.com.
2020/12/17 21:24:16 8250#19 [debug] 192.168.7.1:5353: response: ok
2020/12/17 21:24:16 8250#19 [debug] github.com/AdguardTeam/dnsproxy/proxy.exchangeWithUpstream(): upstream 192.168.7.1:5353 successfully finished exchange of ;avatars0.githubusercontent.com.	IN	 A. Elapsed 5 ms.
2020/12/17 21:24:16 8250#19 [debug] github.com/AdguardTeam/dnsproxy/proxy.(*Proxy).Resolve(): Received IP from the bogus-nxdomain list, replacing response
2020/12/17 21:24:16 8250#19 [debug] github.com/AdguardTeam/dnsproxy/proxy.(*Proxy).Resolve(): RTT: 5 ms
2020/12/17 21:24:16 8250#19 [debug] github.com/AdguardTeam/dnsproxy/proxy.(*Proxy).logDNSMessage(): OUT: ;; opcode: QUERY, status: NXDOMAIN, id: 49853
;; flags: qr rd ra; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 0

;; QUESTION SECTION:
;avatars0.githubusercontent.com.	IN	 A

I tested it with the dnsproxy of the master branch, and without the bogus-nxdomain function, it returned full results (including the one with access problems). After that, all the results were gone.

I'm not sure if this is what it should have been.

@ainar-g
Copy link
Contributor

ainar-g commented Dec 24, 2020

@ameshkov, I'm not quite sure what the verdict here should be. Should we recheck if dnsmasq does the same?

@ameshkov
Copy link
Member

@ainar-g I already checked and this is exactly how it works in dnsmasq.

@ainar-g
Copy link
Contributor

ainar-g commented Dec 25, 2020

@ameshkov, I meant that OP probably wanted AGH to only exclude the blocked IP, and not the whole response (@PussAzuki, can you confirm?). But if you think that what we do now is more correct, then I guess we can close the issue?

@PussAzuki
Copy link
Author

Yes, I think your idea is more suitable for the current environment; dnsmasq said that the function is to prevent DNS service providers from falsifying fake DNS responses, but this should no longer be the case, so I think this function should be changed to: if there is only one answer and is on the list, discard the result, and if there are multiple answers and at least one is not on the list, keep the result that is not on the list. This can solve the problem of routing black holes only in specific areas.

@ameshkov
Copy link
Member

@ainar-g I understand, but it does not change the point -- this feature should work exactly as it is implemented in dnsmasq. There're some "bogus IPs" lists compatible with dnsmasq, if we want to be fully compatible too, we should just repeat.

Regarding the desired functionality, there are different feature requests (#2445) where this could be implemented.

@ainar-g
Copy link
Contributor

ainar-g commented Dec 28, 2020

I see, thanks. I guess I'll close this one then.

@ainar-g ainar-g closed this as completed Dec 28, 2020
heyxkhoa pushed a commit to heyxkhoa/AdGuardHome that referenced this issue Mar 20, 2023
Merge in DNS/adguard-home from 2394-update-dnsproxy to master

Updates AdguardTeam#2394.

Squashed commit of the following:

commit 57526d1
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Dec 16 14:43:29 2020 +0300

    all: document changes

commit acdfd6c
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Dec 16 14:28:23 2020 +0300

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

No branches or pull requests

3 participants