3420 – Allow string import of files using subdirectories

const data = import("dir/data.txt");
Specifying -J. for DMD 1.041 is sufficient to allow this to compile.
I couldn't find an option for DMD 1.042 and newer which would allow this to compile.

Paths are not allowed in the string supplied to the import statement. This is for security reasons. To get the example to compile, use:
 import ("data.txt");
and use the switch:
 -Jdir
This behavior is as intended.

That's fine and dandy, except it doesn't work either:
C:\Temp\D\Bug3420> dir
 Volume in drive C is WS2008X64
 Volume Serial Number is 4CC8-8E34
 Directory of C:\Temp\D\Bug3420
2009年11月30日 12:38 <DIR> .
2009年11月30日 12:38 <DIR> ..
2009年11月30日 12:38 <DIR> dir
2009年11月30日 12:38 38 test.d
 1 File(s) 38 bytes
 3 Dir(s) 11,197,788,160 bytes free
C:\Temp\D\Bug3420> dir dir
 Volume in drive C is WS2008X64
 Volume Serial Number is 4CC8-8E34
 Directory of C:\Temp\D\Bug3420\dir
2009年11月30日 12:38 <DIR> .
2009年11月30日 12:38 <DIR> ..
2009年11月30日 12:38 4 data.txt
 1 File(s) 4 bytes
 2 Dir(s) 11,197,788,160 bytes free
C:\Temp\D\Bug3420> cat test.d
const data = import("dir/data.txt");
C:\Temp\D\Bug3420> dmd -Jdir test.d
test.d(1): Error: use -Jpath switch to provide path for filename dir/data.txt

I think you have to do this:
const data = import("data.txt");
 ^
 no "dir/"
$ dmd -Jdir test.d

Ah, I see. This should be clarified in the documentation... Also, doesn't anyone think that this could be too constricting? What if you have a directory tree of data to import? Not to mention not being able to import two files with the same filename but from different directories...

(In reply to comment #4)
> Ah, I see. This should be clarified in the documentation... Also, doesn't
> anyone think that this could be too constricting?
I do.
Maybe this can be changed to be a feature request. I really can't see how allowing subdirectories can be a security risk, you only have to check that the canonical name of the imported file is still in a subdirectory of an -J'ed directory.
In POSIX you can use realpath(3) to get the canonical name of a file, then just check if the imported canonical name starts with any -J directory canonical name.

Created attachment 520 [details] 
Proof of concept patch
Here it is a proof of concept patch to allow directories in string imports *safely*. The check is done as I said in comment 5: -J paths are converted to canonical names, then the string import path is appended and the resulting path is again converted to a canonical name. Then, the canonical name is checked to be really in the canonical path. This prevents any type of highjacking (even with symlinks).
Here is a simple example:
import("x/../../y") in combination with -J.. (assuming /tmp/x is the current directory) is checked like this:
1) .. is converted to realpath(..) which yields /tmp
2) the canonical path is combined with the file name: /tmp/x/../../y
3) the new filename is converted to a canonical filename: /y
4) the canonical path and the canonical name are checked: /y doesn't start with /tmp, so the import is rejected.
Unfortunately, I'm not a windows developer, and the path is only implemented for POSIX (and only tested in Linux, but if other *nixes don't work it should be fairly simple to fix). Compiling in Windows yields an error for now. If there is no way to implement this on Windows, it's fairly easy to allow this behavior in POSIX and fallback to the old behavior in Windows. Let me know if you want a patch for that.
I'll attach a few test cases.

Created attachment 521 [details] 
Test cases
Here are some test cases, they are packed in a tarball because it includes a directory structure.
Tests are in the directory d2, and they should be compiled in that directory. The tests have a small comment indicating what -J should be used to compile (other -J options should fail).
You can test them all with this simple bash script (run it in the d2 directory):
for f in test{1..7}.d
do
 for j in . ..
 do
 echo -n "$f: "; head -n1 $f
 dmd -J$j $f && ./`basename $f .d`
 done
done
You have to go through the results and check them visually, the script can be improve to make the verification automatically.

BTW, the patch I consider the patch only a proof of concept because it lacks Windows support (and testing on other unixes). Besides that, I think the patch is not bad =)

It's really frustrating when such unannounced changes in compiler behavior break my code. Today I needed to fix up an old program I wrote, which makes extensive usage of importing files from a subdirectory tree. I couldn't use an old compiler version due to bugs... needless to say, I wasted time editing import paths and creating special-case build scripts. An undocumented breaking of functionality hardly fits a "stable" implementation...

So, this is *really* a regression then, I'm sorry I changed the severity. But are you sure it works in DMD 1.041? I tracked this down in the code and it was introduced at DMD 1.006, when the -J option got implemented, so I can't see how it could work in DMD 1.041:
http://www.dsource.org/projects/dmd/changeset/121
http://www.dsource.org/projects/dmd/changeset/121#file4 (expression.c)
http://www.dsource.org/projects/dmd/changeset/121#file11 (mars.c)
http://www.digitalmars.com/d/1.0/changelog.html#new1_006
Before that, you could use string imports in a very promiscuous way. The specs are very brief about string imports and doesn't say anything about restrictions:
 The AssignExpression must evaluate at compile time to a constant string. The
 text contents of the string are interpreted as a file name. The file is read,
 and the exact contents of the file become a string literal. 
So, the -J option seems to be an implementation-defined behavior, as such, I guess it should be documented. For D1, I don't know if the specs should be changed, but I guess the old behavior should work (the attached patch can be used to allow secure string imports without prohibiting sub-directories altogether).
I don't know if the compiler option shouldn't be documented too, it seems pretty useless to make this an implementation defined behavior, you'll have to stay always with the most restrictive implementation limitations if you want to write code that can be used with any compiler, or avoid using this feature at all.

Yes, I was very careful in finding the exact version this problem was introduced. As stated in the issue description, the problem manifests in DMD 1.042 (and newer), but not 1.041. At first I thought it was an accidental regression, caused by "Fix bug where / wasn't recognized as a path separator on Windows", but as Walter has stated, this was an intentional change. 
And yes, I have already agreed that this needs to be better documented.

I just tried DMD 1.041 and I can't make
const data = import("dir/data.txt");
work using -J. (or any other -J option for that matter). Can you try again with DMD 1.041? I can't see any change in DMD post 1.006 that can affect this behavior. The limitation was introduced in that version of the compiler.

C:\Downloads\dmd.1.041\dmd\windows\bin> dmd | head -n 1
Digital Mars D Compiler v1.041
C:\Downloads\dmd.1.041\dmd\windows\bin> type test.d
const data = import("dir/data.txt");
C:\Downloads\dmd.1.041\dmd\windows\bin> dir dir
 Volume in drive C is WS2008X64
 Volume Serial Number is 4CC8-8E34
 Directory of C:\Downloads\dmd.1.041\dmd\windows\bin\dir
2009年12月04日 18:32 <DIR> .
2009年12月04日 18:32 <DIR> ..
2009年12月04日 18:32 0 data.txt
 1 File(s) 0 bytes
 2 Dir(s) 84,268,494,848 bytes free
C:\Downloads\dmd.1.041\dmd\windows\bin> dmd -c -J. test
(no output, compiles successfully)

Doesn't work on Linux, so this is a Windows-only problem (well, it was a Windows-only fix really :). Here is the fix:
http://www.dsource.org/projects/dmd/changeset/156#file64
This is the entry in the DMD 1.042 changelog that fixed this problem:
* Fix bug where / wasn't recognized as a path separator on Windows.
Try with import("dir\\data.txt"), it should fail to compile in DMD 1.006+.
So, summarizing:
* DMD 1.006 introduces a regression, which is supposed to be a feature.
* Supposing this is a real feature, you were relying on a bug in your programs, if you code them with DMD 1.006+ in the first place.
* The specs are not clear about this, reading the specs there is no reason to think that import("a/b") shouldn't work (i.e., at least there should be a compiler switch to make it work or the specs should be fixed to reflect the reality).
* There is a better solution to relax the current restrictions without being completely promiscuous about string imports (see attached patch).

So, my programs relied on a bug in DMD, which was recently rightfully fixed.
Well, that sucks.
Hoping your patch is accepted.

Changeset 396, for Posix only.

Posix-only fix in dmd 1.057 and 2.041

Removing 'patch' keyword -- there's no patch for the systems for which the issue still applies.

This link:
https://www.securecoding.cert.org/confluence/display/seccode/FIO02-C.+Canonicalize+path+names+originating+from+untrusted+sources
states that:
"Producing canonical file names for Windows operating systems is extremely complex and beyond the scope of this standard. The best advice is to try to avoid making decisions based on a path, directory, or file name [Howard 2002]. Alternatively, use operating-system-based mechanisms, such as access control lists (ACLs) or other authorization techniques."
Thus, this issue might not be fixable on Windows. 
I'm downgrading this all the way from 'regression' to 'enhancement', since it was a security bug that it ever worked at all. Perhaps the bug should just be closed.

This was fixed for POSIX in 2.041 and for Windows in 2.072.0.