martin-g commented on code in PR #1432:
URL: https://github.com/apache/wicket/pull/1432#discussion_r3122879719
##########
wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManager.java:
##########
@@ -60,14 +61,42 @@ public File getFolder() {
return folder;
}
+ /**
+ * Returns the path for a directory for use in path traversal checks. Uses
{@code toRealPath()}
+ * when the directory exists; otherwise falls back to {@code
toAbsolutePath().normalize()} to
+ * support validation when the directory has not yet been created.
+ */
+ private static Path getPathForComparison(java.io.File dir) throws
IOException
+ {
+ try
+ {
+ return dir.toPath().toRealPath();
+ }
+ catch (IOException e)
+ {
+ return dir.toPath().toAbsolutePath().normalize();
+ }
+ }
+
@Override
public void save(FileUpload fileItem, String uploadFieldId)
{
- File uploadFieldFolder = new File(getFolder(), uploadFieldId);
- uploadFieldFolder.mkdirs();
try
{
- IOUtils.copy(fileItem.getInputStream(), new FileOutputStream(new
File(uploadFieldFolder, fileItem.getClientFileName())));
+ Path baseFolderPath = getFolder().toPath().toRealPath();
+ java.io.File uploadFieldFolder = new File(getFolder(),
uploadFieldId).getCanonicalFile();
+ if (!uploadFieldFolder.toPath().startsWith(baseFolderPath))
+ {
+ throw new SecurityException("Path traversal detected in
uploadFieldId");
+ }
+ uploadFieldFolder.mkdirs();
+ java.io.File target = new File(uploadFieldFolder,
fileItem.getClientFileName()).getCanonicalFile();
+ Path uploadFieldFolderPath =
getPathForComparison(uploadFieldFolder);
+ if (!target.toPath().startsWith(uploadFieldFolderPath))
+ {
+ throw new SecurityException("Path traversal detected in client
filename");
+ }
+ IOUtils.copy(fileItem.getInputStream(), new
FileOutputStream(target));
Review Comment:
This does not close the streams.
```suggestion
try (java.io.InputStream in = fileItem.getInputStream();
java.io.FileOutputStream out = new FileOutputStream(target))
{
IOUtils.copy(in, out);
}
```
##########
wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManager.java:
##########
@@ -78,6 +107,25 @@ public void save(FileUpload fileItem, String uploadFieldId)
@Override
public File getFile(String uploadFieldId, String clientFileName)
{
- return new File(new File(getFolder(), uploadFieldId), clientFileName);
+ try
+ {
+ Path baseFolderPath = getFolder().toPath().toRealPath();
+ java.io.File uploadFieldFolder = new File(getFolder(),
uploadFieldId).getCanonicalFile();
+ if (!uploadFieldFolder.toPath().startsWith(baseFolderPath))
+ {
+ throw new SecurityException("Path traversal detected in
uploadFieldId");
+ }
+ java.io.File target = new File(uploadFieldFolder,
clientFileName).getCanonicalFile();
+ Path uploadFieldFolderPath =
getPathForComparison(uploadFieldFolder);
+ if (!target.toPath().startsWith(uploadFieldFolderPath))
+ {
+ throw new SecurityException("Path traversal detected in client
filename");
+ }
Review Comment:
Let's extract the duplicated code and reuse it
##########
wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManager.java:
##########
@@ -60,14 +61,42 @@ public File getFolder() {
return folder;
}
+ /**
+ * Returns the path for a directory for use in path traversal checks. Uses
{@code toRealPath()}
+ * when the directory exists; otherwise falls back to {@code
toAbsolutePath().normalize()} to
+ * support validation when the directory has not yet been created.
+ */
+ private static Path getPathForComparison(java.io.File dir) throws
IOException
+ {
+ try
+ {
+ return dir.toPath().toRealPath();
+ }
+ catch (IOException e)
+ {
+ return dir.toPath().toAbsolutePath().normalize();
+ }
+ }
+
@Override
public void save(FileUpload fileItem, String uploadFieldId)
{
- File uploadFieldFolder = new File(getFolder(), uploadFieldId);
- uploadFieldFolder.mkdirs();
try
{
- IOUtils.copy(fileItem.getInputStream(), new FileOutputStream(new
File(uploadFieldFolder, fileItem.getClientFileName())));
+ Path baseFolderPath = getFolder().toPath().toRealPath();
+ java.io.File uploadFieldFolder = new File(getFolder(),
uploadFieldId).getCanonicalFile();
+ if (!uploadFieldFolder.toPath().startsWith(baseFolderPath))
+ {
+ throw new SecurityException("Path traversal detected in
uploadFieldId");
+ }
+ uploadFieldFolder.mkdirs();
Review Comment:
The result of `mkdirs()` is ignored.
##########
wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManager.java:
##########
@@ -78,6 +107,25 @@ public void save(FileUpload fileItem, String uploadFieldId)
@Override
public File getFile(String uploadFieldId, String clientFileName)
{
- return new File(new File(getFolder(), uploadFieldId), clientFileName);
+ try
+ {
+ Path baseFolderPath = getFolder().toPath().toRealPath();
+ java.io.File uploadFieldFolder = new File(getFolder(),
uploadFieldId).getCanonicalFile();
+ if (!uploadFieldFolder.toPath().startsWith(baseFolderPath))
+ {
+ throw new SecurityException("Path traversal detected in
uploadFieldId");
+ }
+ java.io.File target = new File(uploadFieldFolder,
clientFileName).getCanonicalFile();
+ Path uploadFieldFolderPath =
getPathForComparison(uploadFieldFolder);
+ if (!target.toPath().startsWith(uploadFieldFolderPath))
+ {
+ throw new SecurityException("Path traversal detected in client
filename");
+ }
+ return new File(target);
+ }
+ catch (IOException e)
+ {
+ throw new WicketRuntimeException(e);
+ }
}
Review Comment:
Unit tests could be added:
* uploadFieldId = "../escaped" — must throw SecurityException
* clientFileName = "../../etc/passwd" — must throw SecurityException
* Normal valid uploadFieldId and clientFileName — must succeed
##########
wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManager.java:
##########
@@ -60,14 +61,42 @@ public File getFolder() {
return folder;
}
+ /**
+ * Returns the path for a directory for use in path traversal checks. Uses
{@code toRealPath()}
+ * when the directory exists; otherwise falls back to {@code
toAbsolutePath().normalize()} to
+ * support validation when the directory has not yet been created.
+ */
+ private static Path getPathForComparison(java.io.File dir) throws
IOException
+ {
+ try
+ {
+ return dir.toPath().toRealPath();
+ }
+ catch (IOException e)
Review Comment:
Why IOExceptions are ignored here ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
