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

Some fixes to mounting path validation (fix #3733) #3908

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

Conversation

ntorresalberto
Copy link
Contributor

Fixes for #3733 and Additional Issues Found During Inspection

This PR addresses the problems outlined in #3733, along with some related issues discovered during review. Details below.

Issues Fixed

Relative Paths Incorrectly Resolving to /home/ubuntu

The issue occurs when relative paths unintentionally resolve to /home/ubuntu, allowing unexpected mounts. Here’s how to reproduce it (same steps as in the issue, repeated for clarity):

$ mkdir foodir
$ multipass delete -p foo
$ multipass launch --name foo

# Expected to fail, as it does:
$ multipass mount foodir foo:/home/ubuntu  
mount failed: The following errors occurred:
unable to mount to "/home/ubuntu"

# Expected not to work, but it does (matches /home/ubuntu):
$ multipass mount foodir foo:.              
$ multipass mount foodir foo:../ubuntu      

Relative Paths Allowing Mounts to Restricted Directories

Similar path resolution issues allowed mounting to restricted locations such as /dev/, /:

# Expected not to work, but it does (matches /dev):
$ multipass mount foodir foo:../../dev  

# Expected not to work, but it does (matches /):
$ multipass mount foodir foo:../..          

Issues with Non-Normalized Paths Causing Duplicate Entries

Paths should be normalized before being stored in vm_mounts to prevent different representations of the same path from being treated as separate entries. Without normalization, paths like /home/foo and ../foo are considered distinct, leading to inconsistencies.

# Expected to work, and it does:
$ multipass mount foodir foo:/home/foo                     

# Expected NOT to work, and correctly fails (absolute paths are handled properly):
$ multipass mount foodir foo:/home/../home/foo  
mount failed: The following errors occurred:
"/home/foo" is already mounted in 'foo'

# Expected not to work, but it does:
$ multipass mount foodir foo:../foo                                

Fix Implementation

A new function, make_abspath, has been added to src/utils/utils.cpp.

  • Converts relative paths to absolute paths, defaulting from /home/ubuntu.
  • Guarantees unique, normalized paths for each mount, preventing duplicate entries.

Validation & Tests After Fixes

$ multipass delete -p foo
$ multipass launch --name foo
$ multipass mount foodir foo:/home/ubuntu
mount failed: The following errors occurred:
unable to mount to "/home/ubuntu"
$ multipass mount foodir foo:.
mount failed: The following errors occurred:
unable to mount to "/home/ubuntu"
$ multipass mount foodir foo:../ubuntu
mount failed: The following errors occurred:
unable to mount to "/home/ubuntu"
$ multipass mount foodir foo:../../dev
mount failed: The following errors occurred:
unable to mount to "/dev"
$ multipass mount foodir foo:../..
mount failed: The following errors occurred:
unable to mount to "/"
$ multipass mount foodir foo:/home/foo
$ multipass mount foodir foo:/home/../home/foo
mount failed: The following errors occurred:
"/home/foo" is already mounted in 'foo'
$ multipass mount foodir foo:../foo
mount failed: The following errors occurred:
"/home/foo" is already mounted in 'foo'

@ntorresalberto ntorresalberto changed the title Lexically normal mount paths (fix #3733) Some fixes to mounting path validation (fix #3733) Jan 30, 2025
@georgeliao
Copy link
Contributor

@ntorresalberto
Thank you for your contribution.
We’ll review the code once we have some time to dedicate to it.

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

Successfully merging this pull request may close these issues.

2 participants