-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Knowage 8043 #891
Knowage 8043 #891
Changes from 5 commits
da96b6a
997e9ab
e3c6a3c
aaa3d09
8a66b78
09ea635
fd75ee0
c560439
0fab752
2284ba0
f302cdb
b168ec2
9bb7574
71231ad
b7ecf4e
c0dc7cc
f0f1db5
45a0eff
6520e49
ca94b0d
fe68d3d
6d9fa38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,6 @@ | |
import java.io.File; | ||
import java.io.FileOutputStream; | ||
import java.nio.file.Files; | ||
import java.nio.file.Paths; | ||
import java.text.SimpleDateFormat; | ||
import java.util.ArrayList; | ||
import java.util.Calendar; | ||
|
@@ -131,16 +130,15 @@ public Response getresourcePath(@QueryParam("templateName") String fileName, @Qu | |
String separator = File.separator; | ||
if (fileName.endsWith("?")) | ||
fileName = fileName.substring(0, fileName.length() - 1); | ||
|
||
java.nio.file.Path outputPath = Paths.get(SpagoBIUtilities.getResourcePath(), "dossier", String.valueOf(documentId), fileName); | ||
File file = outputPath.toFile(); | ||
String safeDirectory = SpagoBIUtilities.getResourcePath() + separator + "dossier" + separator + documentId; | ||
PathTraversalChecker.get(safeDirectory, fileName); | ||
String outPath = safeDirectory + separator + fileName; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Merge them in 1 line: PathTraversalChecker.get has to return a File or something similar There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
ResponseBuilder responseBuilder = null; | ||
|
||
File file = new File(outPath); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. merge this with the above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
JSONObject response = new JSONObject(); | ||
File dossierDir = new File(SpagoBIUtilities.getResourcePath() + separator + "dossier" + separator + documentId + separator); | ||
try { | ||
PathTraversalChecker.isValidFileName(fileName); | ||
PathTraversalChecker.preventPathTraversalAttack(file, dossierDir); | ||
PathTraversalChecker.get(fileName, dossierDir.getName()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. inputs reversed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
byte[] bytes = Files.readAllBytes(file.toPath()); | ||
responseBuilder = Response.ok(bytes); | ||
responseBuilder.header("Content-Disposition", "attachment; filename=" + fileName); | ||
|
@@ -171,8 +169,7 @@ public Response checkPathFile(@QueryParam("templateName") String fileName, @Quer | |
File dossierDir = new File(SpagoBIUtilities.getResourcePath() + separator + "dossier" + separator + documentId + separator); | ||
JSONObject response = new JSONObject(); | ||
try { | ||
PathTraversalChecker.isValidFileName(fileName); | ||
PathTraversalChecker.preventPathTraversalAttack(file, dossierDir); | ||
PathTraversalChecker.get(fileName, dossierDir.getName()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. inputs reversed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
Files.readAllBytes(file.toPath()); | ||
response.put("STATUS", "OK"); | ||
} catch (Exception e) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1621,9 +1621,12 @@ private void renameAndMoveDatasetFile(String originalFileName, String newFileNam | |
String filePath = resourcePath + File.separatorChar + "dataset" + File.separatorChar + "files" + File.separatorChar + "temp" + File.separatorChar; | ||
String fileNewPath = resourcePath + File.separatorChar + "dataset" + File.separatorChar + "files" + File.separatorChar; | ||
|
||
PathTraversalChecker.get(filePath, originalFileName); | ||
File originalDatasetFile = new File(filePath + originalFileName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. merge those lines There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
PathTraversalChecker.get(fileNewPath, newFileName + "." + fileType.toLowerCase()); | ||
File newDatasetFile = new File(fileNewPath + newFileName + "." + fileType.toLowerCase()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. merge those lines There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
if (originalDatasetFile.exists()) { | ||
|
||
if (newDatasetFile != null && originalDatasetFile != null && originalDatasetFile.exists()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how files can be null There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
/* | ||
* This method copies the contents of the specified source file to the specified destination file. The directory holding the destination file is | ||
* created if it does not exist. If the destination file exists, then this method will overwrite it. | ||
|
@@ -1741,8 +1744,10 @@ private JSONObject getCkanDataSetConfig(SelfServiceDataSetDTO selfServiceDataSet | |
private void deleteDatasetFile(String fileName, String resourcePath, String fileType) { | ||
String filePath = resourcePath + File.separatorChar + "dataset" + File.separatorChar + "files" + File.separatorChar + "temp" + File.separatorChar; | ||
|
||
PathTraversalChecker.get(filePath, fileName); | ||
File datasetFile = new File(filePath + fileName); | ||
if (datasetFile.exists()) { | ||
|
||
if (datasetFile != null && datasetFile.exists()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how files can be null There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
datasetFile.delete(); | ||
} | ||
} | ||
|
@@ -1796,10 +1801,7 @@ private Integer getCategoryCode(String category) { | |
|
||
try { | ||
ICategoryDAO categoryDao = DAOFactory.getCategoryDAO(); | ||
categories = categoryDao.getCategoriesForDataset() | ||
.stream() | ||
.map(Domain::fromCategory) | ||
.collect(toList()); | ||
categories = categoryDao.getCategoriesForDataset().stream().map(Domain::fromCategory).collect(toList()); | ||
} catch (Throwable t) { | ||
throw new SpagoBIRuntimeException("An unexpected error occured while loading categories types from database", t); | ||
} | ||
|
@@ -2174,10 +2176,7 @@ protected List<Integer> getCategories(IEngUserProfile profile) { | |
ICategoryDAO categoryDao = DAOFactory.getCategoryDAO(); | ||
|
||
// TODO : Makes sense? | ||
List<Domain> dialects = categoryDao.getCategoriesForDataset() | ||
.stream() | ||
.map(Domain::fromCategory) | ||
.collect(toList()); | ||
List<Domain> dialects = categoryDao.getCategoriesForDataset().stream().map(Domain::fromCategory).collect(toList()); | ||
if (dialects == null || dialects.size() == 0) { | ||
return null; | ||
} | ||
|
@@ -2191,10 +2190,7 @@ protected List<Integer> getCategories(IEngUserProfile profile) { | |
|
||
List<RoleMetaModelCategory> aRoleCategories = roledao.getMetaModelCategoriesForRole(role.getId()); | ||
List<RoleMetaModelCategory> resp = new ArrayList<>(); | ||
List<Domain> array = categoryDao.getCategoriesForDataset() | ||
.stream() | ||
.map(Domain::fromCategory) | ||
.collect(toList()); | ||
List<Domain> array = categoryDao.getCategoriesForDataset().stream().map(Domain::fromCategory).collect(toList()); | ||
for (RoleMetaModelCategory r : aRoleCategories) { | ||
for (Domain dom : array) { | ||
if (r.getCategoryId().equals(dom.getValueId())) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ | |
import org.apache.log4j.Logger; | ||
import org.json.JSONObject; | ||
|
||
import it.eng.knowage.commons.security.PathTraversalChecker; | ||
import it.eng.qbe.dataset.QbeDataSet; | ||
import it.eng.spago.base.SourceBean; | ||
import it.eng.spago.base.SourceBeanException; | ||
|
@@ -228,8 +229,11 @@ public ObjTemplate fromSDKTemplateToObjTemplate(SDKTemplate sdkTemplate) { | |
} | ||
if (dh != null && dh.getName() != null) { | ||
LOGGER.debug("Deleting attachment file ..."); | ||
File attachment = new File(dh.getName()); | ||
if (attachment.exists() && attachment.isFile()) { | ||
String fileName = dh.getName(); | ||
PathTraversalChecker.isValidFileName(fileName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fileName seems to be a complete path instead of a single file name. Please check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to talk about this in today's call. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. revert the changes, most likely this is a false positive |
||
File attachment = new File(fileName); | ||
|
||
if (attachment != null && attachment.exists() && attachment.isFile()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how files can be null There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
boolean attachmentFileDeleted; | ||
try { | ||
attachmentFileDeleted = Files.deleteIfExists(attachment.toPath()); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,7 @@ | |
|
||
import org.apache.log4j.Logger; | ||
|
||
import it.eng.knowage.commons.security.PathTraversalChecker; | ||
import it.eng.spago.security.IEngUserProfile; | ||
import it.eng.spagobi.services.proxy.DocumentExecuteServiceProxy; | ||
import it.eng.spagobi.utilities.mime.MimeUtils; | ||
|
@@ -78,16 +79,9 @@ public void service(HttpServletRequest request, HttpServletResponse response) { | |
imageFile = new File(completeImageFileName); | ||
|
||
File parent = imageFile.getParentFile(); | ||
// Prevent directory traversal (path traversal) attacks | ||
if (!imageTmpDir.equals(parent)) { | ||
logger.error("Trying to access the file [" + imageFile.getAbsolutePath() + "] that is not inside ${java.io.tmpdir}/birt!!!"); | ||
throw new SecurityException("Trying to access the file [" + imageFile.getAbsolutePath() + "] that is not inside ${java.io.tmpdir}/birt!!!"); | ||
} | ||
String fileName = imageFile.toString(); | ||
|
||
if (!imageFile.exists()) { | ||
logger.error("File " + imageFile.getPath() + " not found"); | ||
return; | ||
} | ||
davide-zerbetto marked this conversation as resolved.
Show resolved
Hide resolved
|
||
PathTraversalChecker.get(parent.toString(), fileName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. parent is NOT a safe dir, imageDirectory or imageTmpDir are safe. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to talk about this in today's call. |
||
|
||
try { | ||
fis = new FileInputStream(imageFile); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
|
||
import org.apache.log4j.Logger; | ||
|
||
import it.eng.knowage.commons.security.PathTraversalChecker; | ||
import it.eng.spagobi.engines.birt.BirtReportServlet; | ||
|
||
public class Utils { | ||
|
@@ -36,8 +37,7 @@ public class Utils { | |
/** | ||
* Resolve system properties. | ||
* | ||
* @param logDir | ||
* the log dir | ||
* @param logDir the log dir | ||
* | ||
* @return the string | ||
*/ | ||
|
@@ -54,10 +54,8 @@ public static String resolveSystemProperties(String logDir) { | |
/** | ||
* Resolve system properties. | ||
* | ||
* @param logDir | ||
* the log dir | ||
* @param startIndex | ||
* the start index | ||
* @param logDir the log dir | ||
* @param startIndex the start index | ||
* | ||
* @return the string | ||
*/ | ||
|
@@ -89,14 +87,10 @@ public static void sendPage(HttpServletResponse response, int pageNumber, String | |
String completeImageFileName = null; | ||
String mimeType = "text/html"; | ||
|
||
htmlFile = new File(BirtReportServlet.OUTPUT_FOLDER + File.separator + reportExecutionId, BirtReportServlet.PAGE_FILE_NAME + pageNumber + ".html"); | ||
|
||
// file path traversal security check | ||
if (!((htmlFile.getParentFile().getParent() + File.separator).equals(BirtReportServlet.OUTPUT_FOLDER))) { | ||
logger.error("Security exception: parent folder " + htmlFile.getParent() + " is not equal to expected folder " + BirtReportServlet.OUTPUT_FOLDER); | ||
throw new RuntimeException( | ||
"Security exception: parent folder " + htmlFile.getParent() + " is not equal to expected folder " + BirtReportServlet.OUTPUT_FOLDER); | ||
} | ||
String directory = BirtReportServlet.OUTPUT_FOLDER + File.separator + reportExecutionId; | ||
String fileName = BirtReportServlet.PAGE_FILE_NAME + pageNumber + ".html"; | ||
PathTraversalChecker.get(fileName, directory); | ||
htmlFile = new File(directory, fileName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. merge those 2 lines There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
|
||
try (InputStream fis = new FileInputStream(htmlFile);) { | ||
writeToOutput(response, mimeType, fis); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,8 @@ | |
|
||
import org.apache.log4j.Logger; | ||
|
||
import it.eng.knowage.commons.security.PathTraversalChecker; | ||
|
||
public class WorksRepository { | ||
private static final Logger LOGGER = Logger.getLogger(WorksRepository.class); | ||
|
||
|
@@ -68,7 +70,6 @@ public File getExecutableWorkProjectDir(CommonjWork work) { | |
return worksDir; | ||
} | ||
|
||
|
||
/** | ||
* Gets the executable work dir. | ||
* | ||
|
@@ -78,12 +79,15 @@ public File getExecutableWorkProjectDir(CommonjWork work) { | |
*/ | ||
public File getExecutableWorkDir(CommonjWork work) { | ||
LOGGER.debug("IN"); | ||
File workDir = new File(rootDir, work.getWorkName()); | ||
|
||
String fileName = work.getWorkName(); | ||
PathTraversalChecker.get(rootDir.getName(), fileName); | ||
File workDir = new File(rootDir, fileName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. merge those 2 lines There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
|
||
LOGGER.debug("OUT"); | ||
return workDir; | ||
} | ||
|
||
|
||
/** | ||
* Gets the executable work file. | ||
* | ||
|
@@ -103,9 +107,12 @@ public File getExecutableWorkFile(CommonjWork work) { | |
* @return true, if successful | ||
*/ | ||
public boolean containsWork(CommonjWork work) { | ||
String fileName = work.getWorkName(); | ||
|
||
PathTraversalChecker.get(rootDir.getName(), fileName); | ||
File workFolder = new File(rootDir, fileName); | ||
|
||
File workFolder=new File(rootDir, work.getWorkName()); | ||
return workFolder.exists(); | ||
return workFolder != null && workFolder.exists(); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't make sense: inputs are reversed, since targetDirectory is the safe dir and filename is what has to be checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed