ppp-project / ppp

Paul's PPP Package: PPP daemon and associated utilities | Official GitHub repo: https://github.com/ppp-project/ppp
https://github.com/ppp-project/ppp
Other
382 stars 228 forks source link

RADIUS protocol RFC 2865 susceptible to forgery attacks #480

Open ShivaNandini03 opened 6 months ago

ShivaNandini03 commented 6 months ago

The Message-Authenticator attribute should be forwarded in the Access-Request, which will be used to prevent spoofing of CHAP, ARAP or EAP Access-Request packets.

IETF recommendation for this Vulnerability:https://datatracker.ietf.org/doc/html/draft-ietf-radext-deprecating-radius-00

Neustradamus commented 4 months ago

@paulusmack, @enaess: What do you think?

paulusmack commented 4 months ago

I don't use radius myself. It sounds like we need somebody to generate a patch and test it (I don't have a way to test it).

@jkroonza any opinion here?

jkroonza commented 4 months ago

So basically they are saying that we should completely BAN using radius over non-encrypted protocols? Even if the target server is localhost? Or on a directly adjacent switch? At what point is a network considered insecure?

Our setup has ppp radius talking with freeradius on "localhost" (typically ::1 and in some cases where for reasons beyond me we're told to completely switch off IPv6), that instance will then typically communicate with a cluster of freeradius nodes, most generally on a shared subnet with the ppp server ... and we always control the entire switching path between the ppp+freeradius nodes and the auth radius servers ... so is UDP really such a big danger in such cases?

I'm very much open to discussion, and I'm OK with disabling plaintext by default, but like other projects doing the same I'd recommend an option of the nature "i know i'm the odd one out but getting certificates for non-public networks is a nuisance i'd rather not deal with" (dovecot calls the trusted networks).

I think 5.1 already alludes to this in a more polite way. "RADIUS/UDP and RADIUS/TCP MUST NOT be used outside of secure networks. A secure network is one which is known to be safe from eavesdroppers, attackers, etc. For example, if IPsec is used between two systems, then those systems may use RADIUS/UDP or RADIUS/TCP over the IPsec connection." ... they go on to talk about misconfigurations etc (which is fair, and given IPSec's reputation for misconfigurations, likely) and thus "Using RADIUS/UDP and RADIUS/TCP in any environment is therefore NOT RECOMMENDED.". Keywords here "NOT RECOMMENDED".

So I'm happy if changes to ppp is made, initially to WARN of insecure transports to destinations other than localhost (or any ip on the local system for that matter, eg, if I've got 10.0.0.1 assigned on lo, or even eth0, then traffic to 10.0.0.1 should be considered part of a trusted network (the simple check on ingress traffic is if the source&dest IP is the same, egress traffic requires a connect() to the remote address and then a call to get the local IP address - I know asterisk does a lot of this to get the correct address to advertise in RTP I just don't recall off the top of my head - This works for both v4 and v6 IP traffic due to the scope resolution rules - possible to modify but I know one other system administrator besides myself that has tampered with that in the past).

Not related to ppp itself, but a large number of router devices we use doesn't even support Radius/(D)TLS for AAA via Radius, so the scale of the change is insane.

My recommendation is simple:

Verify ppp supports Radius/(D)TLS. In 2.5.X branch should be fine (assuming no support yet). This enables ppp to talk with (D)TLS enabled radius servers.

Then the deprecation can be driven from freeradius (and other Radius servers's) side overall.

We can of course add a DEPRECATION WARNING to the ppp radius code itself (based on above rules, and option to switch off the warning such as radius-trust-network).

I don't think we can ever make the radius/udp or radius/tcp code go away completely, I for one have no interest in further loading hosts with pointless crypto code just to protect traffic passing over the loopback device (if at attacker can sniff that I've got FAR BIGGER issues), however, I do agree with the premise that any traffic (radius or otherwise) traversing an untrusted network should wherever possible be encrypted.

paulusmack commented 4 months ago

Do we actually support RADIUS over TLS or DTLS at this point? I can't see anything in the code to say that we do.

I don't see any mention in the referenced draft RFC of the Message-Authenticator attribute. I don't know precisely what is being suggested or how much work it would be to implement.

jkroonza commented 4 months ago

OK, so the aim is to (initially at least) just support TLS and DTLS? I note that we're using an in-tree version of libradiusclient ... any consideration of reverting to the upstream project or even libradiusclient-ng? Assuming they have that support? http://developer.berlios.de/projects/radiusclient-ng/

But in principle, yes, adding support for radius/TLS and DTLS would be a move in the right direction. The right approach in my opinion would be to abandon the libfreeradius clone in the ppp source and just depend on the upstream package (whichever of the variants we choose).

http://developer.berlios.de/projects/radiusclient-ng/ is the only reference I can find though which still seems active (releases at least for debian 12, ununtu 23, RHEL 7 and centos stream, to name but a few, so at least it seems there has been recent releases).

Perhaps just keep the old radius plugin and add a radiusng plugin which uses that, which then (hopefully) enables what we need, once that has seen some testing and wider acceptance we can consider deprecating the current radius plugin and eventually remove it completely?

enaess commented 4 months ago

I think the initial issue was opened because the underlying radius plugin doesn't include a "Message-Authenticator" attribute as a part of the ACCESS_REQUEST, ACCESS_CHALLENGE, ACCESS_REJECT, ACCESS_ACCEPT messages. The details is in the https://www.rfc-editor.org/rfc/rfc3579.txt for how that attribute is formed, i.e. HMAC-MD5

Let's defer the conversation about using TLS or DTLS to wrap the entire radius communication. Per the draft RFC linked above, the EAP-MSCHAPv2 should probably be deprecated or removed in favor of supporting EAP-PEAP-MSCHAPv2 or EAP-TLS where the entire communication is wrapped in TLS. I don't think Microsoft NPS support Radius on a TLS/DTLS connection as of yet (someone speak up if am wrong). Maybe some other vendor does and if someone had equipment to test against, then fair.

However, the right way to resolve this is to deprecate the radius.so plugin, or completely replace the implementation with a plugin that links to libradcli4-dev (on ubuntu) or one of the freeradius ones, e.g. libfreeradius-dev, libfreeradius3-dev

I did start on some of that work a long while ago. Just been bogged down with family life and work at the moment.

paulusmack commented 4 months ago

If someone wants to write and test a patch that adds the Message-Authenticator attribute, that would be great. I don't have a way to test such a patch.

jkroonza commented 4 months ago

There is no way we can abandon radius support. It's an essential part of authentication on the far majority of "server side" implementations. And since Linux on an Intel x86_64 host using rp-pppoe (pppoe-server) + ppp outperforms everything else we've tried we'd rather assist with efforts to help maintain the status quo here.

I can try to add the MA attribute as requested, just can't guarantee when this would happen. Not that this solves any problems (with having had a very limited look, and very basic understanding of the raw radius protocol, generally we only care about which tags does what since we consider the network layer to be "secure" - nothing goes over open internet and in all but one case stays on our own network), since assuming that without MA the frame is "insecure" and can be modified in transit or spoofed, unless radius server requires the MA attribute, a MITM situation can simply strip the MA attribute out before partially modifying the frame. Similar with the response from the radius server. Will have to more thoroughly read the RFC though to fully understand what's going on here.

gr4yg00s3 commented 1 month ago

Hello, I was doing some searching because of this https://blog.cloudflare.com/radius-udp-vulnerable-md5-attack and I came across this thread. I see that there was some discussion here to add support for the Message-Authenticator to the Radius Client code, I was wondering if that had been done?

jkroonza commented 1 month ago

It has not. This is on my "low priority" todo list, if you're happy to read the RFC and see what's required that would be great. We only run ppp's radius on loopback interface so we don't care too much about this to be honest, and all radius only ever traverses trusted networks anyway.

gr4yg00s3 commented 1 month ago

if you're happy to read the RFC and see what's required that would be great.

I can look into this 👍

jkroonza commented 1 month ago

It looks like there are two approaches here:

  1. Deprecate current radius module and use the freeradius client code. I think this is perhaps a larger project on it's own, one I have fairly little interest in to be honest. See above.
  2. Just amend the current code to include the relevant tags and values, which is probably simpler, this assumes that will satisfy the relevant concerns. Consider the relevant attack path, if radius traffic is still traversing plaintext using udp in combination with PAP (which is often the ONLY password validation supported by some clients) then protecting that with an HMAC in the relevant case as considered here is somewhat moot since now you can still just sniff the username & password, and no need for any fancy attacks against MAC or HMAC to begin with.
jkroonza commented 1 month ago

Correction: PW_USER_PASSWORD is encrypted using the shared-secret from the looks of it some simple XOR encryption using MD5 algorithm to generate a PRNG, so likely RC4 or similar. There are some funny things going on here, and we will fail on passwords of length>48.

Not seeing any date types in the dictionaries other than Expiration, which I don't think is used often. The freeradius dictionaries have tons. And here we go, from the primary dictionary for freeradius:

 28 #   date       - 32 bit value in big endian order - seconds since
 29 #            00:00:00 GMT,  Jan.  1,  1970

So fix I pushed should be fine for the time being until we can get a date64 something. Should probably start pushing that now given how long general time_t conversion to 64 bit has taken to date, don't think this affects ppp too much though.

enaess commented 1 month ago

Deprecate current radius module and use the freeradius client code. I think this is perhaps a larger project on it's own, one I have fairly little interest in to be honest. See above.

It's a larger project, but perhaps the "right" way to go. pppd project has favored pulling these projects in, but after the changes made to convert the build-system to GNU automake , it has been considerably easier to pull in third-party libraries (e.g. use libatm, and others). While it may incur some more maintenance work to keep up with releases of third-party software, it does have the benefit of moving that maintenance to the respective project and we don't have to back-port fixes and features.

I don't think radius over DTLS is widely adopted (yet) and Microsoft does not support this. I think this is mostly a recomendation, but when vendors starts adopting this (and closing the backdoor on using UDP/radius), then yeah it's probably time to add support for it.

jkroonza commented 1 month ago

I've never seen radius over TCP/TLS in practise, nor do I think that's a good idea from an operations perspective. But if we can "get it for free" by using an existing library that would be great. DTLS makes most sense for me, but possibly once I start looking into how it actually works (I'm betting there are cookies of sorts involved somehow, which means it's no longer stateless from a server perspective) I might be less inclined to think it's a good idea.