p-e-w / maybe

:open_file_folder: :rabbit2: :tophat: See what a program does before deciding whether you really want it to happen (NO LONGER MAINTAINED)
6.35k stars 163 forks source link

detection for mknod pipes #15

Closed sanketplus closed 8 years ago

sanketplus commented 8 years ago

there might be problem with signature as I do not get that part completely. Also I'm planning to add support for block and char special file detection.

p-e-w commented 8 years ago

Thank you for looking into this. There is a lot of complexity surrounding Unix special files, and some care is needed to make sure we do the right thing here.

First, named pipes are not persistent and calling mknod on an existing path will simply fail with EEXIST, so creating a named pipe with mknod (or mknodat, mkfifo, mkfifoat) cannot actually make any permanent change to the system. I therefore do not think that this is an action that maybe should block.

Things are a bit different with character and block files. Technically, it would be possible for the underlying device driver to translate operations on special files to disk writes on a normal file system. I do not know whether such a driver actually exists. Most programs don't need to create special files so blocking these calls should not interfere with proper operations usually. If my understanding is correct it might be a good idea to indeed block such calls, but I would appreciate some input on this from someone who can explain the details a little better.

In any case, there is a more dangerous invocation of mknod that we should handle first: Reading the manpages I was surprised to see that mknod, with the S_IFREG type, can apparently also be used to create an ordinary disk file! I have no idea why a process would call mknod instead of open in this case, but this is definitely a loophole allowing an operation that maybe is supposed to prevent.

I will need a few days to think this over. Again, thank you for pointing me in this direction with your PR.

As a side note, I try to keep all code in this repository PEP8-compliant, which your PR is currently not. A tool like Flake8 can fix this for your automatically.

sanketplus commented 8 years ago

I checked for persistence of named pipes and it seems they behave like regular files only. Means they exist permanently on disk.Here: [http://unix.stackexchange.com/questions/84286/why-does-a-named-pipe-not-get-deleted-after-system-restart]

In this case maybe need to check if process is making named pipes or not. And they are used generally because when I was making a web server using netcat I needed to use named pipes to communicate between netcat and the script executed for every incoming connection.

I also added check for S_IFREG type but I couldn't test it as it seems it isn't supported in Shell of Ubuntu.

Also I was unaware of PEP8-compliant codes. I tried to fix it but there are still errors related to indentation in SYSCALL_FILTERS area. Check it out and let me know.

And thank you for looking into my PR :innocent:

p-e-w commented 8 years ago

I checked for persistence of named pipes and it seems they behave like regular files only. Means they exist permanently on disk.

Whoa... you're right. What I meant is that data passing though named pipes does not persist, but you are correct in pointing out that the node still remains, and can affect other programs by making future calls to mknod fail. This means we definitely need to block mknod & friends!

I've added a few comments to your code. Once those are addressed this PR should be looking pretty good :+1:

p-e-w commented 8 years ago

As for the PEP8 error concerning indentation in SYSCALL_FILTERS, I ignored that on purpose because indenting there would just create a lot of useless whitespace at the start of hundreds of lines.

sanketplus commented 8 years ago

Made changes as suggested by you in comments :)

And for mkfifo call, in Ubuntu it's a library call which internally calls mknod.

The command strace mkfifo hello had mknod("hello", S_IFIFO|0666) = 0 in it's output. This again can be confirmed by mkfifo's existence in man 3.

But on the other hand while googling for the same, I found mkfifo is a system call for FreeBSD. So I couldn't test for maybe for mkfifo.

Kindly let me know if more changes are required :grin:

p-e-w commented 8 years ago

Your point about some Linux stdlib calls being syscalls on BSD is very interesting. I was not aware of this and simply chose to err on the side of caution, adding all relevant calls from the (2) and (3) manpages. It seems now that at least remove needs not be blocked at all, as it is a higher-level library function in both Linux and FreeBSD.

sanketplus commented 8 years ago

Made suggested changes, kindly review :innocent:

p-e-w commented 8 years ago

Apart from my last two remarks this seems ready to merge. :+1: When those are fixed, please squash the commits and rebase on top of current master.

sanketplus commented 8 years ago

See the latest commit! I kinda messed while rebasing and squashing as I never did it before :sweat_smile:

Kindly look at it if there are any problems! Thanks :yum:

p-e-w commented 8 years ago

The easiest way to squash is usually doing a soft reset to the base commit (last commit before yours)

git reset --soft HASH_OF_BASE_COMMIT

Then you are free to simply write a single new commit as you choose.

sanketplus commented 8 years ago

That's a much simpler way :smile: What I did was

git rebase -i HASH

and then change pick to squash

p-e-w commented 8 years ago

Merged! :fireworks: Thank you very much for this! :elephant:

You seem to know quite a bit about syscalls. My goal with maybe is to block everything that could make permanent changes to the system. Are there any other calls we are currently letting through that fall into that category?

Another thing: Where did you find the signatures for mknod and mknodat? Some Linux online manpages list another parameter, dev_t dev.

sanketplus commented 8 years ago

Thank you @p-e-w!! This was my first contribution in open source and I'm grateful to you to make it happen :smiley:

I had a course in my college on Design of OS and I know the basics only! :sweat_smile: The rest I googled including signatures on man pages! :)

I will definitely keep contributing to this project as I find it very interesting and cool! :D

Thanks Again :yum:

sanketplus commented 8 years ago

The dev_t parameter is used in case if option is other than p which is b or c for block/char special file. They show major(ie USB,printer) and minor(ie which usb port,printer) device number.

I am not sure about whether it should be added to signature or not. If it should be, then we need separate signature for mknod on pipe call and on special file call.