> Hi Mike, > > I'm somewhat wary of this idea, because it creates a very attractive > target for crackers. Once an attacker managers to fool this proxy > daemon, he can talk to your NFS Server, concealing his identity or > even posing as root, etc. > > I looked at the code in the light of this, and there are a few > soft spots. > > - you retrieve credentials in the initial negotiation > only. But client credentials can change, and should be > checked with every packet. > > For instance, I can connect to your service, and fork off > some setuid root application, with stderr connected to that > socket. Any error message the application prints will be arrive > with uid 0. If I manage to make that message appear valid to you, > your daemon will accept any future input unquestioned. >
Interesting attack, although I doubt the setuid program would be attaching an SCM_CREDENTIALS to it's stderr writes. I'll fix it up to check credentials on all packets nevertheless. > So my recommendation would be to validate credentials on > every packet, and if they change from the original values > received during negotiation, drop the connection. > > I'd also recommend to change the permissions on the Unix > socket to 700. > Yup. Sounds good to me. > - This mechanism is really a special-case workaround for the > mount problem, but you're creating a general purpose framework. > That means if it's attacked successfully, it can be abused > to attack any RPC based service on any host. > > If you make it less generic, and allow only mount calls, you'll > be much safer, because in the case of a bug, an attacker will > be able to send fake MOUNT packets, but nothing else. > Hmm. I like the idea of keeping it generic as it may very well solve someone else's problem as well. As for locking it down to MOUNT (and possibly PMAP/RPCB), how about some sort of config file that limits PROG/VERS tuples? > - In several places, you keep packet offsets (pos, bufferpos) > in signed variables and compare them to unsigneds; that's > dangerous. Noted. I'll try and audit to resolve any signedness issues. > > - Your code assumes sizeof(unsigned long) == 4, eg here > > unsigned long len; > [...] > memcpy(&len, conn->buffer, 4); > len = ntohl(len); > buf = &conn->buffer[4]; > > It's better to use uint32_t in such cases. Yup. Will fix. Thanks for looking it over, Mike Waychison _______________________________________________ autofs mailing list [email protected] http://linux.kernel.org/mailman/listinfo/autofs
