pppd: do not rely on have_route_to(0) during auth.#581
pppd: do not rely on have_route_to(0) during auth.#581jkroonza wants to merge 2 commits intoppp-project:masterfrom
Conversation
ppp-project#543 for reference. Closes: ppp-project#543 Signed-off-by: Jaco Kroon <jaco@uls.co.za>
|
Impact assessment is surprisingly hard. The case that changes is where have_route_to(0) previously returned false for non-privileged users and neither auth nor noauth was set. In other words, if we did have a default route, we'd suddenly require the remote side to auth, but if we did not we'd default to noauth (without allow_any_ip, meaning we could have multiple pppd's but only certain IPs would be available to the remote side). This feels counter-intuitive to me. The motivation was that non-privileged users could establish internet connection without needing the privileged noauth option I believe. This use-case should be extremely rare nowadays, so just assume that a default route is already available, and force auth for all non-privileged invocations, meaning a system administrator would need to create a noauth peer for non-privileged users - typically managed via network manager nowadays anyway. |
|
@paulusmack: What do you think about this @jkroonza PR? |
|
If I recall correctly, the reasoning was that adding a route to an IPv4 address that you can't otherwise get to is pretty benign, so we could let unprivileged users do that. But letting them use an IPv4 address that we already have a route to could provide a way to spoof some trusted host, so to control that, we require the peer to authenticate itself, meaning that the sysadmin has to have set up a secrets file entry for them, and that entry can then dictate the IP address the peer can use. Now if there is a default route, there are no IPv4 addresses that we don't have a route to, so any unprivileged use of pppd requires authentication. (Not all of this reasoning really still makes sense today, but I think this was what I was thinking 20 - 30 years ago.) This change would mean that any use by an unprivileged user requires the peer to authenticate, unless they are using a privileged option file in /etc/ppp/peers that includes the "noauth" option. I think that sounds reasonable. Apparently Debian sets up pppd to be setuid-root and executable by group "dip", and they ship with "pon" and "poff" scripts to set-up/tear-down a PPP connection to an ISP. The "pon" script by default uses /etc/ppp/peers/provider, which has the "noauth" option in it. So that wouldn't be affected by this change. |
|
"Now if there is a default route, there are no IPv4 addresses that we don't have a route to" Well, the reasoning is probably solid, and along comes someone like me and invalidates that completely:
Rigged so that the unreachable gets redistributed, and x.y.z.0/22 is redistributed into ospf (redistribute kernel) and redistribute connected is so that non-ppp connections can be redistributed, and route-maps to filter the dynamic /32s for ppp, but not any static ranges so that statically assigned addresses on pppoe does get redistributed. Now, to defend the original statement: This isn't a desktop system. And auth is explicitly set up for sure. Are we okay with the "minimum change" here, or should we perhaps rethink this entirely? debian is generally (in my opinion) a good example to look at. I think this "neither auth nor noauth" makes kinda sense, so it's really noauth but the remote side can only use unreachable IPs, I think now, so if you have a default, LCP will establish now, but peer address negotiation will fail. So it will just take longer to fail I think. I'm not sure how to test this. Since IPv6 negotiates LLv6 only in any case, that should be fine, and DHCPv6 server would need to be configured already to do routing - so I reckon this is fine. What we may want to consider is impact on defaulroute and defaultroute6 options ... can non-privileged users specify those currently? What will happen if either default is already installed on the system? So perhaps defaultroute should become privileged if a default route for IPv4 exist, and defaultroute6 if default route for IPv6 exist? |
|
@paulusmack: Have you seen the latest @jkroonza comment? |
Slightly confused. I assume that by "now" you mean the current situation without this PR. Now, if you have a default, yes LCP will establish, but if the peer doesn't agree to authenticate itself to us, we'll terminate the link (i.e. we never get to IPCP or IPv6CP). That's assuming that pppd is run by non-root and we're not using an options file from /etc/ppp/peers that contains "noauth". With this PR, this will be the behaviour regardless of whether the system has a default route or not.
Yes, those options aren't privileged at present, though the "defaultroute-metric" and "defaultroute6-metric" options are privileged. If there is already a default route then you end up with two, and I suppose the system picks the one with the lower metric. Unfortunately the default value for defaultroute-metric and defaultroute6-metric is 0, and the code adds 1, so the default route will get installed with a metric of 1 by default. It would probably be better if that was 100 or 1000 or something so if there's already a default route the existing one would win. Or we just make "defaultroute" and "defaultroute6" privileged. What do you think? Maybe increasing the default default route metric is less likely to cause problems. |
|
Maybe we should just make "auth" the default irrespective of whether pppd is run by root or non-root. That would be easier to understand. |
Signed-off-by: Jaco Kroon <jaco@uls.co.za>
Now wasn't intended to refer at a tense. It's just my piss-poor-english leaking through. Let's unpack this: pre-patch: if there is a route to 0.0.0.0 (doesn't even have to be default, could be 0.0.0.0/X), then have_route_to() will return true. I'm not sure if an unreachable or prohibit route would also return true in this case but I suspect it will, so a "anti bogon" prohibit 0.0.0.0/8 route would take care of this. The check itself is thus potentially wrong as well depending on the intended meaning. I would suggest we completely drop have_route_to() from pppd - it's used in auth.c only, and is in pppd-private so is not intended to be used by external plugins, but auth_ip_addr() still uses it. If we do decide to make auth the default (noauth is privileged) then we can drop this function completely. And the more I think about this, the more I think this is the appropriate way to go. The other question is - could the "neither auth nor noauth" situation be used for dial-in services such that users can pick their own IPs? Let's say we've got a dial-in service with only a limited number of reachable targets on-net? Would it be useful to allow those connections to pick their own addresses? In hopefully short, the case where this was useful was to allow for a minimal configuration setup, such that if there was no default, a non-root user could if pppd was installed suid root (is this being done as a rule any more? Could the CAP_NET capability patch that we didn't like potentially have been useful in this use-case?) to bring up internet by calling some service and combined with defaulroute and/or defaultroute6. I like this concept, but I'm not sure the relevance any more. post-patch: If a default route exist, we'll pass through LCP + AUTH phases, but IPCP will continue to fail due to auth_ip_addr failing. I don't think we can drop the "somewhere in the middle behaviour", because even as root, if neither option is specified then it makes sense that we would not require the peer to auth to us since we're likely busy bringing up the internet connection.
We could make then privileged if a default route already exist, specifically a route where "rt.rt_genmask" == 0 for the relevant protocol, then that option could be made privileged? The simplest fix is to outright make them privileged IMHO (added to the PR). Given this, I think going default auth is OKAY, however, if we don't want to make auth the default (ie, maintain this somewhere in the middle default) then we should probably only make defaultroute and defaultroute6 privileged if those routes already exist (at whatever metric). |
Hmmm. I thought 0.0.0.0 was special and couldn't be subnetted. But it probably doesn't matter. I would guess that the current situation makes it unnecessarily difficult to set up an IPv6-only link, and that's probably the main problem with how things are now.
If we drop the have_route_to() check from auth_ip_addr(), then when pppd is run by non-root, either the IP of the peer needs to be explicitly authorized by a secrets file entry, or the "noauth" option needs to be in effect (i.e. from a call file in /etc/ppp/peers). When pppd is run by root, the peer can use any IP address, as at present, unless the "auth" option is used, in which case the IP of the peer needs to be authorized by a secrets file entry. If we make the default be to require authentication, then there is still a different between specifying "auth" and not specifying it, in that "auth" overrides "noauth", even if "auth" comes before "noauth" in the options processing sequence.
Well, at the least you'd need some sort of check to make sure two connections didn't pick the same IP address. But you probably have more experience running a dial-in service than me.
Re installing pppd setuid-root: Debian and Ubuntu (and probably other deb-based distros) still do that.
If we remove the !have_route_to(addr) from auth_ip_addr(), yes that's right; in that case running as non-root requires getting the "noauth" option in from a privileged options file, or having a secrets file entry to allow the address.
With this PR, if neither option is specified then the peer is require to authenticate itself to us if pppd is run by non-root, but not if it is run by root. Does that sound OK? (I think so.)
I think that's reasonable. FWIW Debian/Ubuntu have "defaultroute" in /etc/ppp/peers/provider.
Another option would be to have those options continue to be non-privileged but make the default defaultroute metric be very large, so that a default route added by pppd will only be effective if there is no other route to a given address (including other default routes). |
|
Another thing, this PR needs to remove the message in auth_check_options() that says "(because this system has a default route to the internet)". |
It's a route like any other, the mask length just happens to be 0 if you intend to mean "default", it so happens 0.0.0.0/8 is considered a bogon (rfc6890 references rfc1122 but I can't find this in the latter) and why we install that as a prohibit route to enable RPF on the interface to which the default route points to kill incoming traffic from there.
Correct. Not impossible, just annoying.
You're right. Will need to remain, but possibly be ammended to deal with non-forwarding routes such as unreachable/prohibit/blackhole(?)/throw. This however further complicates have_route_to because I'm not sure we get the most specific routes first, so we'd need to always traverse the full routing table and make sure we only look at the most specific matching route. We actually handle IP allocation using a somewhat custom IPA that also takes into consideration the multi-host nature of our setup, and performs the non-reachability check there too before assigning the IP. This reserves the address for 60 seconds and injects it via Framed-IP-Address into Radius. IPv6 allocation is handled by way of DHCPv6, no radius support there yet. But the plan is to after I made progress on radius-ng incorporate that into dhcpv6relay module.
And you're only slightly confused?
Refer above. I also don't mind assigning the same IP to multiple incoming links assuming it's the same customer and remote site (can in effect perform a crude form of link-balancing).
I'll keep that in mind, thank you for bringing this to my attention.
For this reason, and this reason alone I think have_route_to() should perhaps remain after all. For dial-in services one should use explicit auth, and frankly, not rely on this test.
I'll have to double check the logic, which I'm happy to do. The reasoning sounds OK to me.
Noted.
Let me sleep on this. Agreed, I like the privileged if no actual default installed better as I'm sitting here because how long is a piece of string? More work though. |
#543 for reference.
Closes: #543