unbound: restrict creation of PTR records for both the system domain and host overrides #5925

swhite2 · 2022-08-02T13:22:56Z

In order to prevent unpredictable behaviour from something that is not explicitly prohibited in RFC1035, it is best to restrict the creation of PTR records from every single host and alias (except for wildcard entries, no PTR records are created here), to only non-alias overrides (edit: the exception here is an alias whose parent does not create a PTR record). We also further restrict it to unique IP addresses so there can be no confusion in how to maintain the entries within the running Unbound instance.

Hopefully this can pave the way for adding PTR records as a separate type instead of generating them under the hood, as is done currently.

This change should at least address inconsistencies regarding random PTR records being returned as mentioned in #5477

fichtner · 2022-08-02T13:26:38Z

fair enough but my main concern was

core/src/etc/inc/plugins.inc.d/unbound.inc

Line 473 in 2115c45

    
           $unbound_entries .= "local-data-ptr: \"{$laddr} {$config['system']['hostname']}.{$domain}\"\n";

and

core/src/etc/inc/plugins.inc.d/unbound.inc

Line 483 in 2115c45

    
           $unbound_entries .= "local-data-ptr: \"{$laddr6} {$config['system']['hostname']}.{$domain}\"\n";

swhite2 · 2022-08-02T13:35:32Z

fair enough but my main concern was

core/src/etc/inc/plugins.inc.d/unbound.inc

Line 473 in 2115c45

$unbound_entries .= "local-data-ptr: \"{$laddr} {$config['system']['hostname']}.{$domain}\"\n";

and

core/src/etc/inc/plugins.inc.d/unbound.inc

Line 483 in 2115c45

$unbound_entries .= "local-data-ptr: \"{$laddr6} {$config['system']['hostname']}.{$domain}\"\n";

Also very relevant, I suppose the same logic makes the most sense here

fichtner · 2022-08-02T19:21:21Z

src/etc/inc/plugins.inc.d/unbound.inc

+            if ($interface === $interfaces[0] && !in_array($laddr, $ptr_records, true)) {
+                $unbound_entries .= "local-data-ptr: \"{$laddr} {$config['system']['hostname']}.{$domain}\"\n";
+                $ptr_records[] = $laddr;
+            }
            $unbound_entries .= "local-data: \"{$config['system']['hostname']}.{$domain} A {$laddr}\"\n";


Maybe we should widen the conditional to guard A and AAAA as well?

We should, but I'm not entirely sure if we can rely on the sorted list of interfaces and pick the first one. Shouldn't we at the very least determine if the picked interface is used for e.g. the webgui?

So I've made e76f15c for you to make this easier, maybe even move it out of the interfaces loop.
I tend to agree, but the metrics for the web GUI are difficult (what interfaces does the user want to listen on and what sort of NAT is he doing to change it to something else). So what about trying to aim for the same logic as anti-lockout would? That's a little further but not too complicated.

@fichtner e5633b8. What do you think?

src/etc/inc/plugins.inc.d/unbound.inc

…checking to avoid recursive dependencies

src/etc/inc/plugins.inc.d/unbound.inc

…and host overrides (#5925) In order to prevent the unpredictable behaviour of random PTR records being returned, which is not explicitly prohibited in RFC1035, it is best to restrict the creation of PTR records from every single host and alias (except for wildcard entries, no PTR records are created here), to only non-alias overrides (edit: the exception here is an alias whose parent does not create a PTR record, a wildcard entry). We also further restrict it to unique IP addresses so there can be no confusion in how to maintain the entries within the running Unbound instance. Hopefully this can pave the way for adding PTR records as a separate type instead of generating them under the hood, as is done currently. This change should at least address inconsistencies regarding random PTR records being returned as mentioned in #5477 A slight refactor of the existing unbound code is also included here for code reduction purposes. (cherry picked from commit 92a5a22)

fpehla · 2022-09-08T11:35:48Z

The last commit (547c8b1) broke the creation of ptr records for host overrides that are not aliases. No ptr records are generated at all for host entries even if the entry is not an alias and the ip address is not already referenced by another ptr record.

The new condition in unbound.inc
if (($alias === $tmp_aliases[0] || $tmp_aliases[0]['hostname'] === '*') && !in_array($host->server, $ptr_records, true))
evaluates to false for regular host overrides.

Rewriting the condition like this fixes this issue for me (note the negation '!' inserted):
if (!($alias === $tmp_aliases[0] || $tmp_aliases[0]['hostname'] === '*') && !in_array($host->server, $ptr_records, true))

This results in

PTR records being created for host overrides
no PTR records being created for aliases

efahl · 2022-09-09T14:54:57Z

Rewriting the condition like this fixes this issue for me (note the negation '!' inserted): if (!($alias === $tmp_aliases[0] || $tmp_aliases[0]['hostname'] === '*') && !in_array($host->server, $ptr_records, true))

Tested and works for me, thanks.

guttermonk · 2022-09-22T17:07:30Z

I'm still receiving warnings for "PTR record already exists". I see the fix above, but it looks like an update has been deployed. I'm running Unbound version 1.16.2

Was the fix for this issue merged to master on Aug 16th? Is there a way to tell which version of unbound master is on?

efahl · 2022-09-22T17:36:59Z

@guttermonk Shouldn't matter what version of unbound is running, this is in OPNsense's code that generates the unbound config. Looking at master, I don't see the fix that @fpehla proposes above. See https://github.com/opnsense/core/blob/master/src/etc/inc/plugins.inc.d/unbound.inc#L573

guttermonk · 2022-09-22T17:43:47Z

Should this issue be flipped from Merged to Open for visibility?

swhite2 · 2022-09-23T08:05:08Z

I'm still receiving warnings for "PTR record already exists".

This is completely normal for setups where multiple hostnames/domain names point to the same IP address (like when using an alias), this is a warning for this very reason: it is nonsensical to have multiple PTR records all pointing to the same IP, the system will just return a random host/domain name. The warnings' verbosity might be up for discussion though.

No ptr records are generated at all for host entries even if the entry is not an alias and the ip address is not already referenced by another ptr record.

@fpehla I don't think the negation is the solution here, it's a solution by chance since the equality comparison $alias === $tmp_aliases[0] excludes the host override IF a hostname is specified as the object gets modified when this is the case:

if ($alias['hostname'] != '') {
    $alias['hostname'] .= '.';
}

Host overrides which do not have a hostname specified will still generate a PTR record, I can easily reproduce this on my end.

I'll prepare a patch if you wish to test this.

swhite2 · 2022-09-23T08:56:03Z

@fpehla # opnsense-patch 2351f17c3

ref: 2351f17

maciekb · 2022-09-23T12:14:45Z

@swhite2 I I confirm that your patch works, all PTR records are generating correctly. Without the patch, the PTR records for the overwritten hosts did not generate.

swhite2 · 2022-09-23T12:19:53Z

@swhite2 I I confirm that your patch works, all PTR records are generating correctly. Without the patch, the PTR records for the overwritten hosts did not generate.

@maciekb Good to hear. Thanks for the feedback.

@fpehla Thanks for catching this!

fpehla · 2022-09-23T12:23:54Z

Thanks for the patch, this fixes the issue for me. PTR records are generated again for overrides.

I was not sure if the negation I introduced was the correct fix or if it just solved the issue for me by chance and broke something for others. So thanks again for looking into this!

swhite2 requested review from fichtner and AdSchellevis August 2, 2022 13:22

swhite2 self-assigned this Aug 2, 2022

swhite2 changed the title ~~unbound: only generate PTR record for primary domain~~ unbound: restrict creation of PTR records for both the system domain and host overrides Aug 2, 2022

swhite2 mentioned this pull request Aug 2, 2022

Unbound reverse PTR records #5477

Closed

fichtner reviewed Aug 2, 2022

View reviewed changes

src/etc/inc/plugins.inc.d/unbound.inc Outdated Show resolved Hide resolved

fichtner reviewed Aug 2, 2022

View reviewed changes

src/etc/inc/plugins.inc.d/unbound.inc Show resolved Hide resolved

swhite2 added 3 commits August 3, 2022 11:44

unbound: only generate PTR record for primary domain

8695a35

unbound: restrict PTR records for interfaces as well, also do strict …

5539913

…checking to avoid recursive dependencies

unbound: log a warning if an overlapping PTR exists

4d2350a

swhite2 force-pushed the master branch from 9a5d8f8 to 4d2350a Compare August 3, 2022 09:45

swhite2 added 2 commits August 3, 2022 13:34

unbound: refactor unbound_add_host_entries

a3ee365

unbound: toggle system A/AAAA record creation

e5633b8

fichtner reviewed Aug 16, 2022

View reviewed changes

src/etc/inc/plugins.inc.d/unbound.inc Outdated Show resolved Hide resolved

restructure text

69d91f2

fichtner reviewed Aug 16, 2022

View reviewed changes

src/etc/inc/plugins.inc.d/unbound.inc Outdated Show resolved Hide resolved

swhite2 added 2 commits August 16, 2022 13:43

safeguard generating a PTR record for primary interface only

34633e9

people are strange

a5132bb

fichtner approved these changes Aug 16, 2022

View reviewed changes

swhite2 merged commit 92a5a22 into opnsense:master Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unbound: restrict creation of PTR records for both the system domain and host overrides #5925

unbound: restrict creation of PTR records for both the system domain and host overrides #5925

swhite2 commented Aug 2, 2022 •

edited

fichtner commented Aug 2, 2022

swhite2 commented Aug 2, 2022

fichtner Aug 2, 2022

swhite2 Aug 3, 2022

fichtner Aug 3, 2022

swhite2 Aug 3, 2022

fpehla commented Sep 8, 2022

efahl commented Sep 9, 2022

guttermonk commented Sep 22, 2022

efahl commented Sep 22, 2022

guttermonk commented Sep 22, 2022

swhite2 commented Sep 23, 2022 •

edited

swhite2 commented Sep 23, 2022

maciekb commented Sep 23, 2022

swhite2 commented Sep 23, 2022

fpehla commented Sep 23, 2022

unbound: restrict creation of PTR records for both the system domain and host overrides #5925

unbound: restrict creation of PTR records for both the system domain and host overrides #5925

Conversation

swhite2 commented Aug 2, 2022 • edited

fichtner commented Aug 2, 2022

swhite2 commented Aug 2, 2022

fichtner Aug 2, 2022

Choose a reason for hiding this comment

swhite2 Aug 3, 2022

Choose a reason for hiding this comment

fichtner Aug 3, 2022

Choose a reason for hiding this comment

swhite2 Aug 3, 2022

Choose a reason for hiding this comment

fpehla commented Sep 8, 2022

efahl commented Sep 9, 2022

guttermonk commented Sep 22, 2022

efahl commented Sep 22, 2022

guttermonk commented Sep 22, 2022

swhite2 commented Sep 23, 2022 • edited

swhite2 commented Sep 23, 2022

maciekb commented Sep 23, 2022

swhite2 commented Sep 23, 2022

fpehla commented Sep 23, 2022

swhite2 commented Aug 2, 2022 •

edited

swhite2 commented Sep 23, 2022 •

edited