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

FFI support #5566

Closed
wants to merge 10 commits into from
Closed

FFI support #5566

wants to merge 10 commits into from

Conversation

buckle2000
Copy link
Contributor

@buckle2000 buckle2000 commented May 17, 2020

The foreign function interface of Deno should allow code to call functions from shared library with native ABI. The Deno runtime should not segfault the program by itself, but the external library may.

FFI is inherently unsafe.

Capability: --allow-ffi.

We also need https://tootallnate.github.io/ref/ to manage memory addresses and pointers.

Prior Art

Python:

from ctypes import cdll
library = cdll.LoadLibrary("libraylib.so")
library.BeginDrawing()

from ctypes import windll
print(windll.kernel32)  

Go:

package main

// typedef int (*intFunc) ();
//
// int
// bridge_int_func(intFunc f)
// {
//		return f();
// }
//
// int fortytwo()
// {
//	    return 42;
// }
import "C"
import "fmt"

func main() {
	f := C.intFunc(C.fortytwo)
	fmt.Println(int(C.bridge_int_func(f)))
	// Output: 42
}

Node (thrid-party node-ffi):

var ffi = require('ffi');

var libm = ffi.Library('libm', {
  'ceil': [ 'double', [ 'double' ] ]
});
libm.ceil(1.5); // 2

// You can also access just functions in the current process by passing a null
var current = ffi.Library(null, {
  'atoi': [ 'int', [ 'string' ] ]
});
current.atoi('1234'); // 1234

LuaJIT:

local ffi = require("ffi")
ffi.cdef[[
int printf(const char *fmt, ...);
]]
ffi.C.printf("Hello %s!", "world")

Rust: extern keyword

Questions

  • Can a pointer fit inside number?
  • Can we make a ArrayBuffer represent the whole memory?
  • dlclose can run code. Should Library.close require --allow-ffi?
  • How to allow reading data from symbols without random memory access?
  • How to allow calling foreign functions without ret2libc? That will make --allow-ffi=path actually useful.
  • Should we make --allow-ffi whitelist certain paths even if allowing the program to load any external executable means arbitrary code execution?

Related issues

#5551

@CLAassistant
Copy link

CLAassistant commented May 17, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@buckle2000 buckle2000 mentioned this pull request May 18, 2020
@bartlomieju bartlomieju added cli related to cli/ dir feat new feature (which has been agreed to/accepted) labels May 18, 2020
@buckle2000
Copy link
Contributor Author

buckle2000 commented May 21, 2020

The JS API is complete but not implemented. Anyone to vet this?

I feel that putting all of those functions under Deno might make people use ffi for bad things.

@dyedgreen
Copy link
Contributor

Regarding the permission, I feel like it would be better to use --allow-all instead of a dedicated --allow-ffi, to make clear that the permissions are equivalent and all security guarantees are dropped when executing external code from a .so or similar.

@mtornwall
Copy link

mtornwall commented May 24, 2020

I feel like it would be better to use --allow-all instead of a dedicated --allow-ffi, to make clear that the permissions are equivalent and all security guarantees are dropped when executing external code from a .so or similar.

--allow-all is still less restrictive than the hypothetical --allow-ffi flag suggested above. While it's true that an unrestricted FFI would allow you to run any code you'd like, you would still need to actually use the FFI to do so. While it won't protect you against malicious code, opting for --allow-ffi may well protect you against buggy code.

@dyedgreen
Copy link
Contributor

--allow-all is still less restrictive than the hypothetical --allow-ffi flag suggested above.

That's not true. Using native code and the FFI, I can do anything I want, without the user even being able to necessarily inspect the code easily, as can be done with some JS/TS code (I could ship the code compiled and obfuscated, which means the user now needs expert knowledge to inspect what my extension may do).

The point of the permission system is not to guard agains my own buggy code (although that's a nice side-effect), but mainly to guard agains some random code I downloaded from the internet, e.g. to do a specific calculation for me.

@mtornwall
Copy link

mtornwall commented May 24, 2020

That's not true.

It definitely is true, but perhaps you understand "less restrictive" differently than I do? Let me try to explain what I mean:

  1. If you run malicious code that makes use of the FFI to do its evil work, it doesn't matter if you run it with --allow-all or --allow-ffi. So far I believe we agree.
  2. On the other hand, say you run some buggy code that uses the FFI for legitimate purposes, but the author forgot to remove a test snippet he wrote that recursively deletes /home/<username> using the Deno file system API (sorry for the contrived example). In that case there's a big difference between running with --allow-all and --allow-ffi, since --allow-all exposes a much larger API surface. This is what I intended by "less restrictive", though I realize I could have made that more clear.

Even if protection against buggy code is not a primary goal of the permission system, it seems we both agree that it is at least "a nice side-effect". In that case, wouldn't it be better to further explore how --allow-ffi could be made more granular, such as by specifying which particular shared objects and symbols it can access, rather than throwing our hands up and forcing everyone that wants to use the FFI to run their code with --allow-all?

@buckle2000
Copy link
Contributor Author

buckle2000 commented May 24, 2020

I think a seperate --allow-ffi flag is better, and maybe --allow-all shouldn't allow FFI.

Putting that aside, some functionality like make SharedArrayBuffer from arbitrary memory can't be implemented with current "OP" binding (that can only pass data by copy). Anyone with a better understanding with deno?

@lygstate
Copy link

ping for update and merge or any help need?

@ry
Copy link
Member

ry commented Jul 17, 2020

@buckle2000 I'm very sorry for the slow response. I think you sent this PR right after the 1.0 release, and it got buried amongst the other issues.

I would like to get some FFI working - but it seems like this is just a sketch of the API so far. Looks reasonable as far as I can see. I think what we need to do is get some demo working - dlopen a DLL, call a function from it, etc.

Also I think we should just require --allow-all rather than introduce a new --allow-ffi. It's effectively what it means.

@bartlomieju
Copy link
Member

@buckle2000 this branch is very out of date, please rebase to latest master if you want to pursue this change.

@bartlomieju
Copy link
Member

Closing as stale. I'm still interested in further development of this feature so don't hesitate to reopen if updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir feat new feature (which has been agreed to/accepted)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants