Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider tight up created file permission in file_android.cc. #37156

Closed
loongliu opened this issue Jun 4, 2019 · 5 comments
Closed

Consider tight up created file permission in file_android.cc. #37156

loongliu opened this issue Jun 4, 2019 · 5 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-io

Comments

@loongliu
Copy link

loongliu commented Jun 4, 2019

The default created file permission for android is 0666 in thiscode. Others user can read and write this file even it is in in internal storage.

We should change permission to 0660, or provide a method to let developer choose file permission.

@devoncarew devoncarew added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-io labels Jun 6, 2019
@devoncarew
Copy link
Member

cc @zanderso as the author of that line, and @rmacnak-google and @aam as recent contributors to the file

@zanderso
Copy link
Member

zanderso commented Jun 6, 2019

That seems reasonable to me. /cc @sortie

Various related issues:
#14467
#22036
#22037

@sortie
Copy link
Contributor

sortie commented Jun 7, 2019

Mode 666 (octal) is generally a reasonable permission parameter to the open(2) system call because of a concept called the process umask on Unix systems. The umask is normally set to something like 022 or 027, and it removes bits from newly created files. For instance, if you have umask 027 and call the open("foo", O_WRITE | O_CREAT, 0666) system call, then it removes the 0027 bits from the requested 0666 mode, and instead creates it 0640.

This mechanism is mandatory and there's no way to override it on a file creation, the permissions can only be changed to be more permissive after the file has been created. This is how Unix command programs know which modes to use, because they simply don't care and create files mode 666 (if not executable) or 777 (if executable) and rely on the umask, or they are extra careful and creates files 600 or 700.

We can test this with a Dart program:

import 'dart:io';

main() {
  new File("test").writeAsStringSync("test\n");
}

Then we can test it in a shell:

sortie@sortie ~ $ umask 0000 && dart test.dart && ls -l test && rm test
-rw-rw-rw- 1 sortie primarygroup 5 Jun  7 11:40 test
sortie@sortie ~ $ umask 0022 && dart test.dart && ls -l test && rm test
-rw-r--r-- 1 sortie primarygroup 5 Jun  7 11:41 test
sortie@sortie ~ $ umask 0027 && dart test.dart && ls -l test && rm test
-rw-r----- 1 sortie primarygroup 5 Jun  7 11:41 test
sortie@sortie ~ $ umask 0077 && dart test.dart && ls -l test && rm test
-rw------- 1 sortie primarygroup 5 Jun  7 11:41 test
sortie@sortie ~ $ 

So far so good. This means you can control the default file mode for all your newly created files when using the Dart VM directly. It will respect the umask setting of your account and any session specific changes you make to it.

However, Dart offers no way to change the umask. This issue seems to be about android, in which case I'm thinking e.g. Flutter. I'm not sure if Flutter or Android exposes any method to override the umask. Dart certainly doesn't. Does anyone know what the default umask one gets on android is? Is there a potential problem where the data of apps is not properly isolated this way? I would hope Android was properly isolating apps.

The other case is that there's no way of requesting a particular file mode (subject to umask, of course) when creating files in Dart, like there is in the open(2) Unix system call. We could enhance Dart with that ability, although it would be a breaking change to add a new named parameter to class File's create method.

We could enhance Dart to expose the umask value. Although umask is a process wide property, but dart implements isolates using threads in the same process, and we might want to simulate a different umask for each isolate.

A complication in exposing these details is that they're Unix specific, other systems like Windows have very different concepts.

The primary action item here is to identify whether the default file mode on Android is bad. Can you do some testing to find out? Logging into a shell on my android phone, I get a umask of 000, but apps are probably running in a very different context. Can you try the above below test code in e.g. a Flutter app and see what you get?

import 'dart:io';

main() {
  final file = new File("test");
  file.writeAsStringSync("test\n");
  print(file.statSync().modeString);
}

However, the file may reside in a app specific directory that only the app's user can access, in which case the file modes inside that directory doesn't really matter.

@loongliu
Copy link
Author

Actually, android set umask 0777.
I checked four different devices, the created file model is 600, looks good.
Seems I should close this issue.

Our security checker reported me an issue that they found the created file mode is 666, on Nexus 5, Android 5.1.1. I will check the security checker running environment, to find where the problem happened.

@loongliu
Copy link
Author

http://androidxref.com/5.0.0_r2/xref/frameworks/base/core/java/com/android/internal/os/ZygoteInit.java#501
provide the source code for setting umask, if anyone needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-io
Projects
None yet
Development

No branches or pull requests

4 participants