docs: improve security documentation #311
Conversation
n0toose
force-pushed
the
docs-security
branch
from
3419d7d to
36a214d
Compare
Minor wording fixes. Increases visibility of `Builder::permissions`, the `NamedTempFile` Security documentation and `env::override_temp_dir`. Partially addresses Stebalien#303.
n0toose
force-pushed
the
docs-security
branch
from
36a214d to
530d11b
Compare
|
I tried to avoid rewriting the text, give specific instructions in the first page of the documentation (so that they won't be implemented without nuance) and instead opted for pointing people to the right parts of the documentation - where we have enough space to explain everything precisely - and an external resource that explains the problem to the user. Every use case is different, and figuring out the security aspects should be the user's responsibility - accidentally misleading the user is not very worth it. |
|
This also fixes an unfortunate |
src/lib.rs
Outdated
| //! operating systems and depending on your use case, it may be possible to | ||
| //! mitigate this issue by overriding the crate's default options. For more information, | ||
| //! consult the Security documentation of the [`NamedTempFile`] type, | ||
| //! [`Builder::permissions`] and [`env::override_temp_dir`]. |
There was a problem hiding this comment.
Note: Builder::permissions is relevant for Builder::tempdir, but not for Builder::tempfile (NamedTempFile) because:
- Changing the permissions of the file won't stop temporary file cleaners (the main issue).
- The permissions default to 0600.
However, setting the permissions is an important security consideration for temporary directories, so that should probably be called out.
There was a problem hiding this comment.
The permissions default to 0600.
I presume that this is not the case on Windows (with an equivalent permission)? I sent a revised version.
There was a problem hiding this comment.
Either way, it would be interesting to document that so that people know how to work around that if necessary in the meantime (e.g. using /tmp on Linux, but using a custom directory with specific permissions on Windows) and to allow room for a contribution that fixes the need for such a workaround to be made.
There was a problem hiding this comment.
I presume that this is not the case on Windows (with an equivalent permission)? I sent a revised version.
IIRC, windows already defaults to per-user temporary directories with restricted permissions.
Either way, it would be interesting to document that so that people know how to work around that if necessary in the meantime (e.g. using /tmp on Linux, but using a custom directory with specific permissions on Windows) and to allow room for a contribution that fixes the need for such a workaround to be made.
The issue discussed above is about pathological temporary file cleaners. Builder::permissions does not mitigate this issue and I don't want users to think it might.
There was a problem hiding this comment.
Thank you for clarifying. I think I did not really see the forest for the trees. I'll revise my change again for increased clarity.
a temporary file cleaner could delete the temporary file which an attacker could then replace.
There was a problem hiding this comment.
I changed the wording re: "predictable file names" without explicitly explaining the problem. I chose to link to the affected / relevant functions instead.
Minor wording fixes. Increases visibility of
Builder::permissions,the
NamedTempFileSecurity documentation andenv::override_temp_dir.Partially addresses #303.