On Saturday, 1 October 2016 at 19:32:08 UTC, Andrei Alexandrescu wrote:
>> Not like this is real security concern in dmd case but guidelines like
>> "don't make /tmp/ path predictable" exist exactly so that one can have
>> simple safe default and not worry about possibilities.
>
> This may be a misunderstanding. I'm saying is to switch from unpredictable paths rooted in /tmp/ to equally unpredictable paths rooted in /tmp/.dmd-test-run/.

I think that is OK but only if actual file inside the dir is created with `mktemp --tmpdir=/tmp/.dmd-test-run/` (or using a similar technique). I wonder if Phobos provides any cross-platform way to do so - I remember some PR on topic but in current documentation there seems to be nothing useful mentioned,

On Saturday, 1 October 2016 at 19:51:05 UTC, Dicebot wrote:
> I think that is OK but only if actual file inside the dir is created with `mktemp --tmpdir=/tmp/.dmd-test-run/` (or using a similar technique).

This is not sufficient.  Any user can create a symlink from /tmp/.dmd-test-run/ to e.g. /very/private/root/directory/ (that user can't access it, but symlinks don't check the permission of the target).  Executed as root user, mktemp then creates a unique file in /very/private/root/directory/.  Which can be used for example to litter a filesystem, which hurts performance or fills disks.

That's why I was saying /tmp/.dmd-test-run/ should have permissions 0700.  I think a better naming scheme would be /tmp/dmd-testrun-username/, or if that already exists with wrong permissions /tmp/dmd-testrun-username-RANDOMCHARS/.  The files inside that directory don't need to have random names (afaik).

> It seems like more practical issue is simply that no regular destruction of /tmp/ happens on your system.

I'm not sure what you were implying by this.  Deleting anything in /tmp while it's mounted is a very bad idea.  The permission-check of /tmp/dmd-testrun-username/ relies on the fact that the directory won't be deleted.  If it will, then this introduces a race condition.

On 10/01/2016 05:00 PM, Guillaume Boucher wrote:
> On Saturday, 1 October 2016 at 19:51:05 UTC, Dicebot wrote:
>> I think that is OK but only if actual file inside the dir is created
>> with `mktemp --tmpdir=/tmp/.dmd-test-run/` (or using a similar
>> technique).
>
> This is not sufficient.  Any user can create a symlink from
> /tmp/.dmd-test-run/ to e.g. /very/private/root/directory/ (that user
> can't access it, but symlinks don't check the permission of the
> target).  Executed as root user, mktemp then creates a unique file in
> /very/private/root/directory/.  Which can be used for example to litter
> a filesystem, which hurts performance or fills disks.
>
> That's why I was saying /tmp/.dmd-test-run/ should have permissions
> 0700.  I think a better naming scheme would be
> /tmp/dmd-testrun-username/, or if that already exists with wrong
> permissions /tmp/dmd-testrun-username-RANDOMCHARS/.  The files inside
> that directory don't need to have random names (afaik).

Interesting, thanks. Seems like the most robust thing to do is to not use /tmp/ after all. In fact, I've encountered errors because (if I remember correctly) we list the content of the /tmp/ directory in unittests and we get exceptions because some dirs are not accessible.

A PR reviewing all uses of /tmp/ would be awesome.


Andrei

On Saturday, 1 October 2016 at 17:35:29 UTC, Andrei Alexandrescu wrote:
> I think /tmp/ is mounted per user so we should be good.

I have never seen this. In fact, I'm not familiar with any mechanisms of "per-user" mounts on POSIX systems.

The general practice of creating files in /tmp/ is to either put the UID in the filename, or use unique random filenames (e.g. via mkstemp).

If this is security-sensitive, we should not be dismissive about any aspects of this.

> It would also be nice to have a VERY SIMPLE mechanism to delete old runs (e.g. a day or more).

FWIW, systemd mounts /tmp as a tmpfs, and OS X seems to delete files in /tmp older than 3 days.

On Saturday, 1 October 2016 at 21:25:56 UTC, Andrei Alexandrescu wrote:
> Interesting, thanks. Seems like the most robust thing to do is to not use /tmp/ after all.

That will cause performance regressions on systems that mount /tmp in RAM.

On 10/01/2016 06:08 PM, Vladimir Panteleev wrote:
> On Saturday, 1 October 2016 at 21:25:56 UTC, Andrei Alexandrescu wrote:
>> Interesting, thanks. Seems like the most robust thing to do is to not
>> use /tmp/ after all.
>
> That will cause performance regressions on systems that mount /tmp in RAM.

Subtle point, thanks.

On the same topic, I think I found where the intermittent test failures are: https://github.com/dlang/phobos/blob/master/std/file.d#L3312

There, we list the full content of /tmp, recursively. That's not quite the right thing to do. Not only the run time is arbitrarily slow, but the test may file if certain subdirs are inaccessible. Vladimir, maybe I can convince you to find a better solution for that, too? :o)

https://issues.dlang.org/show_bug.cgi?id=16571


Thanks,

Andrei

On Sunday, 2 October 2016 at 01:23:18 UTC, Andrei Alexandrescu wrote:
> There, we list the full content of /tmp, recursively. That's not quite the right thing to do. Not only the run time is arbitrarily slow, but the test may file if certain subdirs are inaccessible. Vladimir, maybe I can convince you to find a better solution for that, too? :o)
>
> https://issues.dlang.org/show_bug.cgi?id=16571

The unittest in question is ensuring that compilation succeeds:

https://github.com/dlang/phobos/pull/3821

Solution: move the dirEntries call inside the __traits(compiles) check.

I'm doing server maintenance tonight so no PR for the moment :)

On Saturday, October 01, 2016 19:51:05 Dicebot via Digitalmars-d wrote:
> I wonder if Phobos provides any
> cross-platform way to do so - I remember some PR on topic but in
> current documentation there seems to be nothing useful mentioned,

We had it, and it got yanked, because there was screaming just because it increased the "hello world" binary by about 500KB because of stuff getting pulled in that really shouldn't be pulled in just to use writeln. There's a bug report for putting it back

https://issues.dlang.org/show_bug.cgi?id=14599

but the compiler issues that resulted in way more getting pulled in with writeln than should have been have to be fixed first.

- Jonathan M Davis

On Sunday, 2 October 2016 at 01:30:15 UTC, Vladimir Panteleev wrote:
> On Sunday, 2 October 2016 at 01:23:18 UTC, Andrei Alexandrescu wrote:
>> There, we list the full content of /tmp, recursively. That's not quite the right thing to do. Not only the run time is arbitrarily slow, but the test may file if certain subdirs are inaccessible. Vladimir, maybe I can convince you to find a better solution for that, too? :o)
>>
>> https://issues.dlang.org/show_bug.cgi?id=16571
>
> The unittest in question is ensuring that compilation succeeds:
>
> https://github.com/dlang/phobos/pull/3821
>
> Solution: move the dirEntries call inside the __traits(compiles) check.
>
> I'm doing server maintenance tonight so no PR for the moment :)

This is partly my fault:

https://github.com/dlang/phobos/pull/4332/files#diff-52580fb75b304ba7b04a6b178fe6cdf4L3270

I'll submit a PR with the fix you suggest.

On 10/02/2016 09:19 AM, Joakim wrote:
> On Sunday, 2 October 2016 at 01:30:15 UTC, Vladimir Panteleev wrote:
>> On Sunday, 2 October 2016 at 01:23:18 UTC, Andrei Alexandrescu wrote:
>>> There, we list the full content of /tmp, recursively. That's not
>>> quite the right thing to do. Not only the run time is arbitrarily
>>> slow, but the test may file if certain subdirs are inaccessible.
>>> Vladimir, maybe I can convince you to find a better solution for
>>> that, too? :o)
>>>
>>> https://issues.dlang.org/show_bug.cgi?id=16571
>>
>> The unittest in question is ensuring that compilation succeeds:
>>
>> https://github.com/dlang/phobos/pull/3821
>>
>> Solution: move the dirEntries call inside the __traits(compiles) check.
>>
>> I'm doing server maintenance tonight so no PR for the moment :)
>
> This is partly my fault:
>
> https://github.com/dlang/phobos/pull/4332/files#diff-52580fb75b304ba7b04a6b178fe6cdf4L3270
>
>
> I'll submit a PR with the fix you suggest.

Thanks! -- Andrei