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: convert '\' to '/' on windows #16852

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

harshavardhana
Copy link
Member

Description

fix: convert '' to '/' on windows

Motivation and Context

This is to ensure on windows we avoid
arbitrary path escalation during PUT
operations.

How to test this PR?

Needs a windows machine to test

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Optimization (provides speedup with no functional changes)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Fixes a regression (If yes, please add commit-id or PR # here)
  • Unit tests added/updated
  • Internal documentation updated
  • Create a documentation update request here

This is to ensure on windows we avoid
arbitrary path escalation during PUT
operations.
@minio-trusted
Copy link
Contributor

Mint Automation

Test Result
mint-erasure.sh ✔️
mint-compress-encrypt-dist-erasure.sh ✔️
mint-pools.sh ✔️
Deleting image on docker hub
Deleting image locally

@harshavardhana harshavardhana merged commit 8d6558b into minio:master Mar 20, 2023
@harshavardhana harshavardhana deleted the bad-path branch March 20, 2023 07:35
@klauspost
Copy link
Contributor

@harshavardhana @kannappanr We should just reject objects with \ - at least just on Windows. TBH I assumed we already did.

@klauspost
Copy link
Contributor

If we accept it, it will be a constant duality between / and \, which we shouldn't have. We should only allow /

@harshavardhana
Copy link
Member Author

harshavardhana commented Mar 20, 2023

If we accept it, it will be a constant duality between / and \, which we shouldn't have. We should only allow /

The reason is we have seen keys with \ on Linux so we can't blindly disallow it, however, we could reject the \ key for windows separately.

@klauspost
Copy link
Contributor

however, we could reject the \ key for windows separately.

Yeah, that was my thought. Otherwise \ gets converted to / on Windows, which IMO is worse than rejecting it from the start.

@harshavardhana
Copy link
Member Author

Yeah, that was my thought. Otherwise \ gets converted to / on Windows, which IMO is worse than rejecting it from the start.

It won't get converted, it's only validated as a check to look for .. @klauspost - so it will reject the PUT on windows with \.. - however outrightly rejected situations like \/hello is also needed on Windows.

Sent a PR #16856

@bh4t bh4t added the bugfix label May 23, 2023
alexanghh added a commit to alexanghh/minio that referenced this pull request Sep 14, 2023
daveaugustus pushed a commit to chef/minio that referenced this pull request Nov 15, 2023
daveaugustus pushed a commit to chef/minio that referenced this pull request Nov 17, 2023
daveaugustus pushed a commit to chef/minio that referenced this pull request Nov 17, 2023
Adphi pushed a commit to linka-cloud/minio-gateway that referenced this pull request Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants