Temp directory improvments

[stevenmcdonald]

We recently had a security audit conducted on our code, we have a fork of IPtProxy called IEnvoyProxy (thank you, your code has been a big help), and they called out a Low severity issue with the temp file location used by IEnvoyProxy/IPtProxy. This is how I fixed in our code, using os.MkdirTemp

I'm still kind of new to Go and mobile dev, but this looks like the right way to do it in 2023. os.MkdirTemp was added in go 1.16, and it looks like that's the minimum version required by IPtProxy, so that shouldn't be a problem.

[tladesignz]

Thank you, very interesting! Are you using this both on Android and iOS?

[stevenmcdonald]

We're only using it on Android. I haven't done anything to disable iOS support in our fork, but we're not actively using it. So, I haven't tested it on iOS, but I think this code should work for both.

[tladesignz]

@bitmold, can you review this for Android?

[bitmold]

Yes, I'll test it later today with Orbot ...

[bitmold]

In my test app calling IPtProxy.setStateLocation like this caused the thing to bug out:

        val stateLocation = File(cacheDir, "pt")
        if (!stateLocation.exists()) stateLocation.mkdir()
        IPtProxy.setStateLocation(stateLocation.absolutePath)

Historically this is how that method was called. Looking into why this may be and how y'all call this method on your end ......

[bitmold]

My mistake, I realize now that you are using the temp directory from your patch and not the return value of the method from Android's Context class...

I'm getting some crashes when invoking IPtProxy functions with my local build of your patch applied on top of the newest code from this repository (cherry-picked your modifications to the init() function onto the HEAD of this repo...

I will post here when I get somewhere figuring out what's going on here, it's very likely the problem could be something dumb on my end ..........,

[bitmold]

The more I think this through @stevenmcdonald and @tladesignz I actually can't come up with one good reason to have a default temp directory for pluggable transports on android...

I think Android apps integrating with IPtProxy ought to use the temp value returned by Context.getCacheDir() which looks something like this: /data/user/0/your.package.name/cache/pg_state There are important benefits to this:

• the system cleans up this directory
• the moment users delete the app all the cache data is deleted. I think this point is especially important when designing software that circumvents the censorship programs of authoritarian governments (which is criminal). It's a reasonable expectation that when you delete the app all the data that the app had should go away with it.

But, at least on Android, this goes much deeper too...

Your switch to go's os.MakedirTemp with the empty string as a parameter invokes a second call to go's os.TempDir() - which on UNIX returns $TMPDIR and on Android that's supposed to resolve to /data/local/tmp which is what we had before. Therefore all using os.MakedirTemp does is provide a randomized path under this temp directory instead of a fixed one. But there are problems with this apporach too:

• The "temp" directory will outlive the app after uninstall
• Each time the app process is started a new "temp" directory will be created with a randomized name alongside all of the previously existing ones. Which means disk space will progressively be eaten up by running pluggable transports.
• And the real kicker is that on Android, /data/local/tmp seems to actually persist across reboots!!! see the multiple comments from 2020 on this post https://android.stackexchange.com/questions/220504/what-and-when-clears-content-of-data-local-tmp As the doc string says: "It is the caller's responsibility to remove the directory when it is no longer needed."

@stevenmcdonald I'm curious what your auditor said about the previous use of /data/local/temp.

I think the only sensible thing to do if the default sucks is to get rid of the default. A new release of IPtProxy can be made that demands the user set the state location explicitly instead of having all of these problems exist. It seems like you're doing something similar on iOS too:

let ptDir = FileManager.default.containerURL(forSecurityApplicationGroupIdentifier: "group.com.example.app")?
  .appendingPathComponent("pt_state")

IPtProxy.setStateLocation(ptDir?.path)

[bitmold]

Also @stevenmcdonald - unless your modifications to this library with the additional transports that you added use StateLocation in some way then you especially don't need this code at all.

As it stands right now, all of IPtProxy's use of that value is through the use of obfs4proxy which you were saying you don't even use in your fork.

[tladesignz]

@bitmold, thank you for this very deep analysis!

The ultimate goal here should provide as much safety and security as possible. Therefore, the default behaviour should be, to use a directory inside the containing app which gets deleted again on uninstall.

For iOS, this expected behaviour is currently, what's happening.

TMPDIR is specifically set for every app to point into an app container which gets cleaned up properly.[^1]

In that case, I actually don't want the directory to be of random name, so transports can reuse state which was acquired in an earlier session.

@stevenmcdonald, do you see any benefit with the directory name randomization besides avoiding reuses between different apps in a situation where the root temp dir is in a shared location?

@bitmold, you say that TMPDIR on Android points to /data/local/tmp, but why the heck did we need this code in the first place, then?

    if runtime.GOOS == "android" {
        StateLocation = "/data/local/tmp"
    } else {
        StateLocation = os.Getenv("TMPDIR")
    }

@bitmold, is there any way to provide a safe default on Android (meaning getting a directory which is inside the app container) from within Go?

Or do we really need the default to be null on Android to force people to provide this before usage?

[^1]: The example of me setting the directory explicitly, you dug out here, @bitmold, is a special case in Orbot iOS, where I actually want to share that directory between the main app and the Network Extension. For normal apps, the provided default is perfectly good.

[bitmold]

you say that TMPDIR on Android points to /data/local/tmp, but why the heck did we need this code in the first place, then?

I'm not sure. Maybe it's possible on some Android build TMPDIR points to somewhere else? But by and large it seems to point to d/l/t

is there any way to provide a safe default on Android (meaning getting a directory which is inside the app container) from within Go?

I'll look into this later but I'm inclined to say probably not? On Android each process is the apps package name itself however you don't necessarily know where on the disk (or disks - there could be SD cards too) the app is being run from. Maybe it'd be trivial to find this out in Go and build the right file path but this seems like something messy that might break somewhere and goes against the grain. I'll look into it later though.

Here's an idea: since only obfs4proxy uses this information why not just make the cache location an argument to the start obfs4proxy function?

Also why does obfs4proxy seemingly need to log to a file on the disk? No one seems to bat an eye that snowflake for instance logs to the console ...

[bitmold]

Hmm my very last comment i think misses something. I realize it's more than just logs it's state too...

[tladesignz]

I'll look into this later but I'm inclined to say probably not? On Android each process is the apps package name itself however you don't necessarily know where on the disk (or disks - there could be SD cards too) the app is being run from. Maybe it'd be trivial to find this out in Go and build the right file path but this seems like something messy that might break somewhere and goes against the grain. I'll look into it later though.

I was afraid you would say that.

Here's an idea: since only obfs4proxy uses this information why not just make the cache location an argument to the start obfs4proxy function?

Meeeh. It's part of the Pluggable Transports API. In theory, there could be a change to goptlib (which encapsulates most of it) which suddenly makes this really necessary. Or a change in Snowflake itself, of course.

So, yes, it seems hardly used, but we're breaking the API, if we don't provide it.

[stevenmcdonald]

@tladesignz

do you see any benefit with the directory name randomization besides avoiding reuses between different apps in a situation where the root temp dir is in a shared location?

I don't know what the PTs are doing with their temp dir, but I think in general the worry is that another app could read or modify sensitive information in the temp dir... but I think it was a generic warning from our auditor, I can share the specific parts of the report here or privately

I'm not certain that any of our code needs that setting, but it seemed like such a simple change it was easier to update the that code than check if it was used, TBH.

[bitmold]

Well @tladesignz our hands are, in fact, tied. There's no way to correctly get that information 100% of the time in go code and even if you could it would be ugly reflexive code. Orbot already was calling IPtProxy.setStateLocation and that's good because otherwise we'd have had problems.

But ya, going forward I think, on Android, we need to have StateLocation be nil and in every function that depends on that value do a nil check and throw an error or something. For now I think this can just be the startObfs4Proxy function. I can write this if you want? Let me know ...

By the way, now that I'm reading pt-spec.txt I'm kinda confused why this data is being written to somewhere temporary in the first place:

Specifies an absolute path to a directory where the PT is allowed to store state that will be persisted across invocations. The directory is not required to exist when the PT is launched, however PT implementations SHOULD be able to create it as required.

   PTs MUST only store files in the path provided, and MUST NOT
   create or modify files elsewhere on the system.

Since, in practice, it's so far only been used or obfs4proxy logs the persistent aspect of this state location thing doesn't seem like it's mattered that much, but as you say, maybe down the line snowflake or a different transport altogether wants to use StateLocation and needs the directory to be persistent.

[tladesignz]

Ok. @stevenmcdonald, thank you for bringing this up and all your input! Also, @bitmold for the huge research in-depth!

I will not merge this PR. Instead, I changed the API: 5f4f242613149979c04cdbc85185101b80f6e7d2 A valid StateLocation will be enforced instead of providing unsafe defaults.

Because of breaking the API, IPtProxy will move to version 2.0.0.