Merge pull request from GHSA-29g3-96g3-jg6c

Multiple vulnerability fixes and improvements

Author: anticomputer

api_test/main.c

@@ -1133,6 +1133,7 @@ int main() {
   int retval;
   test_batch_runner *runner = test_batch_runner_new();
 
+  cmark_init_standard_node_flags();
   version(runner);
   constructor(runner);
   accessors(runner);

extensions/autolink.c

@@ -35,79 +35,78 @@ static int sd_autolink_issafe(const uint8_t *link, size_t link_len) {
 }
 
 static size_t autolink_delim(uint8_t *data, size_t link_end) {
-  uint8_t cclose, copen;
   size_t i;
+  size_t closing = 0;
+  size_t opening = 0;
 
-  for (i = 0; i < link_end; ++i)
-    if (data[i] == '<') {
+  for (i = 0; i < link_end; ++i) {
+    const uint8_t c = data[i];
+    if (c == '<') {
       link_end = i;
       break;
+    } else if (c == '(') {
+      opening++;
+    } else if (c == ')') {
+      closing++;
     }
+  }
 
   while (link_end > 0) {
-    cclose = data[link_end - 1];
-
-    switch (cclose) {
+    switch (data[link_end - 1]) {
     case ')':
-      copen = '(';
-      break;
-    default:
-      copen = 0;
-    }
-
-    if (strchr("?!.,:*_~'\"", data[link_end - 1]) != NULL)
-      link_end--;
-
-    else if (data[link_end - 1] == ';') {
-      size_t new_end = link_end - 2;
-
-      while (new_end > 0 && cmark_isalpha(data[new_end]))
-        new_end--;
-
-      if (new_end < link_end - 2 && data[new_end] == '&')
-        link_end = new_end;
-      else
-        link_end--;
-    } else if (copen != 0) {
-      size_t closing = 0;
-      size_t opening = 0;
-      i = 0;
-
       /* Allow any number of matching brackets (as recognised in copen/cclose)
        * at the end of the URL.  If there is a greater number of closing
        * brackets than opening ones, we remove one character from the end of
        * the link.
        *
        * Examples (input text => output linked portion):
        *
-       *	http://www.pokemon.com/Pikachu_(Electric)
-       *		=> http://www.pokemon.com/Pikachu_(Electric)
+       *        http://www.pokemon.com/Pikachu_(Electric)
+       *                => http://www.pokemon.com/Pikachu_(Electric)
        *
-       *	http://www.pokemon.com/Pikachu_((Electric)
-       *		=> http://www.pokemon.com/Pikachu_((Electric)
+       *        http://www.pokemon.com/Pikachu_((Electric)
+       *                => http://www.pokemon.com/Pikachu_((Electric)
        *
-       *	http://www.pokemon.com/Pikachu_(Electric))
-       *		=> http://www.pokemon.com/Pikachu_(Electric)
+       *        http://www.pokemon.com/Pikachu_(Electric))
+       *                => http://www.pokemon.com/Pikachu_(Electric)
        *
-       *	http://www.pokemon.com/Pikachu_((Electric))
-       *		=> http://www.pokemon.com/Pikachu_((Electric))
+       *        http://www.pokemon.com/Pikachu_((Electric))
+       *                => http://www.pokemon.com/Pikachu_((Electric))
        */
-
-      while (i < link_end) {
-        if (data[i] == copen)
-          opening++;
-        else if (data[i] == cclose)
-          closing++;
-
-        i++;
+      if (closing <= opening) {
+        return link_end;
       }
+      closing--;
+      link_end--;
+      break;
+    case '?':
+    case '!':
+    case '.':
+    case ',':
+    case ':':
+    case '*':
+    case '_':
+    case '~':
+    case '\'':
+    case '"':
+      link_end--;
+      break;
+    case ';': {
+      size_t new_end = link_end - 2;
 
-      if (closing <= opening)
-        break;
+      while (new_end > 0 && cmark_isalpha(data[new_end]))
+        new_end--;
 
-      link_end--;
-    } else
+      if (new_end < link_end - 2 && data[new_end] == '&')
+        link_end = new_end;
+      else
+        link_end--;
       break;
+    }
+
+    default:
+      return link_end;
+    }
   }
 
   return link_end;
@@ -127,8 +126,17 @@ static size_t check_domain(uint8_t *data, size_t size, int allow_short) {
       break;
   }
 
-  if (uscore1 > 0 || uscore2 > 0)
-    return 0;
+  if (uscore1 > 0 || uscore2 > 0) {
+    /* If the url is very long then accept it despite the underscores,
+     * to avoid quadratic behavior causing a denial of service. See:
+     * https://github.com/github/cmark-gfm/security/advisories/GHSA-29g3-96g3-jg6c
+     * Reasonable urls are unlikely to have more than 10 segments, so
+     * this extra condition shouldn't have any impact on normal usage.
+     */
+    if (np <= 10) {
+      return 0;
+    }
+  }
 
   if (allow_short) {
     /* We don't need a valid domain in the strict sense (with
@@ -165,7 +173,7 @@ static cmark_node *www_match(cmark_parser *parser, cmark_node *parent,
   if (link_end == 0)
     return NULL;
 
-  while (link_end < size && !cmark_isspace(data[link_end]))
+  while (link_end < size && !cmark_isspace(data[link_end]) && data[link_end] != '<')
     link_end++;
 
   link_end = autolink_delim(data, link_end);
@@ -225,7 +233,7 @@ static cmark_node *url_match(cmark_parser *parser, cmark_node *parent,
     return 0;
 
   link_end += domain_len;
-  while (link_end < size && !cmark_isspace(data[link_end]))
+  while (link_end < size && !cmark_isspace(data[link_end]) && data[link_end] != '<')
     link_end++;
 
   link_end = autolink_delim(data, link_end);

extensions/strikethrough.c

@@ -67,6 +67,7 @@ static delimiter *insert(cmark_syntax_extension *self, cmark_parser *parser,
   strikethrough->end_column = closer->inl_text->start_column + closer->inl_text->as.literal.len - 1;
   cmark_node_free(closer->inl_text);
 
+done:
   delim = closer;
   while (delim != NULL && delim != opener) {
     tmp_delim = delim->previous;
@@ -76,7 +77,6 @@ static delimiter *insert(cmark_syntax_extension *self, cmark_parser *parser,
 
   cmark_inline_parser_remove_delimiter(inline_parser, opener);
 
-done:
   return res;
 }
 

extensions/table.c

@@ -11,13 +11,21 @@
 #include "table.h"
 #include "cmark-gfm-core-extensions.h"
 
+// Custom node flag, initialized in `create_table_extension`.
+static cmark_node__internal_flags CMARK_NODE__TABLE_VISITED;
+
 cmark_node_type CMARK_NODE_TABLE, CMARK_NODE_TABLE_ROW,
     CMARK_NODE_TABLE_CELL;
 
+typedef struct {
+  cmark_strbuf *buf;
+  int start_offset, end_offset, internal_offset;
+} node_cell;
+
 typedef struct {
   uint16_t n_columns;
   int paragraph_offset;
-  cmark_llist *cells;
+  node_cell *cells;
 } table_row;
 
 typedef struct {
@@ -29,24 +37,24 @@ typedef struct {
   bool is_header;
 } node_table_row;
 
-typedef struct {
-  cmark_strbuf *buf;
-  int start_offset, end_offset, internal_offset;
-} node_cell;
-
-static void free_table_cell(cmark_mem *mem, void *data) {
-  node_cell *cell = (node_cell *)data;
+static void free_table_cell(cmark_mem *mem, node_cell *cell) {
   cmark_strbuf_free((cmark_strbuf *)cell->buf);
   mem->free(cell->buf);
-  mem->free(cell);
+}
+
+static void free_row_cells(cmark_mem *mem, table_row *row) {
+  while (row->n_columns > 0) {
+    free_table_cell(mem, &row->cells[--row->n_columns]);
+  }
+  mem->free(row->cells);
+  row->cells = NULL;
 }
 
 static void free_table_row(cmark_mem *mem, table_row *row) {
   if (!row)
     return;
 
-  cmark_llist_free_full(mem, row->cells, (cmark_free_func)free_table_cell);
-
+  free_row_cells(mem, row);
   mem->free(row);
 }
 
@@ -111,6 +119,24 @@ static cmark_strbuf *unescape_pipes(cmark_mem *mem, unsigned char *string, bufsi
   return res;
 }
 
+// Adds a new cell to the end of the row. A pointer to the new cell is returned
+// for the caller to initialize.
+static node_cell* append_row_cell(cmark_mem *mem, table_row *row) {
+  const uint32_t n_columns = row->n_columns + 1;
+  // realloc when n_columns is a power of 2
+  if ((n_columns & (n_columns-1)) == 0) {
+    // make sure we never wrap row->n_columns
+    // offset will != len and our exit will clean up as intended
+    if (n_columns > UINT16_MAX) {
+      return NULL;
+    }
+    // Use realloc to double the size of the buffer.
+    row->cells = (node_cell *)mem->realloc(row->cells, (2 * n_columns - 1) * sizeof(node_cell));
+  }
+  row->n_columns = n_columns;
+  return &row->cells[n_columns-1];
+}
+
 static table_row *row_from_string(cmark_syntax_extension *self,
                                   cmark_parser *parser, unsigned char *string,
                                   int len) {
@@ -152,24 +178,22 @@ static table_row *row_from_string(cmark_syntax_extension *self,
           cell_matched);
       cmark_strbuf_trim(cell_buf);
 
-      node_cell *cell = (node_cell *)parser->mem->calloc(1, sizeof(*cell));
+      node_cell *cell = append_row_cell(parser->mem, row);
+      if (!cell) {
+        int_overflow_abort = 1;
+        cmark_strbuf_free(cell_buf);
+        parser->mem->free(cell_buf);
+        break;
+      }
       cell->buf = cell_buf;
       cell->start_offset = offset;
       cell->end_offset = offset + cell_matched - 1;
+      cell->internal_offset = 0;
 
-      while (cell->start_offset > 0 && string[cell->start_offset - 1] != '|') {
+      while (cell->start_offset > row->paragraph_offset && string[cell->start_offset - 1] != '|') {
         --cell->start_offset;
         ++cell->internal_offset;
       }
-
-      // make sure we never wrap row->n_columns
-      // offset will != len and our exit will clean up as intended
-      if (row->n_columns == UINT16_MAX) {
-          int_overflow_abort = 1;
-          break;
-      }
-      row->n_columns += 1;
-      row->cells = cmark_llist_append(parser->mem, row->cells, cell);
     }
 
     offset += cell_matched + pipe_matched;
@@ -187,9 +211,7 @@ static table_row *row_from_string(cmark_syntax_extension *self,
       if (row_end_offset && offset != len) {
         row->paragraph_offset = offset;
 
-        cmark_llist_free_full(parser->mem, row->cells, (cmark_free_func)free_table_cell);
-        row->cells = NULL;
-        row->n_columns = 0;
+        free_row_cells(parser->mem, row);
 
         // Scan past the (optional) leading pipe.
         offset += scan_table_cell_end(string, len, offset);
@@ -240,6 +262,10 @@ static cmark_node *try_opening_table_header(cmark_syntax_extension *self,
   const char *parent_string;
   uint16_t i;
 
+  if (parent_container->flags & CMARK_NODE__TABLE_VISITED) {
+    return parent_container;
+  }
+
   if (!scan_table_start(input, len, cmark_parser_get_first_nonspace(parser))) {
     return parent_container;
   }
@@ -267,6 +293,7 @@ static cmark_node *try_opening_table_header(cmark_syntax_extension *self,
     free_table_row(parser->mem, marker_row);
     free_table_row(parser->mem, header_row);
     cmark_arena_pop();
+    parent_container->flags |= CMARK_NODE__TABLE_VISITED;
     return parent_container;
   }
 
@@ -303,9 +330,8 @@ static cmark_node *try_opening_table_header(cmark_syntax_extension *self,
   // since we populate the alignments array based on marker_row->cells
   uint8_t *alignments =
       (uint8_t *)parser->mem->calloc(marker_row->n_columns, sizeof(uint8_t));
-  cmark_llist *it = marker_row->cells;
-  for (i = 0; it; it = it->next, ++i) {
-    node_cell *node = (node_cell *)it->data;
+  for (i = 0; i < marker_row->n_columns; ++i) {
+    node_cell *node = &marker_row->cells[i];
     bool left = node->buf->ptr[0] == ':', right = node->buf->ptr[node->buf->size - 1] == ':';
 
     if (left && right)
@@ -328,10 +354,8 @@ static cmark_node *try_opening_table_header(cmark_syntax_extension *self,
   ntr->is_header = true;
 
   {
-    cmark_llist *tmp;
-
-    for (tmp = header_row->cells; tmp; tmp = tmp->next) {
-      node_cell *cell = (node_cell *) tmp->data;
+    for (i = 0; i < header_row->n_columns; ++i) {
+      node_cell *cell = &header_row->cells[i];
       cmark_node *header_cell = cmark_parser_add_child(parser, table_header,
           CMARK_NODE_TABLE_CELL, parent_container->start_column + cell->start_offset);
       header_cell->start_line = header_cell->end_line = parent_container->start_line;
@@ -378,11 +402,10 @@ static cmark_node *try_opening_table_row(cmark_syntax_extension *self,
   }
 
   {
-    cmark_llist *tmp;
     int i, table_columns = get_n_table_columns(parent_container);
 
-    for (tmp = row->cells, i = 0; tmp && i < table_columns; tmp = tmp->next, ++i) {
-      node_cell *cell = (node_cell *) tmp->data;
+    for (i = 0; i < row->n_columns && i < table_columns; ++i) {
+      node_cell *cell = &row->cells[i];
       cmark_node *node = cmark_parser_add_child(parser, table_row_block,
           CMARK_NODE_TABLE_CELL, parent_container->start_column + cell->start_offset);
       node->internal_offset = cell->internal_offset;
@@ -785,6 +808,7 @@ static int escape(cmark_syntax_extension *self, cmark_node *node, int c) {
 cmark_syntax_extension *create_table_extension(void) {
   cmark_syntax_extension *self = cmark_syntax_extension_new("table");
 
+  cmark_register_node_flag(&CMARK_NODE__TABLE_VISITED);
   cmark_syntax_extension_set_match_block_func(self, matches);
   cmark_syntax_extension_set_open_block_func(self, try_opening_table_block);
   cmark_syntax_extension_set_get_type_string_func(self, get_type_string);