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(core): fix check temp path permission on mac os, fix #6256 #9588

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

0x-jerry
Copy link

close #6256

@0x-jerry 0x-jerry requested a review from a team as a code owner April 27, 2024 02:52
@0x-jerry 0x-jerry marked this pull request as draft April 27, 2024 02:52
@0x-jerry 0x-jerry force-pushed the 1.x branch 3 times, most recently from fe729b9 to e170395 Compare April 27, 2024 03:27
@0x-jerry 0x-jerry marked this pull request as ready for review April 27, 2024 03:37
@FabianLars FabianLars requested a review from a team April 27, 2024 08:04
@tweidinger
Copy link
Contributor

tweidinger commented Apr 30, 2024

Will check this PR in the upcoming days but am wondering if https://github.com/tauri-apps/tauri/pull/9072/files#diff-903ee43f2eb846686e5baf219fc515ff00de27369cef1e81b37164de847d2e90 does fix the underlying issue, as it resolves symlinks now before checking. This is in dev, so not sure if it can/is/will be back ported and if other changes influence behavior.

@0x-jerry
Copy link
Author

0x-jerry commented Apr 30, 2024

@tweidinger I tested how path::is_symlink works, and seems it only checks the exact symbolic path, eg.

use std::{
    env,
    path::PathBuf,
};

fn main() {
    let p_var = PathBuf::from("/var");

    println!("{:?}", p_var.is_symlink()); // => true

    let p_var = env::temp_dir();

    println!("{:?}", p_var.is_symlink()); // => false
}
image

This means any subpath of a symbolic folder will return false, so https://github.com/tauri-apps/tauri/pull/9072/files#diff-903ee43f2eb846686e5baf219fc515ff00de27369cef1e81b37164de847d2e90 may not be able to fix subpath permission check.

@lucasfernog
Copy link
Member

The issue happens because the allowed path is actually a symlink so the full path does not match the path we're checking against (which is canonicalized so it resolves to the actual folder).

There is an easier way to fix this which is to canonicalize the temp dir when resolving the path variables.
basically changing

BaseDirectory::Temp => Some(temp_dir()),
to BaseDirectory::Temp => temp_dir().canonicalize().ok()

it's probably more secure this way since we would still check canonicalized paths in the end, resolving symlinks and path traversals

cc @tweidinger

@0x-jerry 0x-jerry force-pushed the 1.x branch 2 times, most recently from 1209298 to 106f612 Compare May 26, 2024 03:32
@0x-jerry
Copy link
Author

Thanks for the guidance, done!

@0x-jerry
Copy link
Author

The issue happens because the allowed path is actually a symlink so the full path does not match the path we're checking against (which is canonicalized so it resolves to the actual folder).

There is an easier way to fix this which is to canonicalize the temp dir when resolving the path variables. basically changing

BaseDirectory::Temp => Some(temp_dir()),

to BaseDirectory::Temp => temp_dir().canonicalize().ok()
it's probably more secure this way since we would still check canonicalized paths in the end, resolving symlinks and path traversals

cc @tweidinger

When the user creates a temporary file, the js code should be like this:

import { fs, path } from '@tauri-apps/api'
import { tempdir } from '@tauri-apps/api/os'

const tempFile = await path.join(await tempdir(), 'not-exits-file')

await fs.writeFile(tempFile, 'some data')

it will use os.tempdir() to get the temporary path, when writing the file, the tempFile should not exist, so check permission will only check the path, not the canonicalized path, this will fail because os.tempdir() is not canonicalized, while the permission check is checking the canonicalized temporary path.

So, we may also change os.tempdir() API, and make it return the canonicalized temporary path.

@lucasfernog lucasfernog requested a review from tweidinger May 28, 2024 10:55
tweidinger
tweidinger previously approved these changes May 29, 2024
@tweidinger
Copy link
Contributor

LGTM. The only blocker is that not all commits are signed @0x-jerry could you please amend signatures to previous commits or rewrite to a single commit? The history will be squashed into a single commit anyway.

@0x-jerry
Copy link
Author

Done!

@lucasfernog lucasfernog merged commit 8ee8f09 into tauri-apps:1.x Jun 4, 2024
19 checks passed
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.

3 participants