CVE-2020-5230: Improper Neutralization of Special Elements in Output Used by a Downstream Component ('Injection') (CWE-74)
value: Opencast before 8.1 and 7.6 allows almost arbitrary identifiers for media packages and elements to be used. This can be problematic for operation and security since such identifiers are sometimes used for file system operations which may lead to an attacker being able to escape working directories and write files to other locations. In addition, Opencast's Id.toString(…) vs Id.compact(…) behavior, the latter trying to mitigate some of the file system problems, can cause errors due to identifier mismatch since an identifier may unintentionally change. This issue is fixed in Opencast 7.6 and 8.1.
CWE description: The product constructs all or part of a command, data structure, or record using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify how it is parsed or interpreted when it is sent to a downstream component.
CVSS v3 base score: 7.5
Published: 2020-01-30T21:15Z
Repository: https://github.com/opencast/opencast
Language: Java
Files changed: modules/common/src/main/java/org/opencastproject/mediapackage/identifier/Id.java, modules/common/src/main/java/org/opencastproject/mediapackage/identifier/IdImpl.java, modules/ingest-service-impl/src/main/java/org/opencastproject/ingest/endpoint/IngestRestService.java
Commit date: 2020-01-29 11:25:38 +0100
Commit message: Limit Characters Allowed In Ids

Opencast allows almost arbitrary identifiers for media packages and
elements to be used. This can be problematic for operation and security
since such identifiers are sometimes used for file system operations
which may lead to attacker being able to escape working directories and
write files to other locations.

In addition, Opencast's `Id.toString(…)` vs `Id.compact(…)` behavior,
the latter trying to mitigate some of the file system problems, can
cause errors due to identifier mismatch since an identifier may
unintentionally change.

This patch limits the characters allowed to be used in identifiers to
ensure no unsafe operations are possible and no mismatch may happen.

Diff: diff --git a/modules/common/src/main/java/org/opencastproject/mediapackage/identifier/Id.java b/modules/common/src/main/java/org/opencastproject/mediapackage/identifier/Id.java
index 7d0d42688f..f74c7d7ec5 100644
--- a/modules/common/src/main/java/org/opencastproject/mediapackage/identifier/Id.java
+++ b/modules/common/src/main/java/org/opencastproject/mediapackage/identifier/Id.java
@@ -39,10 +39,11 @@ public interface Id {
    * <code>ETH</code> the authority and <code>abcd</code> the local part. <code>toURI()</code> would then return
    * <code>10.3930-ETH-abcd</code> or any other suitable form.
    *
    * @return a path separator-free representation of the identifier
    */
+  @Deprecated
   String compact();
 
   class Adapter extends XmlAdapter<IdImpl, Id> {
     public IdImpl marshal(Id id) throws Exception {
       if (id instanceof IdImpl) {
diff --git a/modules/common/src/main/java/org/opencastproject/mediapackage/identifier/IdImpl.java b/modules/common/src/main/java/org/opencastproject/mediapackage/identifier/IdImpl.java
index 4eb1c09d38..c465ca002d 100644
--- a/modules/common/src/main/java/org/opencastproject/mediapackage/identifier/IdImpl.java
+++ b/modules/common/src/main/java/org/opencastproject/mediapackage/identifier/IdImpl.java
@@ -20,10 +20,12 @@
  */
 
 
 package org.opencastproject.mediapackage.identifier;
 
+import java.util.regex.Pattern;
+
 import javax.xml.bind.annotation.XmlAccessType;
 import javax.xml.bind.annotation.XmlAccessorType;
 import javax.xml.bind.annotation.XmlType;
 import javax.xml.bind.annotation.XmlValue;
 
@@ -32,10 +34,12 @@ import javax.xml.bind.annotation.XmlValue;
  */
 @XmlType
 @XmlAccessorType(XmlAccessType.NONE)
 public class IdImpl implements Id {
 
+  private static final Pattern pattern = Pattern.compile("[\\w-_.:;()]+");
+
   /** The identifier */
   @XmlValue
   protected String id = null;
 
   /**
@@ -48,21 +52,24 @@ public class IdImpl implements Id {
    * Creates a new identifier.
    *
    * @param id
    *          the identifier
    */
-  public IdImpl(String id) {
+  public IdImpl(final String id) {
+    if (!pattern.matcher(id).matches()) {
+      throw new IllegalArgumentException("Id must match " + pattern);
+    }
     this.id = id;
   }
 
   /**
    * {@inheritDoc}
    *
    * @see org.opencastproject.mediapackage.identifier.Id#compact()
    */
   public String compact() {
-    return id.replaceAll("/", "-").replaceAll("\\\\", "-");
+    return toString();
   }
 
   @Override
   public String toString() {
     return id;
diff --git a/modules/ingest-service-impl/src/main/java/org/opencastproject/ingest/endpoint/IngestRestService.java b/modules/ingest-service-impl/src/main/java/org/opencastproject/ingest/endpoint/IngestRestService.java
index 115b7e31ec..4f46033c53 100644
--- a/modules/ingest-service-impl/src/main/java/org/opencastproject/ingest/endpoint/IngestRestService.java
+++ b/modules/ingest-service-impl/src/main/java/org/opencastproject/ingest/endpoint/IngestRestService.java
@@ -835,15 +835,18 @@ public class IngestRestService extends AbstractJobProducerEndpoint {
         } else if (episodeDCCatalogNumber == 0) {
           logger.warn("Rejected ingest without episode metadata. At least provide a title.");
           return Response.serverError().status(Status.BAD_REQUEST).build();
         }
 
-        WorkflowInstance workflow = (wdID == null) ? ingestService.ingest(mp) : ingestService.ingest(mp, wdID,
-                workflowProperties);
+        WorkflowInstance workflow = (wdID == null)
+            ? ingestService.ingest(mp)
+            : ingestService.ingest(mp, wdID, workflowProperties);
         return Response.ok(workflow).build();
       }
       return Response.serverError().status(Status.BAD_REQUEST).build();
+    } catch (IllegalArgumentException e) {
+      return Response.status(Status.BAD_REQUEST).entity(e.getMessage()).build();
     } catch (Exception e) {
       logger.warn(e.getMessage(), e);
       return Response.serverError().status(Status.INTERNAL_SERVER_ERROR).build();
     }
   }

Vulnerable code:   public IdImpl(String id) {
    return id.replaceAll("/", "-").replaceAll("\\\\", "-");
        WorkflowInstance workflow = (wdID == null) ? ingestService.ingest(mp) : ingestService.ingest(mp, wdID,
                workflowProperties);
Fixed code:   @Deprecated
import java.util.regex.Pattern;

  private static final Pattern pattern = Pattern.compile("[\\w-_.:;()]+");

  public IdImpl(final String id) {
    if (!pattern.matcher(id).matches()) {
      throw new IllegalArgumentException("Id must match " + pattern);
    }
    return toString();
        WorkflowInstance workflow = (wdID == null)
            ? ingestService.ingest(mp)
            : ingestService.ingest(mp, wdID, workflowProperties);
    } catch (IllegalArgumentException e) {
      return Response.status(Status.BAD_REQUEST).entity(e.getMessage()).build();