pf: stop resolving hosts as dns that use ":" modifier

[franco_opnsense.org]

Summary

When the interface does not exist avoid passing host
with special pf modifiers to DNS resolution as they
come up empty anyway.

[kp]

Wouldn't it make more sense to do this check in host_dns() ?
Something like this:

diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
index 1f6a194591c0..1a1b7fa688be 100644
--- a/sbin/pfctl/pfctl_parser.c
+++ b/sbin/pfctl/pfctl_parser.c
@@ -1819,6 +1819,9 @@ host_dns(const char *s, int v4mask, int v6mask)
        int                      got4 = 0, got6 = 0;
        char                    *p, *ps;

+       if (strchr(s, ':') != NULL)
+               return (NULL);
+
        if ((ps = strdup(s)) == NULL)
                err(1, "host_dns: strdup");
        if ((p = strrchr(ps, ':')) != NULL && !strcmp(p, ":0")) {

Arguably we should have a more complete check though. For extra fun, I'm not at all sure what those checks should be, or even that ':' is not valid in a DNS name.

RFC 2181, in chapter 11 states: "Those restrictions aside, any binary string whatever can be used as the label of any resource record." (The text before that discusses length limits, hence the 'those restrictions aside')
RFC 4343, chapter 2, has "However, the individual octets of which DNS names consist are not limited to valid ASCII character codes. They are 8-bit bytes, and all values are allowed. Many applications, however, interpret them as ASCII characters."

So, while it's very unlikely for a name to contain ':' (or anything other than a-zA-Z0-9, '.' or '-') I'm very reluctant to actually enforce that, because the standard says something else.

[franco_opnsense.org]

Something like "ovpnc0:network" is hardly a domain name as one user noted seeing these pop up and chasing it to lookups in pfctl. host_if() implements these special markers and we could argue that pfctl-specific markers have priority and shouldn't be handled elsewhere.

I tried handling cont variable through passing the pointer for it to host_if() as well to make a proper decision about special marker use but that was more lines of code that clutter the code here.

I'm not sure someone will ever see a ":" in a domain name but again we're not here to try to prevent it from a DNS lookup. We are trying to omit a lookup if it was possibly a host_if() specific lookup that went nowhere since the interface wasn't there.

There is still the problem of passing down something like "ovpnc0" if it doesn't exist as an interface but that is more of a general pf.conf concern that likely won't fix ever.

[kp]

In D35429#803611, @franco_opnsense.org wrote:

Something like "ovpnc0:network" is hardly a domain name

The DNS RFCs seem to disagree there. In practice it's not, but I'm not keen to enforce limitations not explicitly standardised.

I think what we should do is explicitly check for the suffixes (so :network, :broadcast, :peer and :0) and then not do DNS lookups for those strings.

I tried handling cont variable through passing the pointer for it to host_if() as well to make a proper decision about special marker use but that was more lines of code that clutter the code here.

It would be a lot more obvious what's going on if we're more explicit though. I wouldn't consider that clutter.

There is still the problem of passing down something like "ovpnc0" if it doesn't exist as an interface but that is more of a general pf.conf concern that likely won't fix ever.

It's a design feature that we can list interfaces which don't exist yet, but apply policy on them if they are created later.

[franco_opnsense.org]

• Revert "pf: stop resolving hosts as dns that use ":" modifier"
• pfctl: stop resolving hosts as DNS that use internal ":" modifiers

[franco_opnsense.org]

Updated revision to address requirement to only skip known modifiers. Minimal code change, but more convoluted with the cont pointer being passed down additionally.

From a practical perspective the previous patch works fine in real-world environments as subsequently shipped in our releases. No fallout there as expected.