Better detection of structure in SVG leading to potential DOS

[maximmig]

The problem is in recursive growth of the number of elements due to nested elements creating multiple copies of the content. Eventually it leads to exponential increase in the number of items during rendering.

Nothing in the log. The SVG file is attached. GenerateXxeDos2_2.svg.txt

[weisJ]

Not sure if there is something to do here. If the SVG requests to paint that many elements it will take a lot of time to do so. Do you have something in mind how you would like this to be handled? In my opinion if the SVG content is not to be trusted then this should be handled on the application level. For example this can be achieved with a custom DomProcessor in the post-processing stage.

[maximmig]

Well, I understand the reasons for that answer, but at the same time it sounds like “with DDOS it will take a long time to process all those requests”. And then the server just can't serve the users.

I would do something like this, but I think the solution would be more efficient if it is incorporated in the library itself with all the inner details of SVG file processing and the architecture:

fun hasExcessiveUseExpansion(svgContent: ByteArray, maxDepth: Int = 5, maxCopies: Int = 100): Boolean {
  val document = try {
    DocumentBuilderFactory
      .newInstance()
      .newDocumentBuilder()
      .parse(ByteArrayInputStream(svgContent))
  } catch (t: Throwable) {
    Logger.error(t, "Failed to parse SVG image")
    return false
  }

  val useElements = document.getElementsByTagName("use").takeIf { it.length > 0 } ?: return false

  val graph = mutableMapOf<String, MutableList<String>>()

  (0..<useElements.length).forEach { i ->
    val useElement = useElements.item(i) as? Element ?: return@forEach
    val href = useElement.getAttribute("xlink:href").removePrefix("#")
    val parentId = (useElement.parentNode as? Element)?.getAttribute("id") ?: return@forEach

    graph.computeIfAbsent(parentId) { mutableListOf() }.add(href)
  }

  fun countCopies(node: String, currentDepth: Int): Int {
    if (currentDepth > maxDepth) return 0

    val directCopies = graph[node]?.sumOf { countCopies(it, currentDepth + 1) } ?: 1
    return directCopies.coerceAtMost(maxCopies)
  }

  return graph.keys.any { countCopies(it, 1) >= maxCopies }
}

[weisJ]

The problem here is that there are countless ways to trick these sort of checks. For example if your SVG above is modified so each #atk0 group wraps its content with another <g> indirection your code above breaks.

While I don't intend to tackle this soon I am open to accept PRs in this regard.

[weisJ]

Thinking about this a bit more I believe I have figured out how to do robust checking against large implicit use graphs (without actually having to traverse all of them during the check, which would cause hangs during loading).

[maximmig]

It would be even more interesting to see the code than the result 🙂