Skip to content
Permalink
Browse files Browse the repository at this point in the history
backport directory traversal vulnerability fix
  • Loading branch information
lukashinsch committed Jan 1, 2021
1 parent a4bff61 commit 760acbb
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 8 deletions.
1 change: 1 addition & 0 deletions .gitignore
Expand Up @@ -2,3 +2,4 @@
.idea
*.iml
build/
out/
2 changes: 1 addition & 1 deletion .travis.yml
@@ -1,6 +1,6 @@
language: java
jdk:
- oraclejdk8
- openjdk8

after_success:
- ./gradlew lib:jacocoTestReport lib:coveralls
Expand Up @@ -60,9 +60,9 @@ public String list(Model model, // TODO model should no longer be injected
@RequestParam(required = false, defaultValue = "FILENAME") SortBy sortBy,
@RequestParam(required = false, defaultValue = "false") boolean desc,
@RequestParam(required = false) String base) throws IOException, TemplateException {
securityCheck(base);

Path currentFolder = loggingPath(base);
securityCheck(currentFolder, null);


List<FileEntry> files = getFileProvider(currentFolder).getFileEntries(currentFolder);
List<FileEntry> sortedFiles = sortFiles(files, sortBy, desc);
Expand Down Expand Up @@ -127,10 +127,10 @@ public void view(@RequestParam String filename,
@RequestParam(required = false) String base,
@RequestParam(required = false) Integer tailLines,
HttpServletResponse response) throws IOException {
securityCheck(filename);
response.setContentType(MediaType.TEXT_PLAIN_VALUE);

Path path = loggingPath(base);
securityCheck(path, filename);
response.setContentType(MediaType.TEXT_PLAIN_VALUE);
FileProvider fileProvider = getFileProvider(path);
if (tailLines != null) {
fileProvider.tailContent(path, filename, response.getOutputStream(), tailLines);
Expand Down Expand Up @@ -171,8 +171,15 @@ private void searchAndStreamFile(FileEntry fileEntry, String term, OutputStream
}
}

private void securityCheck(String filename) {
Assert.doesNotContain(filename, "..");
private void securityCheck(Path base, String filename) {
try {
String canonicalLoggingPath = (filename != null ? new File(base.toFile().toString(), filename) : new File(base.toFile().toString())).getCanonicalPath();
String baseCanonicalPath = new File(loggingPath).getCanonicalPath();
String errorMessage = "File " + base.toString() + "/" + filename + " may not be located outside base path " + loggingPath;
Assert.isTrue(canonicalLoggingPath.startsWith(baseCanonicalPath), errorMessage);
} catch (IOException e) {
throw new IllegalStateException(e);
}
}

@Override
Expand Down
Expand Up @@ -330,12 +330,22 @@ public void shouldReturnNullEndpointType() {
public void shouldNotAllowToListFileOutsideRoot() throws Exception {
// given
expectedException.expect(IllegalArgumentException.class);
expectedException.expectMessage(containsString("this String argument must not contain the substring [..]"));
expectedException.expectMessage(containsString("may not be located outside base path"));

// when
logViewEndpoint.view("../somefile", null, null, null);
}

@Test
public void shouldNotAllowToListWithBaseOutsideRoot() throws Exception {
// given
expectedException.expect(IllegalArgumentException.class);
expectedException.expectMessage(containsString("may not be located outside base path"));

// when
logViewEndpoint.view("somefile", "../otherdir", null, null);
}

@Test
public void shouldViewFile() throws Exception {
// given
Expand Down

1 comment on commit 760acbb

@gurshafriri
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 @lukashinsch do you plan to make a release with this change? we (at snyk]) would like to add it as a vulnerability to our db, possibly assign a CVE to it as well.

Please sign in to comment.