mattn / anko

Scriptable interpreter written in golang
http://play-anko.appspot.com/
MIT License
1.45k stars 120 forks source link

dangerous packages import #327

Closed alaingilbert closed 4 years ago

alaingilbert commented 4 years ago

There is a major problem in "newer" versions. Before this commit 8016764 We had to create NewPackage inside the VM and import the functions we wanted. eg:

source = "math"
methods, _ := packages.Packages[source]

pack := myvm.NewPackage(source)
for methodName, methodValue := range methods {
    pack.Define(methodName, methodValue)
}

(so we could whitelist the wanted packages per VM)

Now we have to do _ "github.com/mattn/anko/packages" to initialize the packages, and they are all imported and available in all of your VMs. which means that by default, all VMs will have the net/http package (dangerous). And there is no way to have different VMs with different packages anymore.


Example of malicious usage: If you let your users script on your website using anko.


I might want to have 1 user with net/http package enable, and other users with the package disabled. This is no longer possible.

MichaelS11 commented 4 years ago

Packages now default to having all packages added. You can remove packages as wanted.

If you want to have more than one package config, just import env more than once and configure as you would like.

MichaelS11 commented 4 years ago

Also note, you can use the Go build flag appengine which will remove some of the packages, like net and net/http, from being adding by default.

alaingilbert commented 4 years ago

https://github.com/mattn/anko/blob/5f413ab07b663b97e425cc0a056520bd56ad25b9/vm/vmExpr.go#L515 The import function in the VM uses a "global" variable.

https://github.com/mattn/anko/blob/5f413ab07b663b97e425cc0a056520bd56ad25b9/env/env.go#L32

As far as I know, this global variable is shared among all VMs. If you change it, you change it for everybody. (and you also get multi-threading issues (as they all use the same global variable, and no mutex protect it))

MichaelS11 commented 4 years ago

If you import env twice, there will be two env, each with there own Packages varable.

alaingilbert commented 4 years ago

so let's do something a bit exagerated: Let say I have 100 customers, and they all have different configs (packages allowed) I would have to import manually the "env" a hundred times, and manually configure the packages with all possible combinations ?

MichaelS11 commented 4 years ago

I get your point, it is not as flexible as before, however, do you understand why this change was made? How env is copied in the VMs and the resource load a VM copy was taking for most use cases?

alaingilbert commented 4 years ago

tbh, I don't really know why the change was made. I didn't find any explanations (other than "Simplified packages") in the commit. This is just something I realized while trying to upgrade the lib in my app.

MichaelS11 commented 4 years ago

Welcome to ask questions :)

alaingilbert commented 4 years ago

I'm curious now ! I guess that loading the packages in the VM was taking more memory than not loading them. Do you have any benchmark / order of magnitude ? like how much ram it takes to load "math" package for example ? (I'm genuinely curious. I know I could do the benchmark myself, I'm just curious if you have any numbers at hand)

MichaelS11 commented 4 years ago

The PR that commit was in had a lot going on. Performance was just a small part of it and not really the main focus. One of the main goals was splitting out env to make things cleaner, especially for testing.

I do not have any benchmarks. Every time a new env was created, all the defines had to be run again. If you had a lot of VMs, this would have taken up a lot of space.

So can you close this issue?