Guard against incorrect paths

[grzesiek2010]

Problem description

In PR #6340, we introduced a check to determine if paths were pointing to locations within the provided dirPath. This was further refined in PR #6357, and eventually, in PR #6363, the check was partially disabled, logging a non-fatal event instead of throwing an exception.

Thanks to PR #6363, we identified an issue with the original path comparison. Occasionally, after calling getCanonicalPath, the case of some letters in the path would change, such as: /storage/emulated/0/android/data/org.odk.collect.android/files/projects/ instead of /storage/emulated/0/Android/data/org.odk.collect.android/files/projects/.

What we need to do

1. Revert the code from the androidshared module back to the shared module: https://github.com/getodk/collect/pull/6363/commits/8f05e2813bf24faf00606ea79104fd7b4a533d4f
2. Reintroduce the SecurityException throw: https://github.com/getodk/collect/pull/6363/commits/f9377c42ca43ebd1f4ad243393aa2d448ce996de#diff-a8a1c2c9a690d0a3c02bd93865b8ee78dc4fea28e39dc1b59adf60437c71257eL14
3. Enhance path comparisons by making them case-insensitive: https://github.com/getodk/collect/pull/6363/commits/f9377c42ca43ebd1f4ad243393aa2d448ce996de#diff-a8a1c2c9a690d0a3c02bd93865b8ee78dc4fea28e39dc1b59adf60437c71257eR14

[grzesiek2010]

This is an issue with the paths returned by #getCanonicalPath. While the original paths start with storage/emulated/0/Android/data/ (the correct path), the paths returned by #getCanonicalPath may begin with variations like storage/emulated/0/android/data/ or storage/emulated/0/Android/Data/.

To address this while maintaining the case sensitivity of the Android OS, we can:

1. Check if the path returned by #getCanonicalPath starts with storage/emulated/0/Android/data/, ignoring case.
2. If step 1 returns true, update the canonical path to start with the correct format: storage/emulated/0/Android/data/ and use.
3. If step 1 returns false, throw an error, as this may indicate the potential vulnerability we were originally concerned with.

@lognaturel @seadowg if you have any thoughts please share.

[lognaturel]

When you say "and use" in step 2, you mean and use to do the rest of the path check, right? If so, that sounds right to me.

I think we should also write a stackoverflow post about this unexpected inconsistency in case so at least there's a record of it. I was going to do it but I think you should, @grzesiek2010!

I think it should point to the Android docs (https://developer.android.com/privacy-and-security/risks/path-traversal and https://developer.android.com/reference/java/io/File#getCanonicalPath()), say what we experienced when we did something like that (what you described above), and ask whether this is expected and if there's another recommended approach.

[grzesiek2010]

When you say "and use" in step 2, you mean and use to do the rest of the path check, right?

Yes, I was considering something like this:
https://github.com/getodk/collect/commit/c243c48bf0d513fd4648f85c0680ce07a6252bf5

There are thousands of reports, and it's not feasible to review even 1% of them carefully. However, it seems that only the segment of the path containing /Android/data/ might vary, potentially appearing as /android/data/ or /android/Data/. Therefore, I plan to sanitize this segment.

For the beta release, there won’t be any major changes. I aim to avoid exceptions that could crash the app. My goal is simply to determine if sanitizing the /Android/data/ segment is sufficient.

[lognaturel]

That sounds great.

Are you up for writing the Stackoverflow post? The primary goal is to document this behavior in a way that's easy to find. A secondary goal would be to get an alternative workaround or assurance that what you're planning makes sense.

[grzesiek2010]

Yes, I can do that but my plan was to do that once we agree on my solution so that I can link the post in the comment above the fix and the other way around show on stackovrflow what's the problem and how we deal with it.

[seadowg]

@lognaturel @grzesiek2010 what's the status of this? I think the last update I had is that we'll keep collecting data and look to fix as part of the next release. Is that still correct?

[grzesiek2010]

I have implemented sanitizing https://github.com/getodk/collect/pull/6404 to check if it is just about /Android/data/ part of the path or maybe there is something else that might be wrong. If we don't see any reports that means we can close the issue. But other segments of the path also might cause problems so we will see.

[seadowg]

If we don't see any reports that means we can close the issue. But other segments of the path also might cause problems so we will see.

By reports do we mean crashes here? I think if so we should close this and then can reopen and/or file bugs if they come up after release.

[grzesiek2010]

Those are not crashes we log errors but they do not crash the app.

[seadowg]

Ah right! So the next release will just update our logging not fix the actual problem? If that's the case I can just move this to v2024.4.

[grzesiek2010]

Ah right! So the next release will just update our logging not fix the actual problem?

If the problem was just with /Android/data/ part of the path (which could be /android/data/or /android/Data/ after calling getCanonicalPath) then it is fixed because https://github.com/getodk/collect/pull/6404 sanitizes that. However, we can't be sure if that's the only problem. We have thousands of errors reported so I'm not able to review all of them. I think I have reviewed ~100 of them and I saw that the problem was only with /Android/data/ but I can imagine the same could happen with other segments of the path as well.

[lognaturel]

If we confirm that the case change does not result in false reports of mismatches, we will switch to an exception in a point release so that we can call this fully done, I believe.