Kernel module for advanced rickrolling replaces open() call
github.com
github.com
These problems are, of course, fixable someone may fix it.
Bonus points for whoever fixes the problems and submits a pull request. All the info you need is in this discussion thread.
(edit: I can already see some pull requests on this)
I will incorporate all fixes and address all issues raised in the next few days.
I think that instead of the strlen pull request you accepted, you should use getname/strncpy_from_user like the open() syscall does instead of strlen+memcpy.
Also, while you're here, could you explain what is the purpose of disabling memory write protection? Is it to allow writing to the syscall table? You seem to enable writing in the page table for the syscall table before writing, but never disable it. Could it be that you forgot to flush the translation lookaside buffer (TLB) after updating the pagetable and the change you did does not take effect soon enough? I don't know how you flush TLB's in Linux but the Intel manuals tell you to do so after updating the page table.
It's fun to see that there were so many people interested in this, although it's not much more than a joke. I guess part of the interest stems from the fact that it's a small kernel module with so little code that it's easy to grok.
For a joke weekend project that you never intended anyone to see / use seriously? Sorry, no. I would never waste time making something secure that I did for funzies that I'll likely never touch again. If I happen to come across or realize a security issue as I'm writing it I might make a comment there in the source or in the README. Otherwise, that's time spent I could have been making my real projects better and more secure.
This is a very bad idea as path is user-supplied and has to be treated as malicious. An attacker can omit the string-terminator...
So your assumption on omitting the null terminator is wrong, BUT there's another flaw in this. The length of the string can be less than four bytes. By allocating the string in a nice page boundary this can be used to cause a memory protection fault.
The PATH_MAX guarantees that the string copy will terminate even if no null character is present.
Now if any routines that call strncpy_from_user assume that it leaves them with a null terminated string there is still a potential for trouble but that does not stem from the use of strncpy_from_user.
Check the implementation for do_getname:
http://lxr.free-electrons.com/source/fs/namei.c
And you'll see that it returns -ENAMETOOLONG when the name is >= the buffer allocated, only < len is allowed. (line 133 in the link above).
But the kernel does not call strlen() on the user-supplied string...
Anyone who's using open() can kill the kernel after loading this module.
And if you have that ability, worrying about the security of a module such as this seems, to me, totally and utterly pointless, because you could be inserting a rootkit, or a module that randomly deletes a file every time you press the letter A etc.
So could you explain what you mean please? Why be concerned about security when security is already compromised? How does it being multi-user make a difference?
Suppose your admin is a funny guy and loads this kernel module at April 1st. I (Jonny non-root user) can no longer listen to my favorite mp3, because the kernel module changes the path all the time. The admin loughs and has fun.
But then later I call open() with some fancy arguments and take over the whole machine (or at least crash it). This is the part where I lough.
is a absolute no-go. It's a nice and funny kernel module but it dooms the kernel's security.
What this means is that the kernel can no longer see if a user is writing to a read-only location, which is a major part of memory protection. It has major security implications. I don't know whether other parts of the kernel enable or disable this at context switches or at other times.
What this also means is that this kernel module will only work on x86 CPU's. So no rickrolling you ARM boxes or smartphones.
No, disabling WP in CR0 is only relevant for privileged code. Writes to write protected pages in user mode will still trap. It might mess up some in-kernel COW stuff though.
Deleted comment
There are some stddef.h which cast it to a void*, but technically that's just a compatibility hack. See http://c-faq.com/null/macro.html http://c-faq.com/null/safermacs.html http://c-faq.com/null/nullor0.html http://c-faq.com/null/long0.html
2. Using FUSE you would have to guaranty that your fs overlaps all other mounted filesystems.
You might even have several libc's in your system. You might have a uClibc + Busybox based initrd and a full glibc based root system.
Btw. how does LD_PRELOAD act together static binaries?
So an app calls the open() function. This results in an unresolved symbol which the run-time linker matches up with the open() exported by glibc. glibc then does the open() syscall.
If libc implements it, then it is by definition in section 3.
EDIT: (signing out of this sub-thread)
Have you ever called a system call by hand using assembly?
Have you ever looked at glibc's source?
Habe you ever looked at the kernel's close() syscall?