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

fix(backend): reject symlinks on emoji import #15535

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

eternal-flame-AD
Copy link
Contributor

What

Check the emoji files in import are actually regular files.

Why

It is possible someone can upload a symlink and in turn read arbitrary files on the system.

This is currently not exploitable because the zip crate version in slacc is too low and extracts to a 0o777 file instead of an actual symlink, but after bumping to the latest verion it will become vulnerable.

Additional info (optional)

Maybe consider add checks in slacc too, might be better to check every file before extracting instead of just call .extract().

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
@github-actions github-actions bot added the packages/backend Server side specific issue/PR label Feb 22, 2025
Copy link
Contributor

このPRによるapi.jsonの差分
差分はありません。
Get diff files from Workflow Page

Copy link

codecov bot commented Feb 22, 2025

Codecov Report

Attention: Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 42.13%. Comparing base (7c87dec) to head (05499ac).

Files with missing lines Patch % Lines
...e/processors/ImportCustomEmojisProcessorService.ts 10.00% 9 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #15535      +/-   ##
===========================================
+ Coverage    40.86%   42.13%   +1.26%     
===========================================
  Files         1609     1609              
  Lines       161806   161815       +9     
  Branches      3786     4084     +298     
===========================================
+ Hits         66119    68177    +2058     
+ Misses       95239    93158    -2081     
- Partials       448      480      +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anatawa12
Copy link
Member

anatawa12 commented Feb 22, 2025

I feel it's better to check if each entry is (expected to be) symlink before writing to file system in slacc than checking on javascript side

(or possibly zip2 crates security problem? since the crate has security safeguards for path traversal for extracting files, but not for symlinks feels problem in zip2 crate.)

@eternal-flame-AD
Copy link
Contributor Author

I agree, I can write a PR for slacc if you accept PR there.

It isn't a security problem in the crate IMO because it is not specifically designed to handle user uploaded files.

The behavior is documented and I agree it is expected behavior that when one asks for "extract this zipfile" by default it creates symlinks, and there are legitimate uses like taking a backup of a directory which can include symlinks that traverse out of the directory.

@eternal-flame-AD
Copy link
Contributor Author

I found a related but different sec concern in zip2 and I contacted the author to confirm, it would be best if we hold off updating for now.

But generally speaking I don't think we should extract zip files from any user upload, just read the content of record.fileName into a JS buffer and return. Zip files can encode a lot more than just file contents and the default behavior for extracting files can have many unexpected behaviors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/backend Server side specific issue/PR
Projects
Development

Successfully merging this pull request may close these issues.

2 participants