Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dangerous packages import #327

Closed
alaingilbert opened this issue Mar 2, 2020 · 10 comments
Closed

dangerous packages import #327

alaingilbert opened this issue Mar 2, 2020 · 10 comments

Comments

@alaingilbert
Copy link
Contributor

alaingilbert commented Mar 2, 2020

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.

  • With the new version, they can use net/http to "ddos" others website using your server.
  • They can also ping their service, and leak your server IP address (and now you are vulnerable to be DDOSed)... let say you were using cloudflare to protect your IP, now it's public.

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

alaingilbert commented Mar 2, 2020

methods, ok := env.Packages[name]

The import function in the VM uses a "global" variable.

Packages = make(map[string]map[string]reflect.Value)

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
Copy link
Contributor

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

@alaingilbert
Copy link
Contributor Author

alaingilbert commented Mar 2, 2020

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
Copy link
Contributor

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
Copy link
Contributor Author

alaingilbert commented Mar 2, 2020

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
Copy link
Contributor

Welcome to ask questions :)

@alaingilbert
Copy link
Contributor Author

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
Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants