sandboxie-plus / Sandboxie

Sandboxie Plus & Classic
https://Sandboxie-Plus.com
GNU General Public License v3.0
13.69k stars 1.52k forks source link

EasyTag and Sandboxie #441

Closed stdedos closed 3 years ago

stdedos commented 3 years ago

I tried to use EasyTag (https://wiki.gnome.org/Apps/EasyTAG) inside the sandbox.

I have tried to alter metadata of F:\Video\Ράδιο Αρβύλα\Bits\2021-01-21 - Ράδιο Αρβύλα - Νούμερο Ένα - Εθνικός Ύμνος.mp4 file (~150 MB). The write operation was always failing.

image I moved the file to a "Direct File Access" directory (E:\Temp). No dice. (I have also tried renaming the file as "a.mp4", it did not make a difference) I saw the notice, and I have moved E:\Temp to Full Access. Success.

All of that happened with no messages (except a "non-fatal" message during the installation process).

On the side: Are you familiar with SandboxDiff? It could be something of interest

typpos commented 3 years ago

All of that happened with no messages

Not sure if this helps, but if you install and run EasyTag outside sandboxie and edit an mp3 file that has the read-only file flag set, then the save-action silently "succeeds" but nothing is saved, so it's possibly nothing to do with sandboxie.

stdedos commented 3 years ago

It has a big fat error, with red letters and everything - so that's not it

All of that happened with no messages With this, I meant no Sandboxie messages.

NewKidOnTheBlock commented 3 years ago

a) I don't understand why you'd want to run a tagging program in a sandbox. To actually do its work, the tagger needs to be able to write to the files. That means you end up with a ton of copies of all the tagged files in your sandbox.

b) You'd wanna give SB access to the folder where the files are located: F:\Video\Ράδιο Αρβύλα\Bits\

c) The Greek letters might be the problem.

stdedos commented 3 years ago

I am not sure why anything of the above is a problem. While non-standard, agreed, there should be a file modified inside the sandbox.

Greek letters do not seem to be the problem in e.g. Notepad: image

hg421 commented 3 years ago

When you modify an existing file from an sandboxed program, the migration size limit might be a problem: screenshot It is supposed to limit the size of files which are copied into the sandbox when they are opened with write access. Although there should be a message when a file exceeds the limit (causing the file to be opened read-only), maybe it's worth a try to raise the limit above the ~150MB of the file where the problem occured?

stdedos commented 3 years ago

Limit at 510978 kb (499.00 MB)

hg421 commented 3 years ago

Ok, I think I have found the cause for this problem.

I installed EasyTag and could reproduce the error using a (very small) mp4 file. Tracing through the API calls showed that the file simply wasn't migrated into the sandbox, so that the subsequent open call failed because the file did not exist inside the box.

Then I dug through the code related to the file migration mechanism, and eventually found this interesting snippet buried deep inside the hooked NtCreateFile routine in SbieDll: https://github.com/sandboxie-plus/Sandboxie/blob/9e5ddd44fba8cc78f7f9455d0714a979ead3bdb1/Sandboxie/core/dll/file.c#L2867-L2898 So apparently certain file types, including .mp4 files, are deliberately never copied into the sandbox, but forced to appear read-only for sandboxed processes instead. Probably this is because some applications needlessly opened media files with write access, thus copying them into the sandbox, and it was assumed that modifying media files would never be needed. So if you now try to do that...

As a fix we could of course just remove that special case handling (which is really ubiquitous all over the source code, coming from the pre-open source days), or maybe even better make it configurable on a per-box basis. That would enable more fine grained control over which files can or cannot be migrated, instead of only having the option to block by file size as we have now.

DavidXanatos commented 3 years ago

LOL... yea so what do we do... its definetly a shitty workaround and counter intuitive... I'd say I'll disable this behavior by default and leave an ini option in to re enable it

stdedos commented 3 years ago

Or .... export _ReadOnlyFileTypes as an ini configuration option :-D

I'd say it's worth it to issue a SBIE message too though, just in case ...

NewKidOnTheBlock commented 3 years ago

Hmm. You are gonna be in a world of pain if you run all your Metatag Editors - or, even better - File Managers in Sandboxie. For fun I tried to run Tag&Rename (an mp3 tag editor) in Sandboxie: tagger

stdedos commented 3 years ago

True, and it makes no sense; however, it makes even less sense to "magically" block an otherwise "ready" file with a fake error "File is lock by application(s): unknown application"

DavidXanatos commented 3 years ago

this is really messy, in File_MigrateFile_ShouldBypass is an other list of files that should not be migrated. the difference is that the first list results in the operation to fail -> STATUS_ACCESS_DENIED while the second successfully opens but only for reading so a later write attempt will fail.

I'll merge that down to one list that is ini defined, and does nto contain media files by default, with an other ini setting to define if it should be access denided or access but for reading only

DavidXanatos commented 3 years ago

also btw there is a 3rd list of fiels to be "migrated" but empty so great even mroe cases to differentiate

stdedos commented 3 years ago

Or avoid the pain, and issue a message.

In a github/wiki, you can add the workarounds (Full Access or rename extension if possible).

I guess, however, user will have to restart the sandbox (ie no Live Reload of "Full Access" list)

hg421 commented 3 years ago

I'll merge that down to one list that is ini defined, and does nto contain media files by default, with an other ini setting to define if it should be access denided or access but for reading only

Yes I think that's a good idea to move the hardcoded rules to the ini file. (This also makes it possible to set them by Template rules)