tauri icon indicating copy to clipboard operation
tauri copied to clipboard

fix(core): fix check temp path permission on mac os, fix #6256

Open 0x-jerry opened this issue 1 year ago • 7 comments

close #6256

0x-jerry avatar Apr 27 '24 02:04 0x-jerry

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.

tweidinger avatar Apr 30 '24 05:04 tweidinger

@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.

0x-jerry avatar Apr 30 '24 06:04 0x-jerry

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 https://github.com/tauri-apps/tauri/blob/44e3335da8c1a811a2ebe72f2036dfad2381da24/core/tauri/src/api/path.rs#L296 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

lucasfernog avatar May 25 '24 15:05 lucasfernog

Thanks for the guidance, done!

0x-jerry avatar May 26 '24 03:05 0x-jerry

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

https://github.com/tauri-apps/tauri/blob/44e3335da8c1a811a2ebe72f2036dfad2381da24/core/tauri/src/api/path.rs#L296

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.

0x-jerry avatar May 26 '24 04:05 0x-jerry

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.

tweidinger avatar May 29 '24 06:05 tweidinger

Done!

0x-jerry avatar May 29 '24 11:05 0x-jerry