search

matthew-brett / delocate

Find and copy needed dynamic libraries into python wheels
BSD 2-Clause "Simplified" License
262 stars 59 forks source link

sanitize rpaths #101

Closed isuruf closed 7 months ago

isuruf commented 3 years ago

In gfortran-darwin-arm64 toolchain, I had accidentally kept absolute rpaths like /Users/isuruf/miniforge3/lib in there and delocate didn't do anything. (I've since fixed it). If we had released numpy wheels with these, anybody who could manipulate /Users/isuruf/miniforge3/lib can inject code. It's good to sanitize these absolute rpaths to avoid security issues like these.

cc @mattip

HexDecimal commented 2 years ago

Is there a good way to determine which paths are intended verses which paths are superfluous?

Is it as simple as blacklisting user directories or whitelisting system directories?

Are all relative paths okay? Or should anything that doesn't start with @loader_path or @executable_path be removed? Should only relative paths starting with @loader_path be allowed? Should paths with @executable_path be replaced with @loader_path?

Should there be multiple sanitation levels or options? What should be the default?

What are the existing tools doing about this kind of issue?

isuruf commented 2 years ago

We should definitely blacklist user directories. Other absolute directories like /opt might be okay, but that wouldn't really make the wheel relocatable. So, I think we should blacklist any absolute rpaths and maybe have an option to disable it.

HexDecimal commented 2 years ago

So, I think we should blacklist any absolute rpaths and maybe have an option to disable it.

That could work. I was thinking you couldn't blacklist a path like /System/Library but now I'm not sure if anything depends on those without linking to them directly (without using rpaths.)

isuruf commented 2 years ago

For /System/Library, you don't need rpath. Dynamic loader will look at those paths irrespective of rpaths.

mattip commented 7 months ago

Would the next step be to use --sanitize-paths in invocations of delocate in multibuild and cibuildwheel?

HexDecimal commented 7 months ago

Yes, after the new version is released. Which I'll do in a moment.

HexDecimal commented 7 months ago

New version released. It's optional right now with --sanitize-paths. I was too paranoid to make it the default, but maybe it should be.

I only know of my own use-case which is to temporarily link a path for delocate to find the library at. The current tests are for this case.