Two Security Improvements #43
Conversation
iteratee
force-pushed
the
kb/security-improvements
branch
from
d3c5d32
to
68edc9c
Compare
|
Hi @iteratee thanks for the PR. Can you elaborate a bit on how these measures improve security? |
Sure. The first one mirrors the behavior of (mkstemp)[https://www.gnu.org/software/libc/manual/html_node/Temporary-Files.html#index-mkstemp]. Although with the default behavior in Briefly a file is created exclusively, because a reference to that opened file is not returned to the caller, only the path, there is no guarantee that when the caller opens the file, it will be the same file. It's called a (TOCTOU)[https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use] vulnerability. By returning the pid, it is the equivalent to returning the file descriptor in C. The second change mirrors (mkdtemp)[https://www.gnu.org/software/libc/manual/html_node/Temporary-Files.html#index-mkdtemp], but for the temporary directory that briefly creates, or ensures exists, the first time a pid asks for a temporary file. It isn't necessary for security to re-create it every time, it could be done once at startup and managed by the genserver. But the existing code possibly created a new directory for each pid, so I kept that paradigm. The security part of the temporary directory change is that the directory may already exist and be owned by another user, or may be a symlink to some other place. The code now makes sure that the last component of the temporary directory is newly created. This prevents it from being owned by another user. It also changes the mode to 0700. I looked through the erlang source, there isn't a way to pass the mode to the Tracking the temporary directory that a file belongs to and attempting optimistic cleanup isn't strictly necessary for security, but goes along with the idea of Briefly. When we clean up for a pid, we attempt to remove the temporary directory created for that pid. We only want to remove it if it is completely empty. It may not be empty if we have given away a temporary file that is still in use. When cleanup is executed for a pid, if it has gifted files, cleanup attempts to remove the temporary directory for the gifted file. It only does this if the pid that created the file has already been cleaned up. The cleanup of directories uses |
|
@benwilson512 Ping? I noticed I need to adjust the description on one commit. We don't track the pids anymore because they default to closing when the process that opened them closes. |
|
Hi @iteratee thanks for your very helpful explanation. Your thorough test additions are also extremely welcome. Two quick follow up questions:
It occurs to me that one way to solve both of these things is to deprecate Thoughts? |
iteratee
force-pushed
the
kb/security-improvements
branch
from
68edc9c
to
57fa091
Compare
|
Sorry for the long delay. I started down a research rabbit hole of where the "s" in mkstemp came from and what it stands for. I couldn't find any clear sources. I went ahead and implemented your suggestion. Let me know if there are docs that you think need to be updated. |
| end | ||
| end | ||
|
|
||
| defp open(%{directory: true} = options, tmp, attempts, _) when attempts < @max_attempts do |
There was a problem hiding this comment.
If we are going to straight up remove this option and not basically convert it to type: :directory then we need to make this a major version bump since it's a breaking change. We could alternatively note that it is deprecated in the docs, and then convert directory: true to type: :directory in the code and do a minor bump for now, with plans for a major release later.
There was a problem hiding this comment.
I went ahead and added back handling of directory: true and added a test that it was still handled. I changed the documentation that I could find all to be type: :directory, so that if you want to drop directory: true in a major release, you can do that.
Sorry that I missed this.
If the temporary directory is world writable, in order to securely open a temporary file, you need to create it and open it at the same time and have exclusive access to it. It remains secure as long as you hold onto the file descriptor returned by open. Requesting a device returns the io device and the path, rather than just the path. Note that the io device is owned by the pid that called open(), and will be closed when that process exits. There is no way to hand off the device to another pid. If a temporary device needs to be long lived, it needs to be opened by a process that is at least as long lived.
There is no guarantee that the cleanup will have run before the test thread receives the monitor notification. Busy wait watching ETS to ensure that cleanup has had a chance to run.
Just hides one ets usage behind a helper. No other functional changes.
Create a new temporary directory with defined permissions for each pid. Ensuring that the creation is new prevents race conditions where the directory already exists. Ideally we would pass the mode when creating the directory, but that is not possible with the Elixir nor Erlang standard libraries. When cleaning up for a pid, optimistically attempt to delete the temporary directory using rmdir. This won't delete the directory if any files are still there, which occurs if they have been given away. When cleaning up for a pid, look for any temporary directories for files we have been gifted. Remove them if pid in question is no longer using the temporary directory. Allow the application to configure the mode for the temporary directory.
iteratee
force-pushed
the
kb/security-improvements
branch
from
57fa091
to
5435cdf
Compare
First, allow a user to receive an io_device for the newly opened file. This mirrors the behavior of mkstemp in standard libraries.
Second, when creating the base temporary directories, ensure that the directory is newly created. We could do this once in the server, but I did it new for each pid.
Cleanup these temporary directories on pid shutdown.
Includes tests for both features.