Sorry, I forgot to reply.
Do you think it would make sense to split the PEP into two PEPs? The auditing hook and import opener hook are related, but distinct improvements. The auditing part looks solid and ready now. The import opener may need some more refinement. I would also like to get feedback from some Linux Kernel security engineers first.
On 01/04/2019 18.31, Steve Dower wrote:
On 31Mar2019 0538, Christian Heimes wrote:
I don't like the fact that the PEP requires users to learn and use an additional layer to handle native code. Although we cannot provide a fully secure hook for native code, we could at least try to provide a best effort hook and document the limitations. A bit more information would make the verified open function more useful, too.
So instead they need to learn a significantly more complicated API? :) (I was very happy to be able to say "it's the same as open(p, 'rb')").
PyObject *PyImport_OpenForExecution( const char *path, const char *intent, int flags, PyObject *context )
- Path is an absolute (!) file path. The PEP doesn't specify if the file
name is relative or absolute. IMO it should be always absolute.
Yeah, this is fair enough. I'll add it as a requirement.
- The new intent argument lets the caller pass information how it
intents to use the file, e.g. pythoncode, zipimport, nativecode (for loading a shared library/DLL), ctypes, ... This allows the verify hook to react on the intent and provide different verifications for e.g. Python code and native modules.
I had an intent argument at one point and the feedback I got (from teams who wanted to implement it) is that they wouldn't trust it anyway :)
In each case there should be associated audit events for tracking the intent (and interrupting at that point if it doesn't like the intended action), but for the simple case of "let me open this specific file" it doesn't really add much. And it almost certainly shouldn't impact decision making.
There is no need to trust the intent flag that much. I would like to have a way to further narrow down the scope for an open call. This would allow the caller to tell the hook "I want to open something that should be a shared library suitable for ctypes". It would allow tighter control.
Audit events are useful and powerful. But I don't want to put too much burden on the auditing framwork. I prefer to have checks that prevent operations rather than allow operations and audit them.
- The flags argument is for additional flags, e.g. return an opened file
or None, open the file in text or binary mode, ...
This just makes it harder for the hook implementer - now you have to allow encoding/errors arguments and probably more. And as mentioned above, there should be an audit event showing the intent before this call, and a hook can reject it at that point (rather than verify without actually returning the verified content).
I retract this part of my proposal. With O_MAYEXEC it's better to always open the file, but then use the file's FD to retrieve the actual file name for dlopen(). That approach allows the Kernel to verify DAC permissions, prevents memfd_create() hacks through readlink, and simplifies the hook.
* Linux: readlink("/proc/self/fd/%i") * macOS: fcntl F_GETPATH * Windows: GetFileInformationByHandleEx
- Context is an optional Python object from the caller's context. For
the import system, it could be the loader instance.
I think the audit event covers this, unless you have some way of using this context in mind that I can't think of?
To be honest I don't have a good use case yet. I just like the idea to have a way to pass some custom thing into an API and now who called an API. You seem to like it, too. Your hook has a void *userData, but it's not passed into the Python function. :)
int PyImport_SetOpenForImportHook(hook_func handler, void *userData)