Attempting to fcopy() a temporary savefile results in a safety check.

ID:2424469
 
Jan 26 2019, 5:46 am
BYOND Version:512.1463
Operating System:Windows 10 Pro 64-bit
Web Browser:Chrome 70.0.3538.102
Applies to:Dream Seeker
Status: Open
Issue hasn't been assigned a status value.
Descriptive Problem Summary:
If you create a new savefile, but leave out the filename, you will get a temporary savefile that gets deleted as soon as the reference is gone. The problem is copying the file. If you try to use fcopy() on the temporary savefile, you will be greeted with an ugly safety check popup, since the file is technically outside of the game's directory, even though the game created it and already has full access to it.

Numbered Steps to Reproduce Problem:
1. Create a new project and run the following code:

Code Snippet (if applicable) to Reproduce Problem:
mob
    verb
        safety_check()
            var/savefile/f = new
            f << "thing"

            fcopy(f, "newsave.sav")
            var/savefile/f2 = new("newsave.sav")

            var/val
            f2 >> val
            src << val

        no_safety_check()
            var/savefile/f = new
            f << "thing"

            var/savefile/f2 = new("newsave.sav")
            f2.ImportText(null, f.ExportText())

            var/val
            f2 >> val
            src << val

2. Click on the verbs and notice how one results in a safety check, and the other doesn't, even though they are both accomplishing the same thing. Unfortunately, the one without the safety check is likely to be much less efficient.


Expected Results:
Both verbs behave exactly the same.

Actual Results:
One has a safety check and the other doesn't.

Does the problem occur:
Every time? Or how often?
Every time.
In other games?
The presence of an actual hub might change the level of access the game has to files in the cache, but I don't know.
In other user accounts?
Probably.
On other computers?
Probably.

When does the problem NOT occur?
I don't think it will occur in trusted mode. It won't occur if the savefile you are copying is a permanent file rather than a temporary one.

Did the problem NOT occur in any earlier versions? If so, what was the last version that worked? (Visit http://www.byond.com/download/build to download old versions for testing.)
I have no idea.

Workarounds:
Use ExportText() and ImportText() like in the example above.
Jan 26 2019, 12:47 pm
This isn't technically a bug, things are working as intended. BYOND gives a little special access to handling cached stuff internally, but the sandbox doesn't really want developers touching that same stuff themselves outside of the actions that cause things to be stored there. (Unless the host trusts the game enough to run in trusted mode)

Once you've created that temporary savefile you're pretty much limited to what's being done internally to handle the temporary reference, which is a temporary file, that file isn't meant to be used beyond that purpose and when you go to copy it, as you said, you're trying to copy a file from the cache which is going to result in a safety check outside of trusted mode for anything.

To avoid it, don't use a temporary cached savefile, make a new named one then delete it afterwards with fdel(). Name it something nonsensical that nobody would ever have a conflict with (that's how temporary files work in general except they use an actual naming scheme standard), no reason you can't handle it yourself fairly easily.

I'd honestly prefer this to stay how it is because there's probably a whole can of worms of security nonsense it would open.
Jan 26 2019, 3:47 pm
In response to Nadrew
Nadrew wrote:
I'd honestly prefer this to stay how it is because there's probably a whole can of worms of security nonsense it would open.

I can see your point but I beg to differ. You are seeing savefiles only as external files, but I am seeing a reference to something more than just a file. A savefile exists as something in between an object and a file. No other file has vars and procs that can be accessed. Clearly this represents a special edge case in which the copy is allowed, because it already is allowed...

From the fcopy proc reference page:
Src may be either a cache file, a savefile, or the name of an external file.

As you can see, there is a clear distinction being made between savefiles and other files. They are similar, yet different, and no file "security" should ever get in the way of copying something that acts like a datum. That would be terribly inconsistent.

You are trying to make this into something it's not, and it's not a "can of worms".
Jan 31 2019, 1:47 pm
I can see both sides of this but I think Nadrew is closer to correct. You're looking at this as copying a datum, but it's not; you're copying a file, which is evident from the fact that you're using the fcopy() command to do it. Since the temporary file isn't in the game directory, the game doesn't have access to it other than the strictly controlled access granted by savefile operations.

One simple way to work around this would be to create the new savefile using a temporary file with a name of your choosing. Then you're not copying from the temp file, but from one you decided to create, and you can fdel() the temporary savefile later.

The big question here is, what are you trying to accomplish when copying the savefile? What's its purpose in your code? Knowing that would help me decide on whether some other kind of handling is needed, like for instance a new savefile proc.
Jan 31 2019, 3:02 pm (Edited on Jan 31 2019, 3:55 pm)
In response to Lummox JR
Lummox JR wrote:
One simple way to work around this would be to create the new savefile using a temporary file with a name of your choosing. Then you're not copying from the temp file, but from one you decided to create, and you can fdel() the temporary savefile later.

Am I misreading that or are you saying that you can create a new savefile from an existing one?

The big question here is, what are you trying to accomplish when copying the savefile? What's its purpose in your code? Knowing that would help me decide on whether some other kind of handling is needed, like for instance a new savefile proc.

This was a land mine I ran into when writing a proc that modifies savefiles. When writing it, I noticed how easy it would be for new developers to trip on this inconsistency, so I reported it. I had to include the workaround mentioned here to ensure that all uses for savefiles are supported. Since the proc is meant to work on existing savefiles, changing the way they are initially created is not an option in that case. For more context, I wrote this proc in response to a post in developer help.

Something like savefile.Copy() would be more useful than the slow workaround used here, but that seems redundant if fcopy() could just be made to do the same exact thing. Why does the method matter if the end result is the same?
Jan 31 2019, 3:27 pm
 
fcopy("savefile.sav","tmpsavefile.sav")
// Do stuff
fdel("tmpsavefile.sav")
Jan 31 2019, 3:31 pm
In response to Nadrew
I'm already doing that. Named savefiles are not the issue.
Jan 31 2019, 3:40 pm
You can name them in any case you have to use them, instead of doing

var/savefile/F = new()


Do

var/savefile/F = new("tmpsavefile.sav")


Then get rid of it when you don't need it anymore.
Jan 31 2019, 3:45 pm
In response to Nadrew
As I mentioned, the way a savefile is created is beyond the scope of what that proc is meant to do. All savefiles must be treated the same. The creation of them is irrelevant.
Jan 31 2019, 3:46 pm
Since the temporary file isn't in the game directory, the game doesn't have access to it other than the strictly controlled access granted by savefile operations.

Under your own argument, allowing temp save files in safe mode at all is a security exploit.

This seems like the bug.

You allow people to write to a temporary file not in the game directory, via temp saves

But you also argue that people should not be allowed to write to a temporary file not in the game directory via temp saves.

it seems like the solutions are:

1: block writing to the temporary directory in safe mode by blocking temp savefiles.

2: move the temporary directory to a subfolder of the game folder.

3: make temp save files exist in memory only.

4: admit this is all silly and just make fcopy allow copying temp save files.
Jan 31 2019, 4:06 pm
In response to MrStonedOne
This is reminiscent of the file permission conflicts in Windows. It's like fcopy() is blind and can't see it, therefore it doesn't exist, even though it really does. That's some quantum level logic right there.
Jan 31 2019, 8:48 pm
In response to MrStonedOne
It's not an exploit since the file handle is already open. It's not like this provides real access to anything.

Adapting the existing security routine to try to figure out if the savefile is a temp and if so to allow it is probably a lot harder than it sounds on the surface. What I'm scratching my head over is why fcopy() was ever allowed to accept a savefile at all.