Skip to content
Permalink
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
lkiesow committed Jan 29, 2020
1 parent 74bfb70 commit bbb473f
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 4 deletions.
Expand Up @@ -41,6 +41,7 @@ public interface Id {
*
* @return a path separator-free representation of the identifier
*/
@Deprecated
String compact();

class Adapter extends XmlAdapter<IdImpl, Id> {
Expand Down
Expand Up @@ -22,6 +22,8 @@

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;
Expand All @@ -34,6 +36,8 @@
@XmlAccessorType(XmlAccessType.NONE)
public class IdImpl implements Id {

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

/** The identifier */
@XmlValue
protected String id = null;
Expand All @@ -50,7 +54,10 @@ public IdImpl() {
* @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;
}

Expand All @@ -60,7 +67,7 @@ public IdImpl(String id) {
* @see org.opencastproject.mediapackage.identifier.Id#compact()
*/
public String compact() {
return id.replaceAll("/", "-").replaceAll("\\\\", "-");
return toString();
}

@Override
Expand Down
Expand Up @@ -837,11 +837,14 @@ public Response addMediaPackage(@Context HttpServletRequest request, @PathParam(
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();
Expand Down

0 comments on commit bbb473f

Please sign in to comment.