twtiger / gosecco

Go seccomp parser and compiler
GNU Lesser General Public License v3.0
53 stars 7 forks source link

Constant strings for argument values in policies #36

Closed dma closed 8 years ago

dma commented 8 years ago

Hey,

How are we to use the constants in https://github.com/twtiger/gosecco/blob/master/constants/go_constants.go? Should the gosecco parser just know to look for them, or should we include a separate file with all of these as variables?

compile of a policy with this line fails on defined variables:

socket: arg0 == AF_LOCAL || arg0 == AF_INET || arg0 == AF_INET6 || arg0 == AF_NETLINK

chelseakomlo commented 8 years ago

Hey- you should be able to do both- declare constants within a specific policy file, and as well as in a separate file that can be applied to multiple policy files. Variables that you define in the policy file will be applied for anything below its declaration.

So, for example: varA = 3 socket: arg0 == a varA = 5 read: arg1 == a

The rules will be evaluated as: socket: arg0 == 3 read: argo == 5

Where do you currently declare constants?

chelseakomlo commented 8 years ago

Oh, I see. These are specifically for https://github.com/twtiger/gosecco/blob/master/constants/go_constants.go

dma commented 8 years ago

Yeah, there are some very useful constants defined there, but they're not usable anywhere it seems. I think we can plug them into the parser (or maybe the unifier?) as was done here:

https://github.com/twtiger/gosecco/blob/master/compiler/return_actions.go#L36

(there also needs to be a constants.GetArg(string name) or GetConstant(string name) or whatever, this seems to be missing)

chelseakomlo commented 8 years ago

I believe we are using these constants currently only in the case of declaring a syscall as well as in the case of an error-

https://github.com/twtiger/gosecco/blob/79afbebb9e9d2b49d2b5441471fb907324aee6e3/native/calls.go#L18

However, I don't believe we are checking to see if a constant that is declared in a policy file is one of these declared in https://github.com/twtiger/gosecco/blob/master/constants/go_constants.go

It is worth thinking over whether we should include this functionality (if you think these constants will be used frequently enough), or if it is enough of an edge case to be only provide them as extra definitions:

https://github.com/twtiger/gosecco/blob/master/seccomp.go#L41

dma commented 8 years ago

Yeah, the syscall names are used, the return constants are used, but the argument constants aren't. I'll just put them in my own extra definitions file for now. Maybe I can later submit a pull request for gosecco if I implement it in the library in a way that I'm happy with. Thanks for the quick replies.

chelseakomlo commented 8 years ago

If you think that these constants will be used widely as variables across different policies, then it would make sense to have them available by default. We can also take a look early this week- I suspect this could be a small change we can do quickly. El 10/7/2016 12:38 p. m., "David Mirza Ahmad" notifications@github.com escribió:

Yeah, the syscall names are used, the return constants are used, but the argument constants aren't. I'll just put them in my own extra definitions file for now. Maybe I can later submit a pull request for gosecco if I implement it in the library in a way that I'm happy with. Thanks for the quick replies.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/twtiger/gosecco/issues/36#issuecomment-231601034, or mute the thread https://github.com/notifications/unsubscribe/ACabc3iGD1EVPSTq1gjiAEXLbNfw0Er9ks5qUS4ogaJpZM4JIttb .

dma commented 8 years ago

Yes, they will be. I think this is one of the key features / advantages of gosecco.

Right now we have policies like:

socket: arg0 == 2 || arg0 == 10 || arg0 == 1

By using these constants, we'd have a policy like:

socket: arg0 == AF_LOCAL || arg0 == AF_INET || arg0 == AF_INET6 || arg0 == AF_NETLINK or setsockopt: (arg1 == SOL_SOCKET && arg2 == SO_REUSEADDR) || (arg1 == SOL_SOCKET && arg2 == SO_KEEPALIVE)

Advantages of having like "AF_LOCAL" instead of "1" are that it's a) human readable, and b) cross-platform. Finally we could use static analysis to parse source code for system call arguments looking for these constants as they are defined in the code to proactively create policies, rather than relying on tracing as we do today, resulting in greater code coverage. If it's quick and easy for you.. yes please :)

olabini commented 8 years ago

This was supposed to have been there - the only reason why we have allConstants in the first place. Luckily it was easy peasy to add. The latest commit fixes it.

dma commented 8 years ago

Ok, thanks a lot! I'm porting Oz to Gosecco, so you folks may hear more from me. I'm also studying Gosecco because I think I can learn a lot from how you designed it + the fact that it's relatively simple for a compiler.

olabini commented 8 years ago

Great! Feel free to ask anything, of course.