Add symbolic link functionality for SMB2

[GregBragg]

Fixes #245

Add symbolic link SMB2 Error Context handling for four use cases for supporting the current SMB 3.1.1 protocol:

1. Add isSymLink() method to see if the file the SmbResource represents is a symbolic link.
2. Add handling to read a directory or file that is a symbolic link
3. Add handling to read files in a directory that is a symbolic link
4. Add handling to list files in a directory that is a symbolic link

[mbechler]

Sorry for the rather long delay, a bit of unexpected downtime...

Some comments inline.

[GregBragg]

@mbechler thanks for your review! Ok I’m off on vacation for two weeks and when I get back I’ll look into this, thanks.

[GregBragg]

@mbechler Sorry for the lengthy delay... I'm going to start reviewing your feedback notes this week and next and address your comments. Thanks!

[GregBragg]

@mbechler I've addressed a lot of your feedback points, however I need your help and expertise on a few items that I might not have covered off completely. Thanks!

[GregBragg]

@mbechler My apologies for the chatty commits over the last couple of weeks, however I am ready for another review. Thanks!

[GregBragg]

@mbechler I rebased my branch to master to make sure they were in sync with the latest changes in master. Please review at your convenience and let me know if we can move forward with this PR. Thanks!

[mbechler]

Thanks, unfortunately It's going to be another two to three weeks till I can get back on this.

[Dirk-A]

Hi Greg, thanks for contribution to the jcifs SMB library, it stumbled our team a bit that no one else came across symbolic link issues much sooner ;-). Anyway , we recently had issues with paths (Windows) containing symbolic links and it seemed your solution was the one to go to. I tested the code from your branch for our specific needs and after 1 modification, it does the job very well.
Issue in class Smb2ErrorDataFormat.java :
line 41: 8 bytes are read from the error buffer => it seems that on Windows Enterprise10/Server 2019, the symbolic link error tag is already read at this point and hence this.errorid already equals 4C4D5953 and not 0 => the SMBProtocolDecodingException is incorrectly thrown.

[GregBragg]

Hi Greg, thanks for contribution to the jcifs SMB library, it stumbled our team a bit that no one else came across symbolic link issues much sooner ;-). Anyway , we recently had issues with paths (Windows) containing symbolic links and it seemed your solution was the one to go to. I tested the code from your branch for our specific needs and after 1 modification, it does the job very well. Issue in class Smb2ErrorDataFormat.java : line 41: 8 bytes are read from the error buffer => it seems that on Windows Enterprise10/Server 2019, the symbolic link error tag is already read at this point and hence this.errorid already equals 4C4D5953 and not 0 => the SMBProtocolDecodingException is incorrectly thrown.

Interesting... according to the spec at https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/9ff8837c-c4f7-452b-9272-8818a119dae9 ErrorId for a symlink error should be zero 0x00000000 unless it's a share redirect. Please review the link and the class Smb2ErrorContextResponse for more context. The ErrorId is part of the 8 byte aligned boundary.

I really don't think the Windows server version has anything to do with what you are seeing. I have to follow the spec that Microsoft has written so not sure what is going on... Can you attach a code snippit of your fix so that I can test it?

The Windows File server that I have been testing against is the Datacenter 2016 version, I'll see if I have a server version that you are using, however again I really don't think it has anything to do with that because the SMB2 specs have been around for quite some time now and it doesn't look like Microsoft has revised them for each server version, however @mbechler would know better as he has more experience with the specs.

[Dirk-A]

It seems to follow this spec;
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/f15ae37d-a787-4bf2-9940-025a8c9c0022
Code snippet:

	int symLinkLength = 0;
	int symLinkErrorTag = 0;
    int bufferIndex = super.readErrorContextResponse(buffer);
    if ( this.errorId != 0 ) {
    	if (Integer.toHexString(this.errorId).toUpperCase().equals("4C4D5953")) {
    		symLinkErrorTag = this.errorId;
    		this.errorId = 0;
    		symLinkLength = this.errorDataLength;
        } else {
            throw new SMBProtocolDecodingException("ErrorId should be 0 for STATUS_STOPPED_ON_SYMLINK");
        }
    } else {

        // SymLinkLength (4 bytes)
        symLinkLength = SMBUtil.readInt4(buffer, bufferIndex);
        bufferIndex += 4;

        // SymLinkErrorTag (4 bytes) (always 0x4C4D5953)
        symLinkErrorTag = SMBUtil.readInt4(buffer, bufferIndex);
        if ( !Integer.toHexString(symLinkErrorTag).toUpperCase().equals("4C4D5953") ) {
            throw new SMBProtocolDecodingException("SymLinkErrorTag should be 0x4C4D5953");
        }
        bufferIndex += 4;
    }

....

[Dirk-A]

It seems readErrorResponse in ServerMessageBlock2.java already strips of the first 8 bytes of the error and returns the actual ErrorData which should match the structure in https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/f15ae37d-a787-4bf2-9940-025a8c9c0022

[mbechler]

I still believe we should make sure that we get the behavior in #309 (comment) right/according to spec. I was planning to check against the Microsoft implementation, but haven't gotten around to.

[mbechler]

So with the setup on the server (2K19) as follows:

C:\test\symlinktests>mklink /d relative-dir ..\x-test
symbolic link created for relative-dir <<===>> ..\x-test
C:\test\symlinktests>mklink relative-file ..\x-test\foo.txt
symbolic link created for relative-file <<===>> ..\x-test\foo.txt
C:\test\symlinktests>mklink absolute-file C:\test\x-test\foo.txt
symbolic link created for absolute-file <<===>> C:\test\x-test\foo.txt
C:\test\symlinktests>mklink /d absolute-dir C:\test\x-test\
symbolic link created for absolute-dir <<===>> C:\test\x-test\
C:\test\symlinktests>mklink /d unc-dir \\w2k19-single-dc\test\x-test\
symbolic link created for unc-dir <<===>> \\w2k19-single-dc\test\x-test\
C:\test\symlinktests>mklink unc-file \\w2k19-single-dc\test\x-test\foo.txt
symbolic link created for unc-file <<===>> \\w2k19-single-dc\test\x-test\foo.txt

after enabling bidirectional local/remote symlink following on a Win10 machine (fsutil behavior set SymlinkEvaluation L2L:1 R2R:1 L2R:1 R2L:1) this comes out pretty much as I had expected/described:

PS C:\Users\user> Get-Content \\w2k19-single-dc\test\symlinktests\relative-dir\foo.txt
this is the remote file
[Explorer shows target: \\w2k19-single-dc\test\x-test\ for relative-dir]
PS C:\Users\user> Get-Content \\w2k19-single-dc\test\symlinktests\relative-file
this is the remote file
[Explorer shows target: \\w2k19-single-dc\test\x-test\foo.txt for relative-file]
PS C:\Users\user> Get-Content \\w2k19-single-dc\test\symlinktests\absolute-dir\foo.txt
this is the local file
[Explorer shows target: C:\test\x-test\ for absolute-dir]
PS C:\Users\user> Get-Content \\w2k19-single-dc\test\symlinktests\absolute-file
this is the local file
[Explorer shows target: C:\test\x-test\foo.txt for \absolute-file]
PS C:\Users\user> Get-Content \\w2k19-single-dc\test\symlinktests\unc-file
this is the remote file
[Explorer shows target: \\w2k19-single-dc\test\x-test\foo.txt for unc-file]
PS C:\Users\user> Get-Content \\w2k19-single-dc\test\symlinktests\unc-dir\foo.txt
this is the remote file
[Explorer shows target: \\w2k19-single-dc\test\x-test for unc-dir]

Relevant packet captures, link error data: capture.zip

So I think the correct behavior would be:

1. Process the Link data according to spec (UnparsedNameLength/SubstituteName)
2. Check that the result is an UNC path. If it is a local path, I don't think these should be handled in the library in any way. Throwing an exception containing the target path may be appropriate, to allow handling by the user
3. Check that the UNC path is on the same server/share (otherwise handling like for DFS would have to be implemented, making things much more complicated), else, throw an exception.
4. Follow symlink

[GregBragg]

after enabling bidirectional local/remote symlink following on a Win10 machine (fsutil behavior set SymlinkEvaluation L2L:1 R2R:1 L2R:1 R2L:1) this comes out pretty much as I had expected/described:

Can you explain why you would do this on the client? Or am I misinterpreting this test case which I never would have anticipated for?

Furthermore my client is locked down by my organization, so I don't have local admin access to run a command like this to enable bi-directional symlinks on my laptop which I am sure would not be allowed by my organization anyways due to security concerns. Is this really a valid test case scenario that needs to be supported?

From my point of view all that is needed is to be able to resolve symlinks on the remote machine only, even if the substitute name comes back as a path that is local to the remote machine.

The solution I have come up with is to replace the front part of the substitute name path with the remote share path that we already know and append the substitute name path back from the end to figure out the target path. Wouldn't that be a more portable solution, particularly if the client is on a UNIX host?

[mbechler]

That was just to investigate how Windows would interpret the symlinks. As I mentioned I also don't think we should resolve remote to local symlinks. What I mean is that with the current implementation we will end up resolving to invalid targets (e.g. imagine an absolute local path that contains the share name, or an UNC path that contains the share name in the hostname, or what happens if it does not - indexOf = -1). This may cause strange, difficult to understand errors which we should avoid and instead do it according to spec and then be able to properly detect which type it is and whether to handle it or bail out with an error.

[GregBragg]

Ok understood... However I'm still at a lost to move forward with any other changes for this contribution. I'd appreciate if you could take what I have already proposed and make the changes as you see fit to work with the current implementation as I don't have the detailed level of knowledge of the internal workings of this code base that you do. Thanks!

[Dirk-A]

Hi Greg,
We recently encountered an issue with the determination of the root path, more specifically in SmbFile.java, 804-806:
int i = substituteName.indexOf(this.getShare());
String rootPath = substituteName.substring(i + this.getShare().length());
log.debug("Root Path -> {}", rootPath);
=> it can occur here that the substituteName does not contain the share name, hence the rootPath will be constructed from a wrong substituteName substring.
In our case:
SymLink Path -> \srvwfm\symlink1\20210216
SymLink Length -> 124
Absolute Path -> true
Print Name -> \srvwfm\20210216
Substitute Name -> \??\UNC\srvwfm\20210216
Share Name -> symlink100 (length = 10)
Root Path -> rvwfm\20210216 (starting substring: -1 + 10 => 9)

This is the case with and without forcing the dialect to SMB311

[GregBragg]

Hi Greg, We recently encountered an issue with the determination of the root path, more specifically in SmbFile.java, 804-806: int i = substituteName.indexOf(this.getShare()); String rootPath = substituteName.substring(i + this.getShare().length()); log.debug("Root Path -> {}", rootPath); => it can occur here that the substituteName does not contain the share name, hence the rootPath will be constructed from a wrong substituteName substring. In our case: SymLink Path -> \srvwfm\symlink1\20210216 SymLink Length -> 124 Absolute Path -> true Print Name -> \srvwfm\20210216 Substitute Name -> ??\UNC\srvwfm\20210216 Share Name -> symlink100 (length = 10) Root Path -> rvwfm\20210216 (starting substring: -1 + 10 => 9)

This is the case with and without forcing the dialect to SMB311

Yep ok, got it... I never unit tested with a UNC path (if that is what I am interpreting here), however now I get what @mbechler is concerned about, however still not sure how to code it properly as I don't have the detailed knowledge of the internal code base.

[GregBragg]

@Dirk-A can you pull down the latest changes and review/test please? I've refactored the code to follow the specs for resolving symlinks and absolute target paths starting with \??\ will not be handled as per @mbechler. Thanks!

[GregBragg]

So I think the correct behavior would be:

1. Process the Link data according to spec (UnparsedNameLength/SubstituteName)
2. Check that the result is an UNC path. If it is a local path, I don't think these should be handled in the library in any way. Throwing an exception containing the target path may be appropriate, to allow handling by the user
3. Check that the UNC path is on the same server/share (otherwise handling like for DFS would have to be implemented, making things much more complicated), else, throw an exception.
4. Follow symlink

@mbechler Ok points 1, 2, 4 are completed. Point 3 is completed, however for DFS I have no internal knowledge of how this was implemented so I did not add any handling for this scenario.

Local target paths will not be resolved, this will be left up to the client to further resolve/process as they will know the share and share directory to construct the root path to the server's local symlink target which will be availble via SmbFile.getSymLinkTargetPath().

[Dirk-A]

Hi Gregory,
Here are my remarks after testing;
SmbFile.java:
withOpen method => as we use exist() checks on files/folders, the exception handling here should also take care of symbolic links:

Smb2CreateResponse createResp = cr.getResponse();
if (createResp != null && createResp.isReceived() && createResp.getStatus() == NtStatus.NT_STATUS_STOPPED_ON_SYMLINK)

{ this.isSymLink = true; Smb2SymLinkResolver resolver = new Smb2SymLinkResolver(); String targetPath = resolver.parseSymLinkErrorData(createResp.getFileName(), createResp.getErrorData()); this.symLinkTargetPath = targetPath; th.send(new Smb2CloseRequest(th.getConfig(), createResp.getFileId()), RequestParam.NO_RETRY); }
else if ( createResp.isReceived() && createResp.getStatus() == NtStatus.NT_STATUS_OK )

{ th.send(new Smb2CloseRequest(th.getConfig(), createResp.getFileId()), RequestParam.NO_RETRY); }

RecursionLimiter.java:
As we use exist check + readable/writable checks, the recursion method stack can be larger than the default check on 50 => increase to 200

It still seems the intention is to only support DialectVersion.SMB311, will this change in the near future?

Now only local paths are supported (\??\)
=> throws exception), but it is such an issue to take eg, the print name or the substitute name and strip off the ??\UNC part?
SymLink Path -> 20210216
Print Name -> \hostname\20210216
Substitute Name -> ??\UNC\hostname\20210216
(as it should become)Target Path -> \hostname\20210216

[mbechler]

Regarding 3.1.1: That is the only version that supports symlinks via this mechanism (providing the symlink data via error response). There might be an alternate route, e.g. making an IOCTL request if the error code is received, but have not looked at this at all.

Regarding absolute targets, if it's cross-share or cross-server things become a lot more complicate as now the requests need to be routed differently, and doing that transparently causes all kinds of issues, both architecture-wise and also logically (e.g. what credentials should be used in that case?). I haven't looked closely but I guess some improvement could be made regarding absolute targets on the same share, this should work as is, but I don't think this applies to your example, which is cross-share.
An alternative option I see would be to provie the target as part of the exception, so that you could get a new SmbFile object for thfe target. Would that be helpful, too, or do you really need transparent handling?

[GregBragg]

An alternative option I see would be to provie the target as part of the exception, so that you could get a new SmbFile object for thfe target. Would that be helpful, too, or do you really need transparent handling?

The exception provided does supply the local target path as part of the message, however this can always be retrieved via SmbFile.getSymLinkTargetPath(). The JUnit test class has an example of how to catch the SMBProtocolDecodingException and construct a new SmbFile object with the resolved symlink target path.

Regarding exist() I believe we shouldn't resolve the path in there, that is more an architectural decision on my part to only identify that a symlink was encountered.

With regards to RecursionLimiter the check limit can certainly be increased but the question becomes what will be an ideal upper limit? Personally this was something I believe was not really needed, however it does limit run away threads in a loop executing forever. There are other libraries out there that don't limit recursion in any way and I have personally never had to implement anything to keep track of, or limit the recursion depth.

[GregBragg]

@Dirk-A I tested your code suggestion:

SmbFile.java: withOpen method => as we use exist() checks on files/folders, the exception handling here should also take care of symbolic links:

Smb2CreateResponse createResp = cr.getResponse(); if (createResp != null && createResp.isReceived() && createResp.getStatus() == NtStatus.NT_STATUS_STOPPED_ON_SYMLINK)

{ this.isSymLink = true; Smb2SymLinkResolver resolver = new Smb2SymLinkResolver(); String targetPath = resolver.parseSymLinkErrorData(createResp.getFileName(), createResp.getErrorData()); this.symLinkTargetPath = targetPath; th.send(new Smb2CloseRequest(th.getConfig(), createResp.getFileId()), RequestParam.NO_RETRY); } else if ( createResp.isReceived() && createResp.getStatus() == NtStatus.NT_STATUS_OK )

{ th.send(new Smb2CloseRequest(th.getConfig(), createResp.getFileId()), RequestParam.NO_RETRY); }

This line after resolving the symlink throws an exception trying to close the request:
th.send(new Smb2CloseRequest(th.getConfig(), createResp.getFileId()), RequestParam.NO_RETRY);

so I propose we could enhance it this way:
Smb2CreateResponse createResp = cr.getResponse();
if ( createResp != null && createResp.isReceived() && createResp.getStatus() == NtStatus.NT_STATUS_STOPPED_ON_SYMLINK ) {
this.isSymLink = true;
Smb2SymLinkResolver resolver = new Smb2SymLinkResolver();
this.symLinkTargetPath = resolver.parseSymLinkErrorData(createResp.getFileName(), createResp.getErrorData());
} else if ( createResp.isReceived() && createResp.getStatus() == NtStatus.NT_STATUS_OK ) {
th.send(new Smb2CloseRequest(th.getConfig(), createResp.getFileId()), RequestParam.NO_RETRY);
}

[GregBragg]

@Dirk-A any feedback on the latest changes? It's been awhile...

@mbechler wondering when we can move forward with this or whether you need any other changes committed or whether you would like to take it from here and modify the contribution as you see fit...

[Dirk-A]

Hi Gregory,
bit busy here :-), next week I will plan to do some re-testing on the JCIFS library.

[mbechler]

I should be able to take it from here. Hoping to be able to work on some stuff over christmas.

[mbechler]

Have not found as much time as I would have liked, but I created a branch (https://github.com/AgNO3/jcifs-ng/tree/GregBragg-Add_symlink_functionality) where I made some changes (fd5bd47). Still a work in progress though. I'm also hoping to implement symlink creation as well, as this would really be benefical for test cases.

[mbechler]

Some more progress, implemented symlink creation etc, so I can work on automated test cases next. Also might have a solution for pre 3.1 versions.

[GregBragg]

@mbechler I was able to pull down your branch and review, I think I understand the improvements you are making to my contribution. I was able to get the JUnit test to complete successfully with the following changes to the code:

Looking forward to seeing the final changes to this enhancment.
Thanks!

[GregBragg]

@mbechler It's been awhile since we had any activity on this enhancement. Any thoughts for when we can collaborate further on this PR proposal? Thanks!

[mbechler]

My apologies, I haven't had much time lately and got a bit distracted. Hoping to get back to work soon.

[Dirk-A]

Hi, it has indeed been a while. My feedback is still that the limitation to MIN version SMB311 is not acceptable for our application needs. These are the local library changes I still maintain to cover all use cases we encountered in the field:

Smb2ErrorDataFormat.java =>

public int readSymLinkErrorResponse ( byte[] buffer ) throws SMBProtocolDecodingException {
    int symLinkLength = 0;
	int symLinkErrorTag = 0;
	int bufferIndex = super.readErrorContextResponse(buffer);
    
    //DIRK: FIX TO GET RID OF DIALECT DEPENDENCY!
    if ( this.errorId != 0 ) {
    	if (Integer.toHexString(this.errorId).toUpperCase().equals("4C4D5953")) {
    		symLinkErrorTag = this.errorId;
    		this.errorId = 0;
    		symLinkLength = this.errorDataLength;
        } else {
        	throw new SMBProtocolDecodingException("ErrorId should be 0 for STATUS_STOPPED_ON_SYMLINK");
        }
    } else {

        // SymLinkLength (4 bytes)
        symLinkLength = SMBUtil.readInt4(buffer, bufferIndex);
        bufferIndex += 4;

        // SymLinkErrorTag (4 bytes) (always 0x4C4D5953)
        symLinkErrorTag = SMBUtil.readInt4(buffer, bufferIndex);
        if ( symLinkErrorTag != 0x4C4D5953 ) {
            throw new SMBProtocolDecodingException("SymLinkErrorTag should be 0x4C4D5953");
        }
        bufferIndex += 4;
    }

....

SmbFile.java => remove throwing exceptions in case of :

1. if (config.getMinimumVersion() != DialectVersion.SMB311){ ....
2. if (targetPath.startsWith("\??\")) { ...
3. and take share into account:
   this.symLinkTargetPath = resolver.parseSymLinkErrorData(createResp.getFileName(), createResp.getErrorData(),this.getShare());

Smb2SymLinkResolver.java => take share into account:

    ....
    String rootPath = "";
    if (erdf.isAbsolutePath()) {
    	if ((null != shareName) && !shareName.isEmpty()) {
        	log.debug("Share Name -> {}", shareName);
            int i = substituteName.indexOf(shareName);
            if (i > 0){
            	rootPath = substituteName.substring(i + shareName.length());
            } else {
            	rootPath = erdf.getPrintName();
            }
    	} else {
        	rootPath = erdf.getPrintName();
        }
    	log.debug("Root Path -> {}", rootPath);	
    	 if (!originalFileName.endsWith("\\") && rootPath.endsWith("\\")) { // is this a file in a symlink directory?
             String fileName = originalFileName.substring(originalFileName.lastIndexOf("\\") + 1);
             log.debug("File Name -> {}", fileName);
             target = rootPath + fileName;
         }
         else {
             target = rootPath + unparsedPath;
         }	
    } else {
    	 if (!originalFileName.endsWith("\\")) { // is this a file in a symlink directory?
             String symLinkFileName = originalFileName.substring(originalFileName.lastIndexOf("\\") + 1);
             log.debug("SymLink File Name -> {}", symLinkFileName);
             target = originalFileName.replace(symLinkFileName, "") + substituteName;
         }
         else {
            String parsedPath = getSymLinkParsedPath(originalFileName, unparsedPathLength);
            StringBuilder b = new StringBuilder();
            int startIndex = parsedPath.lastIndexOf("\\");
            if (startIndex != -1) {
                b.append(parsedPath, 0, startIndex);
                b.append('\\');
            }
            b.append(substituteName);
            b.append(unparsedPath);
            target = b.toString();
         }
    }

[GregBragg]

Hi, it has indeed been a while. My feedback is still that the limitation to MIN version SMB311 is not acceptable for our application needs. These are the local library changes I still maintain to cover all use cases we encountered in the field:

@Dirk-A

My comment is that symlinks are only supported with the SMB 3.11 protocol. What you are proposing as a workaround does not follow the Microsoft specs for SMB2 as Moritz has already pointed out in a previous comment.

[mbechler]

Not entirely what I said. The symlink information is only available in the error response starting with 3.11, for earlier versions it has to be explicitly requested as part of the error handling. This need additional logic, but can be done. It will however be very inefficient, especially if the results are not cached.