[cometvisu] add more path checks (#2696)

also deny external xml schema loading (avoid XXE attacks) Signed-off-by: Tobias Bräutigam <tbraeutigam@gmail.com>
openhab · Aug 8, 2024 · 5e53b21 · 5e53b21
1 parent 85cd64e
commit 5e53b21
Show file tree

Hide file tree

Showing 5 changed files with 19 additions and 5 deletions.
diff --git a/...ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/backend/rest/FsResource.java b/...ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/backend/rest/FsResource.java
@@ -116,6 +116,7 @@ public Response create(@QueryParam("path") String path, @QueryParam("type") Stri
     }
 
     @POST
+    @RolesAllowed({ Role.USER, Role.ADMIN })
     @Consumes(MediaType.MULTIPART_FORM_DATA)
     @Produces(MediaType.APPLICATION_JSON)
     @Operation(summary = "Create a binary file", responses = { @ApiResponse(responseCode = "200", description = "OK"),

diff --git a/...cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/backend/sitemap/VisuConfig.java b/...cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/backend/sitemap/VisuConfig.java
@@ -144,6 +144,7 @@ private String marshal(Pages bean, String xsdSchema) {
             classes[0] = bean.getClass();
             JAXBContext jaxbContext = JAXBContext.newInstance(bean.getClass());
             SchemaFactory schemaFactory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI);
+            schemaFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
             Schema schema = (xsdSchema == null || xsdSchema.trim().isEmpty()) ? null
                     : schemaFactory.newSchema(new File(xsdSchema));
             Marshaller marshaller = jaxbContext.createMarshaller();

diff --git a/...i.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/servlet/CometVisuServlet.java b/...i.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/servlet/CometVisuServlet.java
@@ -455,6 +455,10 @@ private void processStaticRequest(@Nullable File file, HttpServletRequest reques
         } else {
             processFile = file;
         }
+        if (!processFile.getCanonicalPath().startsWith(rootFolder.getCanonicalPath() + File.separator)) {
+            response.sendError(HttpServletResponse.SC_NOT_ACCEPTABLE);
+            return;
+        }
         if (processFile.equals(rootFolder) || (processFile.exists() && processFile.isDirectory())) {
             processFile = new File(file, "index.html");
         }

diff --git a/...org.openhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/util/FsUtil.java b/...org.openhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/util/FsUtil.java
@@ -140,16 +140,18 @@ public void saveFile(File file, InputStream fileInputStream, String hash) throws
 
             // 1. check if we need to create a backup
             boolean backup = false;
-            for (final Pattern pattern : settings.getBackupOnChange()) {
-                if (pattern.matcher(file.getName()).find()) {
-                    backup = true;
-                    break;
+            String name = file.getName();
+            if (name.length() < 100) {
+                for (final Pattern pattern : settings.getBackupOnChange()) {
+                    if (pattern.matcher(name).find()) {
+                        backup = true;
+                        break;
+                    }
                 }
             }
             boolean exists = file.exists();
             File backupFile = null;
             if (backup && exists) {
-                String name = file.getName();
                 int lastIndexOf = name.lastIndexOf(".");
                 String extension = lastIndexOf >= 0 ? name.substring(lastIndexOf) : "";
                 String filename = lastIndexOf == -1 ? name : name.substring(0, lastIndexOf);

diff --git a/...penhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/util/MountedFile.java b/...penhab.ui.cometvisu/src/main/java/org/openhab/ui/cometvisu/internal/util/MountedFile.java
@@ -106,6 +106,12 @@ private String normalize(String path) throws FileOperationException {
         if (normalizedPath.contains(".." + File.separator) || normalizedPath.contains(File.separator + "..")) {
             throw new FileOperationException("path not allowed", Status.NOT_ACCEPTABLE);
         }
+        if (File.separator.equals("\\")) {
+            // special case for windows that also supports "/" beneath the official file separator
+            if (normalizedPath.contains("../") || normalizedPath.contains("/..")) {
+                throw new FileOperationException("path not allowed", Status.NOT_ACCEPTABLE);
+            }
+        }
         return normalizedPath;
     }
 }