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

Add option to disallow symlinks #26

Closed
mortenson opened this issue Nov 23, 2019 · 11 comments
Closed

Add option to disallow symlinks #26

mortenson opened this issue Nov 23, 2019 · 11 comments

Comments

@mortenson
Copy link
Contributor

For applications where uploaded archives are untrusted, it would be useful to have a way to disallow symlinks. This would effectively protect against exploits that involve archives that point to (and eventually override) system paths. See https://www.exploit-db.com/papers/13199 and https://github.com/BuddhaLabs/PacketStorm-Exploits/blob/master/0101-exploits/tar-symlink.txt for more information.

I'm happy to create a PR for this, but want to check to see if this is a feature you would accept and if so, where it should live. I was thinking of adding it as another parameter to \Archive_Tar::extract.

What do you think?

@mrook
Copy link
Member

mrook commented Nov 27, 2019

Yeah that sounds good to me, a PR is appreciated!

@mortenson
Copy link
Contributor Author

mortenson commented Nov 28, 2019

@mrook Thanks! Just filed #27

@mrook
Copy link
Member

mrook commented Dec 3, 2019

Merged the PR - I'll make sure to tag a new release later this week.

@mortenson
Copy link
Contributor Author

@mrook Thank you!

@mrook
Copy link
Member

mrook commented Dec 4, 2019

@mrook mrook closed this as completed Dec 4, 2019
@klausi
Copy link

klausi commented Dec 20, 2019

The Drupal security advisory is saying that Archive_Tar had a security release: https://www.drupal.org/sa-core-2019-012

But that is not the case I think? Symbolic links are still processed by default, so users are still in danger if they use this library the same way. Should the release notes warn users to change their library usage?

@mortenson
Copy link
Contributor Author

@klausi I'm not sure if you mean the Drupal release notes or the Archive_Tar release notes - but for Drupal, core code that wraps Archive_Tar was updated to disallow symlinks. Drupal is not responsible for how users use third party dependencies directly.

I think it was incorrect for the SA to say that the Archive_Tar release was "security update" - allowing symlinks is a feature of Archive_Tar and not a security flaw, so the update really added an optional security enhancement for applications that do not want to support symlinks.

@klausi
Copy link

klausi commented Dec 20, 2019

I mean the Drupal release notes which say "The Drupal project uses the third-party library Archive_Tar, which has released a security update that impacts some Drupal configurations."

But Archive_Tar did not mention that this is a security update in the release notes at https://pear.php.net/package/Archive_Tar/download/1.4.9

Allowing symlinks is a very dangerous feature for anybody that uses Archive_Tar on attacker supplied archive files.

@mortenson
Copy link
Contributor Author

@klausi It was not a security update of Archive_Tar, it was an optional security feature addition. I agree that the release notes could have told people who untar untrusted data to use the new flag, however. Not sure if that's possible retroactively.

@greggles
Copy link

I've updated the Drupal security advisory page to try to clarify what happened.

I think it could be a good idea for the Archive_Tar library to add some help text about being cautious in enabling symlinks for user-generated content.

@mrook
Copy link
Member

mrook commented Dec 20, 2019

Good idea, I'll add that.

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

4 participants