[m-rev.] for review: secure temporary file and directory creation for Java
Julien Fischer
jfischer at opturion.com
Tue Mar 28 14:35:38 AEDT 2023
Hi,
In the absence of a review, I shall commit this one this evening.
Julien.
On Sat, 25 Mar 2023, Julien Fischer wrote:
>
> For review by anyone.
>
> Note that the code is essentially what we have had in the extras distribution
> for years, so it's really only the documentation that
> needs review. Also, with this change the Suffix argument to
> make_temp_directory/6 is now ignored by all of our backends,
> so we should probably introduce a version that doesn't have it.
>
> ---------------------------------------------------------------
>
> Secure temporary file and directory creation for Java.
>
> In the Java grade, the implementation of predicates used to create unique
> temporary files and directories was not secure, due to the underlying Java
> API
> methods not setting permissions correctly. Alternative methods were added in
> Java 7, but we currently use the older insecure methods. This change switches
> the Java implementations of temporary file and directory creation over to the
> newer methods.
>
> We have long provided a Mercury wrapper for secure temporary file / directory
> creation in the extras/java_extra_libs. That will be redundant after this
> change
> and I will remove it in a separate change.
>
> library/io.file.m:
> Use the secure temporary file and directory creation methods introduced
> in Java 7.
>
> Mention that for make_temp_directory/6, the Java implementation will
> now
> ignore the Suffix argument.
>
> Delete some that was used to generated temp file names; its use of
> java.util.Random probably wasn't particularly secure either.
>
> Julien.
>
> diff --git a/library/io.file.m b/library/io.file.m
> index ed464a7..6de7b4c 100644
> --- a/library/io.file.m
> +++ b/library/io.file.m
> @@ -2,7 +2,7 @@
> % vim: ft=mercury ts=4 sw=4 et
> % ---------------------------------------------------------------------------%
> % Copyright (C) 1993-2012 The University of Melbourne.
> -% Copyright (C) 2013-2022 The Mercury team.
> +% Copyright (C) 2013-2023 The Mercury team.
> % This file is distributed under the terms specified in COPYING.LIB.
> % ---------------------------------------------------------------------------%
> %
> @@ -128,10 +128,6 @@
> %
> % The file is placed in the directory returned by get_temp_directory/3.
> %
> - % On the Java backend, this does not attempt to create the file
> - % with restrictive permissions (600 on Unix-like systems) and therefore
> - % should not be used when security is required.
> - %
> :- pred make_temp_file(io.res(string)::out, io::di, io::uo) is det.
>
> % make_temp_file(Dir, Prefix, Suffix, Result, !IO) creates an empty
> % file
> @@ -154,18 +150,12 @@
> % - Prefix is ignored.
> % - Suffix is ignored.
> %
> - % On the Java backend, this does not attempt to create the file
> - % with restrictive permissions (600 on Unix-like systems) and therefore
> - % should not be used when security is required.
> - %
> :- pred make_temp_file(string::in, string::in, string::in,
> io.res(string)::out,
> io::di, io::uo) is det.
>
> % make_temp_directory(Result, !IO) creates an empty directory whose
> % name
> % is different from the name of any existing directory.
> %
> - % On the Java backend this is insecure as the file permissions are not
> set.
> - %
> :- pred make_temp_directory(io.res(string)::out, io::di, io::uo) is det.
>
> % make_temp_directory(ParentDirName, Prefix, Suffix, Result, !IO)
> % creates
> @@ -183,7 +173,8 @@
> % - Prefix is ignored.
> % - Suffix is ignored.
> %
> - % On the Java backend this is insecure as the file permissions are not
> set.
> + % The Java backend has the following limitation:
> + % - Suffix is ignored.
> %
> :- pred make_temp_directory(string::in, string::in, string::in,
> io.res(string)::out, io::di, io::uo) is det.
> @@ -1268,9 +1259,6 @@ make_temp_file(Dir, Prefix, Suffix, Result, !IO) :-
> [will_not_call_mercury, promise_pure, tabled_for_io, thread_safe,
> may_not_duplicate],
> "
> - // Java 7 java.nio.file.Files.createTempFile() would allow us
> - // to set the file mode.
> -
> if (Prefix.length() > 5) {
> // The documentation for io.make_temp says that we should only use
> // the first five characters of Prefix.
> @@ -1278,16 +1266,11 @@ make_temp_file(Dir, Prefix, Suffix, Result, !IO) :-
> }
>
> try {
> - File new_file = new File(DirName, makeTempName(Prefix, Suffix));
> - if (new_file.createNewFile()) {
> - FileName = new_file.getAbsolutePath();
> - Error = null;
> - } else {
> - FileName = """";
> - // Any other errors should be be reported by createNewFile() by
> - // throwing an exception.
> - Error = new java.io.IOException(""File already exists"");
> - }
> + java.nio.file.Path dir_path = java.nio.file.Paths.get(DirName);
> + java.nio.file.Path new_file =
> + java.nio.file.Files.createTempFile(dir_path, Prefix, Suffix);
> + FileName = new_file.toAbsolutePath().toString();
> + Error = null;
> } catch (java.lang.Exception e) {
> FileName = """";
> Error = e;
> @@ -1398,26 +1381,24 @@ make_temp_directory(ParentDirName, Prefix, Suffix,
> Result, !IO) :-
> ").
>
> :- pragma foreign_proc("Java",
> - do_make_temp_directory(ParentDirName::in, Prefix::in, Suffix::in,
> _Sep::in,
> - DirName::out, Error::out, _IO0::di, _IO::uo),
> + do_make_temp_directory(ParentDirName::in, Prefix::in, _Suffix::in,
> + _Sep::in, DirName::out, Error::out, _IO0::di, _IO::uo),
> [will_not_call_mercury, promise_pure, tabled_for_io, thread_safe,
> may_not_duplicate],
> "
> if (Prefix.length() > 5) {
> - // The documentation for io.make_temp says that we should only use
> - // the first five characters of Prefix.
> + // The documentation for io.make_temp_directory says that we should
> + // only use the first five characters of Prefix.
> Prefix = Prefix.substring(0, 5);
> }
>
> try {
> - File new_dir = new File(ParentDirName, makeTempName(Prefix,
> Suffix));
> - if (new_dir.mkdir()) {
> - DirName = new_dir.getAbsolutePath();
> - Error = null;
> - } else {
> - DirName = """";
> - Error = new java.io.IOException(""Error creating directory"");
> - }
> + java.nio.file.Path parent_dir_path =
> + java.nio.file.Paths.get(ParentDirName);
> + java.nio.file.Path new_dir =
> + java.nio.file.Files.createTempDirectory(parent_dir_path,
> Prefix);
> + DirName = new_dir.toAbsolutePath().toString();
> + Error = null;
> } catch (java.lang.Exception e) {
> DirName = """";
> Error = e;
> @@ -1487,40 +1468,6 @@ using System.Security.Principal; // For
> IdentifyReference etc.
> #endif
> ").
>
> -:- pragma foreign_decl("Java", local, "
> -import java.io.File;
> -import java.io.IOException;
> -import java.util.Random;
> -").
> -
> -:- pragma foreign_code("Java", "
> - public static Random ML_rand = new Random();
> -
> - public static String makeTempName(String prefix, String suffix)
> - {
> - StringBuilder sb = new StringBuilder();
> -
> - sb.append(prefix);
> - // Make an 8-digit mixed case alpha-numeric code.
> - for (int i = 0; i < 8; i++) {
> - char c;
> - int c_num = ML_rand.nextInt(10+26+26);
> - if (c_num < 10) {
> - c_num = c_num + '0';
> - } else if (c_num < 10+26) {
> - c_num = c_num + 'A' - 10;
> - } else{
> - c_num = c_num + 'a' - 10 - 26;
> - }
> - c = (char)c_num;
> - sb.append(c);
> - }
> - sb.append(suffix);
> -
> - return sb.toString();
> - }
> -").
> -
> % ---------------------%
>
> get_temp_directory(Dir, !IO) :-
>
More information about the reviews
mailing list