Fix ReDoS, stack-overflow, and CSS-injection in Markdown rendering

src/main/java/io/github/gitbucket/markedj/Grammer.java

@@ -76,7 +76,13 @@ private static String removeLineStart(String regex){
     public static Map<String, Rule> INLINE_BREAKS_RULES = new HashMap<>();
 
     public static String INLINE_ESCAPE = "^\\\\([\\\\`*{}\\[\\]()#+\\-.!_>])";
-    public static String INLINE_TEXT   = "^[\\s\\S]+?(?=[\\\\<!\\[_*`]| {2,}\\n|$)";
+    // The trailing-space branch is bounded (rather than " {2,}") because an unbounded
+    // quantifier here is re-probed by the lazy `[\s\S]+?` loop at every character of a
+    // long run of spaces, turning a single "hard line break" check into O(n^2) work
+    // over a long run of trailing whitespace with no following newline. Two or more
+    // spaces is all CommonMark/GFM ever require to recognize a hard break, so bounding
+    // the run length doesn't change behavior for any real input.
+    public static String INLINE_TEXT   = "^[\\s\\S]+?(?=[\\\\<!\\[_*`]| {2,20}\\n|$)";
     public static String INLINE_BR     = "^( {2,}|\\\\)\\n(?!\\s*$)";
 
     static {
@@ -102,7 +108,10 @@ private static String removeLineStart(String regex){
 
         INLINE_BREAKS_RULES.putAll(INLINE_GFM_RULES);
         INLINE_BREAKS_RULES.put("br", new FindFirstRule(INLINE_BR.replace("{2,}", "*")));
-        INLINE_BREAKS_RULES.put("text", new FindFirstRule(INLINE_TEXT.replace("]|", "~]|").replace("|", "|https?://|").replace("{2,}", "*")));
+        // Use "{0,20}" here, not "*": breaks mode only needs to lower the minimum from
+        // 2 spaces to 0 (any single newline is a break), not drop the upper bound that
+        // keeps this lookahead from being the same O(n^2) trap described above.
+        INLINE_BREAKS_RULES.put("text", new FindFirstRule(INLINE_TEXT.replace("]|", "~]|").replace("|", "|https?://|").replace("{2,20}", "{0,20}")));
     }
 
 }

src/main/java/io/github/gitbucket/markedj/Lexer.java

@@ -10,8 +10,17 @@
 
 public class Lexer {
 
+    // token() recurses once per nesting level for blockquotes and list items (and for
+    // any Extension that re-enters via TokenConsumer). With no limit, a document with a
+    // few thousand nesting levels overflows the JVM call stack with an uncaught
+    // StackOverflowError instead of a catchable failure. This bounds recursion to a
+    // depth no real-world document comes close to, and degrades gracefully by rendering
+    // anything nested deeper than this as plain text rather than crashing.
+    private static final int MAX_NESTING_DEPTH = 100;
+
     protected Options options;
     protected Map<String, Rule> rules = null;
+    private int depth = 0;
 
     public Lexer(Options options){
         this.options = options;
@@ -39,6 +48,21 @@ public LexerResult lex(String src){
     }
 
     protected void token(String src, boolean top, boolean bq, LexerContext context){
+        depth++;
+        try {
+            if(depth > MAX_NESTING_DEPTH){
+                if(!src.isEmpty()){
+                    context.pushToken(new ParagraphToken(src));
+                }
+                return;
+            }
+            tokenInternal(src, top, bq, context);
+        } finally {
+            depth--;
+        }
+    }
+
+    private void tokenInternal(String src, boolean top, boolean bq, LexerContext context){
         while(src.length() > 0){
             // newline
             {

src/main/java/io/github/gitbucket/markedj/Options.java

@@ -3,6 +3,9 @@
 import io.github.gitbucket.markedj.extension.Extension;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.regex.Pattern;
+import org.jsoup.nodes.Attribute;
+import org.jsoup.nodes.Element;
 import org.jsoup.safety.Safelist;
 
 public class Options {
@@ -13,7 +16,28 @@ public class Options {
     private boolean sanitize = false;
     private String langPrefix = "lang-";
     private String headerPrefix = "";
-    private Safelist safelist = new Safelist()
+
+    // jsoup only checks whether "style" may exist on a tag, never what CSS it contains,
+    // so allowing it unconditionally would let raw HTML in markdown carry arbitrary CSS
+    // (clickjacking overlays via position:fixed, data exfiltration via attribute
+    // selectors, etc). The only legitimate producer of "style" in this library is
+    // Renderer.tablecell()'s table-column alignment, so that exact shape is all that's
+    // allowed through; everything else is stripped.
+    private static final Pattern SAFE_STYLE_VALUE =
+            Pattern.compile("^text-align: (left|right|center)$");
+
+    private Safelist safelist = new Safelist() {
+                @Override
+                public boolean isSafeAttribute(String tagName, Element el, Attribute attr) {
+                    if (!super.isSafeAttribute(tagName, el, attr)) {
+                        return false;
+                    }
+                    if ("style".equals(attr.getKey())) {
+                        return SAFE_STYLE_VALUE.matcher(attr.getValue()).matches();
+                    }
+                    return true;
+                }
+            }
                 .addTags(
                         "a", "b", "blockquote", "br", "caption", "cite", "code", "col",
                                 "colgroup", "dd", "div", "dl", "dt", "em", "h1", "h2", "h3", "h4", "h5", "h6",

src/test/java/io/github/gitbucket/markedj/MarkedTest.java

@@ -273,11 +273,82 @@ public void testHardLineBreakWithSpaces() {
                 " Line 2</p>", result);
     }
 
+    // Options' default Safelist allows the "style" attribute on every tag (":all"),
+    // but jsoup only checks whether an attribute may exist -- it never validates CSS
+    // *content*. Arbitrary attacker-controlled style is a real primitive (clickjacking
+    // overlays via `position:fixed`, data exfiltration via CSS attribute selectors),
+    // reachable via any raw HTML in markdown (sanitize=false is also the default).
+    // The only legitimate producer of `style` in this library is
+    // Renderer.tablecell()'s "text-align: left|right|center", so nothing but that
+    // exact shape should ever survive.
+    @Test
+    public void testStyleAttributeRejectsArbitraryCss() throws Exception {
+        String result = Marked.marked(
+                "<div style=\"position:fixed;top:0;left:0;width:100%;height:100%;" +
+                "background:url(javascript:alert(1))\">clickjack</div>");
+        assertEquals("<div>\n clickjack\n</div>", result);
+    }
+
+    // The fix for the above must not break the one legitimate use of `style`:
+    // column alignment on tables.
+    @Test
+    public void testStyleAttributeAllowsTableAlignment() throws Exception {
+        String result = Marked.marked("|A|B|\n|--:|:--|\n|1|2|");
+        assertTrue(result.contains("style=\"text-align: right\""));
+        assertTrue(result.contains("style=\"text-align: left\""));
+    }
+
     @Test
     public void testHardLineBreakWithBackslash() {
         String result = Marked.marked("Line 1\\\n" +
                 "Line 2");
         assertEquals("<p>Line 1<br>\n" +
                 " Line 2</p>", result);
     }
+
+    // Reproduces a quadratic-time blowup in the inline "text" rule (Grammer.INLINE_TEXT).
+    // A single character followed by a long run of trailing spaces and no terminating
+    // newline forces the lazy `[\s\S]+?` loop to re-probe the ` {2,}\n` lookahead at
+    // every one of the ~50,000 positions, and each probe rescans the remaining space
+    // run looking for a `\n` that never comes -- O(n^2) total work from one paragraph.
+    // This is trivially reachable via any GitBucket issue/PR/comment/wiki body.
+    @Test(timeout = 2000)
+    public void testLongTrailingSpacesDoesNotHang() {
+        StringBuilder sb = new StringBuilder("x");
+        for (int i = 0; i < 50_000; i++) {
+            sb.append(' ');
+        }
+        Marked.marked(sb.toString());
+    }
+
+    // Same reproduction as testLongTrailingSpacesDoesNotHang, but with breaks=true.
+    // INLINE_BREAKS_RULES built its "text" rule by replacing the bounded "{2,20}"
+    // quantifier with an unbounded "*", silently re-introducing the O(n^2) blowup
+    // the bound above was added to fix, only reachable through this option.
+    @Test(timeout = 2000)
+    public void testLongTrailingSpacesDoesNotHangWithBreaks() {
+        StringBuilder sb = new StringBuilder("x");
+        for (int i = 0; i < 50_000; i++) {
+            sb.append(' ');
+        }
+        Options options = new Options();
+        options.setBreaks(true);
+        Marked.marked(sb.toString(), options);
+    }
+
+    // Lexer.token() recurses once per nesting level of "> " to handle nested
+    // blockquotes, with no depth limit. A few thousand nesting levels (trivially
+    // reachable via any GitBucket issue/PR/comment/wiki body) blow the JVM call
+    // stack with an uncaught StackOverflowError, which -- unlike a checked
+    // exception -- will not be caught by ordinary `catch (Exception e)` error
+    // handling around the rendering call, and can crash the calling thread.
+    @Test
+    public void testDeeplyNestedBlockquoteDoesNotOverflow() {
+        StringBuilder sb = new StringBuilder();
+        for (int i = 0; i < 5_000; i++) {
+            sb.append("> ");
+        }
+        sb.append("hello");
+        Marked.marked(sb.toString());
+    }
 }