archive_recipe: compress tars with --dereference flag - #56
Conversation
With the "--dereference" flag, tar follows symlinks, archives and dump the files they point to, replacing the symlink. For e.g. the following: lrwxrwxrwx 1 39 libgpg-error.so -> /lib/x86_64-linux-gnu/libgpg-error.so.0 will now become: -rw-r--r-- 1 84032 libgpg-error.so due to the "--dereference" flag. Pros: * Avoids the possiblity of archive extraction vulnerabilities, where an attacker may gain access to parts of the file system outside of the target folder on extraction. See e.g. https://snyk.io/blog/severe-security-vulnerability-in-bowers-zip-archive-extraction/ * The resultant dependency.tgz will pass all systems testing against the above stated vuln. Cons: * Increases the compressed dependency archive size since symlinks have to be replaced by their targets during compress-time. The compress-time filesystem is expected to have the target available. For e.g. the following: lrwxrwxrwx 1 14 libuv.so -> libuv.so.1.0.0 lrwxrwxrwx 1 14 libuv.so.1 -> libuv.so.1.0.0 -rwxr-xr-x 1 747232 libuv.so.1.0.0 will now become: -rwxr-xr-x 3 747232 libuv.so -rwxr-xr-x 3 747232 libuv.so.1 -rwxr-xr-x 3 747232 libuv.so.1.0.0
|
@sophiewigmore @brayanhenao Added both of you as reviewers in case you have any thoughts before I merge this. |
|
@arjun024 does this have an effect on the size of other dependencies? If so, is there a way to just do this for PHP? Also, were you able to run the tests (see README)? I don't have really any prior context on the related packit symlinking issue, so I'm not sure. Can I get a second opinion from @ForestEckhardt since you've been looking into some related symlinking issues lately? |
|
@sophiewigmore It should have an effect on all dependencies that are tar.gz and has symlinks. There's not a way I have found where only symlinks pointing to outside the archive can be dereferenced. I think we should change the recipe for all not just PHP so that future dependencies are covered against any chance of extraction vulnerabilities. The increase in size is a trade off against this. |
|
@arjun024 @sophiewigmore First of all the symlink issue has been resolved hopefully for good 🤞. I am fine with a 2MB size bump if it makes the dependency more secure and more reliable. I think you are good to merge imo! |
|
I'm merging this. In case we see any of the buildpacks/dependencies increasing size unreasonably, let's revisit this and only dereference symlinks for certain dependencies. |
Fixes: #253 Fixes: #255 Also see cloudfoundry/binary-builder#56
Fixes: #253 Fixes: #255 Also see cloudfoundry/binary-builder#56
This makes the changes done in #56 to only affect php. We noticed undesired consequences in node and .Net and thus is only done in php until more details come to light.
With the
--dereferenceflag, tar follows symlinks, archives and dumpthe files they point to, replacing the symlink.
For e.g. the following:
will now become:
due to the
--dereferenceflag.Pros:
Avoids the possiblity of archive extraction vulnerabilities, where an
attacker may gain access to parts of the file system outside of the
target folder on extraction.
See e.g. https://snyk.io/blog/severe-security-vulnerability-in-bowers-zip-archive-extraction/
The resultant dependency tarball will pass all systems testing against the
above stated vuln.
Cons:
to be replaced by their targets during compress-time. The compress-time
filesystem is expected to have the target available.
For e.g. the following:
will now become:
See issue filed on a buildpack that consumes dependencies built using the binary-builder: paketo-buildpacks/php-dist#253