Merge pull request #376 from sehe/pr-364

Fix security issue #364 and non-keyword subgraph parsing

Author: jeremy-murphy

src/read_graphviz_new.cpp

@@ -53,6 +53,7 @@ namespace boost
 
 namespace read_graphviz_detail
 {
+    static const long max_subgraph_nesting_level = 255;
     struct token
     {
         enum token_type
@@ -207,7 +208,7 @@ namespace read_graphviz_detail
 
         tokenizer(const std::string& str) : begin(str.begin()), end(str.end())
         {
-            std::string end_of_token = "(?=(?:\\W))";
+            // std::string end_of_token = "(?=(?:\\W))"; // SEHE: unused?
             std::string whitespace = "(?:\\s+)";
             std::string slash_slash_comment = "(?://.*?$)";
             std::string slash_star_comment = "(?:/\\*.*?\\*/)";
@@ -527,6 +528,7 @@ namespace read_graphviz_detail
         std::map< subgraph_name, subgraph_info > subgraphs;
         std::string current_subgraph_name;
         int sgcounter; // Counter for anonymous subgraphs
+        long sgnesting_level;
         std::set< std::pair< node_name, node_name > >
             existing_edges; // Used for checking in strict graphs
 
@@ -538,7 +540,7 @@ namespace read_graphviz_detail
         subgraph_member_list& current_members() { return current().members; }
 
         parser(const std::string& gr, parser_result& result)
-        : the_tokenizer(gr), lookahead(), r(result), sgcounter(0)
+        : the_tokenizer(gr), lookahead(), r(result), sgcounter(0), sgnesting_level(0)
         {
             current_subgraph_name = "___root___";
             current() = subgraph_info(); // Initialize root graph
@@ -773,10 +775,18 @@ namespace read_graphviz_detail
             bool is_anonymous = true;
             if (first_token.type == token::kw_subgraph)
             {
-                if (peek().type == token::identifier)
+                switch (peek().type)
                 {
+                case token::identifier:
                     name = get().normalized_value;
                     is_anonymous = false;
+                    break;
+                case token::left_brace:
+                    is_anonymous = true;
+                    break;
+                default:
+                    error("Subgraph reference needs a name");
+                    break;
                 }
             }
             if (is_anonymous)
@@ -790,25 +800,30 @@ namespace read_graphviz_detail
                     = current(); // Initialize properties and defaults
                 subgraphs[name].members.clear(); // Except member list
             }
-            if (first_token.type == token::kw_subgraph
-                && peek().type != token::left_brace)
+            if (!is_anonymous && peek().type != token::left_brace)
             {
-                if (is_anonymous)
-                    error("Subgraph reference needs a name");
                 return name;
             }
             subgraph_name old_sg = current_subgraph_name;
+            if (++sgnesting_level > max_subgraph_nesting_level)
+            {
+                error("Exceeded maximum subgraph nesting level");
+            }
             current_subgraph_name = name;
-            if (peek().type == token::left_brace)
-                get();
-            else
-                error("Wanted left brace to start subgraph");
+            if (first_token.type != token::left_brace)
+            {
+                if (peek().type == token::left_brace)
+                    get();
+                else
+                    error("Wanted left brace to start subgraph");
+            }
             parse_stmt_list();
             if (peek().type == token::right_brace)
                 get();
             else
                 error("Wanted right brace to end subgraph");
             current_subgraph_name = old_sg;
+            sgnesting_level -= 1;
             return name;
         }
 

test/graphviz_test.cpp

@@ -440,6 +440,49 @@ void test_basic_csr_directed_graph_ext_props()
         edge_weight);
 }
 
+void test_subgraphs()
+{
+    // on the BGL side, the new parser doesn't support subgraphs
+    // however, the docs promise to support reading them on the input side as
+    // "syntactic sugar".
+    for (auto gv : {
+             Fixture { "digraph {}" },
+             Fixture { "digraph { 1 -> {} }", 1 },
+             Fixture { "digraph { 1 -> {2} }", 2 },
+             Fixture { "digraph { 1; { 2; 3; } }", 3 },
+             Fixture { "digraph { { 2; 3; } 1; }", 3 },
+             Fixture { "digraph { 1; subgraph { 2; 3; } }", 3 },
+             Fixture { "digraph { 1 -> subgraph { 2; 3; } }", 3 },
+             Fixture { "digraph { 1 -> subgraph hello { 2; 3; } }", 3 },
+             Fixture { "digraph { 1 -> subgraph clust_Hello { 2; 3; } }", 3 },
+             Fixture { "digraph { 1 -> subgraph \"hello\" { 2; 3; } }", 3 },
+             Fixture {
+                 "digraph { {2} -> subgraph \"hello\" {{{{ 2; 3; }}}} }", 2 },
+         })
+    {
+        TEST_GRAPH(Models::DiGraph, gv);
+    }
+}
+
+void test_subgraph_nesting_limit() // issue #364
+{
+    auto independent_nests = [=](unsigned level)
+    {
+        auto sg = std::string(level, '{') + " 2; 3; " + std::string(level, '}');
+        ComparisonDriver::test_graph< Models::DiGraph >(
+            { "digraph{1->" + sg + "}", 3 });
+        ComparisonDriver::test_graph< Models::DiGraph >(
+            { "digraph{1->" + sg + ";4->" + sg + "}", 4 });
+    };
+
+    constexpr unsigned limit = 255;
+    independent_nests(1);
+    independent_nests(limit / 2);
+    independent_nests(limit - 1);
+    independent_nests(limit); // edge-case
+    BOOST_TEST_THROWS(independent_nests(limit + 1), boost::bad_graphviz_syntax);
+}
+
 int main()
 {
     test_basic_directed_graph_1();
@@ -457,5 +500,7 @@ int main()
     test_comments_embedded_in_strings();
     test_basic_csr_directed_graph_ext_props();
     test_basic_csr_directed_graph();
+    test_subgraphs();
+    test_subgraph_nesting_limit();
     return boost::report_errors();
 }