Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,25 @@ public void execute(Database database) throws CustomChangeException {
* @return a concept id.
* @throws CustomChangeException
*/
private Integer findConceptByName(JdbcConnection connection, Map<String, String[]> names) throws CustomChangeException {
private Integer findConceptByName(JdbcConnection connection, Map<String, String[]> names)
throws CustomChangeException {
for (Map.Entry<String, String[]> e : names.entrySet()) {
String locale = e.getKey();

for (String name : e.getValue()) {
Integer ret = getInt(connection, "select concept_id from concept_name where name = '" + name
+ "' and locale like '" + locale + "%'");
if (ret != null) {
return ret;
String sql = "select concept_id from concept_name where name = ? and locale like ?";

try (PreparedStatement ps = connection.prepareStatement(sql)) {
ps.setString(1, name);
ps.setString(2, locale + "%");

try (ResultSet rs = ps.executeQuery()) {
if (rs.next()) {
return rs.getInt(1);
}
}
} catch (Exception ex) {
throw new CustomChangeException("Error querying concept by name", ex);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,16 @@ protected File getFile(HttpServletRequest request) {
}

realPath = realPath.replace("/", File.separator);

File f = new File(realPath);
// fix
String safePath = realPath;
if (safePath.contains("..")) {
throw new UnsupportedOperationException(
"Attempted to access file '" + safePath +
"' but this is rejected because it contains '..' which may indicate " +
"a path traversal or zip-slip style attack."
);
}
File f = new File(safePath);
if (!f.exists()) {
log.warn("No file with path '" + realPath + "' exists for module '" + module.getModuleId() + "'");
return null;
Expand Down
36 changes: 32 additions & 4 deletions web/src/main/java/org/openmrs/module/web/WebModuleUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,13 @@ public static boolean startModule(Module mod, ServletContext servletContext, boo
log.debug("Moving file from: {} to {}", name, absPath);

// get the output file
File outFile = new File(absPath.toString().replace("/", File.separator));
String safePath = absPath.toString();

// basic path traversal protection
if (safePath.contains("..") || safePath.contains("\\") || safePath.contains("//")) {
throw new IllegalArgumentException("Unsafe file path detected: " + safePath);
}
File outFile = new File(safePath.replace("/", File.separator));
if (entry.isDirectory()) {
if (!outFile.exists()) {
outFile.mkdirs();
Expand Down Expand Up @@ -782,7 +788,15 @@ public static void shutdownModules(ServletContext servletContext) {

// clear the module messages
String messagesPath = realPath + "/WEB-INF/";
File folder = new File(messagesPath.replace("/", File.separator));
// Normalize separators
Copy link
Contributor

Choose a reason for hiding this comment

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

Kindly the comments here don’t seem to add extra value so it would be better to remove them and rely on the code readability instead.

String safePath = messagesPath.replace("/", File.separator);

// Detect basic path traversal patterns
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

if (safePath.contains("..")) {
throw new IllegalArgumentException("Invalid path: potential path traversal detected: " + safePath);
}

File folder = new File(safePath);

File[] files = folder.listFiles();
if (folder.exists() && files != null) {
Expand Down Expand Up @@ -833,8 +847,22 @@ public static void stopModule(Module mod, ServletContext servletContext, boolean
String realPath = getRealPath(servletContext);

// delete the web files from the webapp
String absPath = realPath + "/WEB-INF/view/module/" + moduleId;
File moduleWebFolder = new File(absPath.replace("/", File.separator));
String absPath = realPath + "/WEB-INF/web/module/" + moduleId;

// normalize path separator
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Kindly remove all the comments that starts with //.

String safePath = absPath.replace("/", File.separator);

// Basic path traversal protection
if (safePath.contains("..")) {
throw new UnsupportedOperationException(
"Attempted to access '" + safePath +
"' but this was rejected because it contains '..', which may indicate " +
"a path traversal or zip-slip style attack."
);
}

File moduleWebFolder = new File(safePath);

if (moduleWebFolder.exists()) {
try {
OpenmrsUtil.deleteDirectory(moduleWebFolder);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,9 @@ protected void doGet(HttpServletRequest httpRequest, HttpServletResponse httpRes
}

PrintWriter writer = httpResponse.getWriter();
writer.write(toJSONString(result));
String jsonText = toJSONString(result);
jsonText = org.apache.commons.text.StringEscapeUtils.escapeHtml4(jsonText);
writer.write(jsonText);
writer.close();
} else if (InitializationWizardModel.INSTALL_METHOD_AUTO.equals(wizardModel.installMethod)
|| httpRequest.getServletPath().equals("/" + AUTO_RUN_OPENMRS)) {
Expand Down Expand Up @@ -1039,6 +1041,10 @@ private File getRuntimePropertiesFile() {

String pathName = OpenmrsUtil.getRuntimePropertiesFilePathName(WebConstants.WEBAPP_NAME);
if (pathName != null) {
// Block path traversal attempts
if (pathName.contains("..") || pathName.contains("/") || pathName.contains("\\")) {
throw new IllegalArgumentException("Unsafe path detected: " + pathName);
}
file = new File(pathName);
} else {
file = new File(OpenmrsUtil.getApplicationDataDirectory(), getRuntimePropertiesFileName());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,11 @@ protected static boolean addZippedTestModules(InputStream in) {
//Convert the names of .omod files located in nested directories so that they get
//created under the module repo directory when being copied
if (fileName.contains(System.getProperty("file.separator"))) {
fileName = new File(entry.getName()).getName();
String entryName = entry.getName();
if (entryName.contains("..") || entryName.contains("/") || entryName.contains("\\")) {
throw new IOException("Blocked unsafe zip entry name: " + entryName);
}
fileName = new File(entryName).getName();
}

log.debug("Extracting module file: {}", fileName);
Expand All @@ -186,6 +190,13 @@ protected static boolean addZippedTestModules(InputStream in) {
FileUtils.cleanDirectory(moduleRepository);

final File zipEntryFile = new File(moduleRepository, fileName);

String canonicalRepoPath = moduleRepository.getCanonicalPath();
String canonicalTargetPath = zipEntryFile.getCanonicalPath();

if (!canonicalTargetPath.startsWith(canonicalRepoPath)) {
throw new IOException("Blocked unsafe file path: " + fileName);
}

if (!zipEntryFile.toPath().normalize().startsWith(moduleRepository.toPath().normalize())) {
throw new IOException("Bad zip entry");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,10 @@ else if (REVIEW_CHANGES.equals(page)) {

addLogLinesToResponse(result);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there away you can fix the format changes please?


String jsonText = toJSONString(result);
httpResponse.setContentType("application/json;charset=UTF-8");
jsonText = org.apache.commons.text.StringEscapeUtils.escapeHtml4(jsonText);
httpResponse.getWriter().write(jsonText);
}
}
Expand Down