Fix recent security fixes if JENKINS_HOME is a symlink and update tests for compatibility with Windows and Git plugin 4.10.3 #139
Conversation
… and Git plugin 4.10.3
…S_HOME is a symlink
| @@ -256,7 +256,7 @@ | |||
| for (LibraryRecord library : action.getLibraries()) { | |||
| FilePath libResources = libs.child(library.getDirectoryName() + "/resources/"); | |||
| FilePath f = libResources.child(name); | |||
| if (!new File(f.getRemote()).getCanonicalFile().toPath().startsWith(libResources.absolutize().getRemote())) { | |||
| if (!new File(f.getRemote()).getCanonicalFile().toPath().startsWith(new File(libResources.getRemote()).getCanonicalPath())) { | |||
There was a problem hiding this comment.
The only intended difference here is that if JENKINS_HOME is a symlink, the code should no longer throw an exception in non-dangerous scenarios.
dwnusbaum
changed the title
| @@ -256,7 +256,7 @@ | |||
| for (LibraryRecord library : action.getLibraries()) { | |||
| FilePath libResources = libs.child(library.getDirectoryName() + "/resources/"); | |||
| FilePath f = libResources.child(name); | |||
| if (!new File(f.getRemote()).getCanonicalFile().toPath().startsWith(libResources.absolutize().getRemote())) { | |||
| if (!new File(f.getRemote()).getCanonicalFile().toPath().startsWith(new File(libResources.getRemote()).getCanonicalPath())) { | |||
There was a problem hiding this comment.
| if (!new File(f.getRemote()).getCanonicalFile().toPath().startsWith(new File(libResources.getRemote()).getCanonicalPath())) { | |
| if (!new File(f.getRemote()).getCanonicalFile().toPath().startsWith(new File(libResources.getRemote()).getCanonicalFile().toPath())) { |
?
There was a problem hiding this comment.
Though even better is to use NIO fully:
| if (!new File(f.getRemote()).getCanonicalFile().toPath().startsWith(new File(libResources.getRemote()).getCanonicalPath())) { | |
| if (!Paths.get(f.getRemote()).toRealPath().startsWith(Paths.get(libResources.getRemote()).toRealPath())) { |
There was a problem hiding this comment.
Path.toRealPath throws if the path does not exist on the file system, so we would have to change things a bit more if we wanted to keep attackers from being able to probe the existence of files using symlinks.
Is there a meaningful difference between Path.startsWith(String) and Path.startsWith(Path)? They both return false for things like foo/bar starting with f.
There was a problem hiding this comment.
Path.toRealPaththrows if the path does not exist on the file system
Mm, I see. So there is no NIO equivalent to File.getCanonicalPath?
Is there a meaningful difference
I do not know.
There was a problem hiding this comment.
Mm, I see. So there is no NIO equivalent to File.getCanonicalPath?
Not directly as far as I know. I did consider using NIO in the original patch and catching NoSuchFileException so that error messages would be the same regardless of file existence, but it seemed simpler to use getCanonicalFile.
I do not know.
Ok, I just wasn't sure if it was a style-related suggestion or if I was missing an important detail. As far as I know the current code with Path.startsWith(f.getCanonicalPath()) and the proposed code with Path.startsWith(f.getCanonicalFile().toPath()) should have the same behavior.
There was a problem hiding this comment.
Probably. The weird corner cases are likely to be on Windows. I think the current code is secure and likely to fix the regression for most if not all users.
There was a problem hiding this comment.
Ok, thanks for elaborating. I'll merge the code as-is and will follow up as needed.
Fix recent security fixes if JENKINS_HOME is a symlink and update tests for compatibility with Windows and Git plugin 4.10.3 (cherry picked from commit e62a4eb)
The tests for security fixes for https://www.jenkins.io/security/advisory/2022-02-15/ fail against recent version of Git plugin due to jenkinsci/git-plugin#1207, and also fail when running on Windows due to a difference in the behavior.
The fix itself also did not allow
JENKINS_HOMEto be a symlink, because it compared a canonical path to an absolute path.