Test the CONF parser more extensively
Change-Id: I8792f118d281bc7a407dfbabe1c8b8e63f9eed9f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60085
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/conf/conf.c b/crypto/conf/conf.c
index 622c13e..0c05df4 100644
--- a/crypto/conf/conf.c
+++ b/crypto/conf/conf.c
@@ -68,15 +68,8 @@
#include "conf_def.h"
#include "internal.h"
#include "../internal.h"
-#include "../lhash/internal.h"
-DEFINE_LHASH_OF(CONF_VALUE)
-
-struct conf_st {
- LHASH_OF(CONF_VALUE) *data;
-};
-
static const char kDefaultSectionName[] = "default";
// The maximum length we can grow a value to after variable expansion. 64k
diff --git a/crypto/conf/conf_test.cc b/crypto/conf/conf_test.cc
index fe03c5f..f913d1e 100644
--- a/crypto/conf/conf_test.cc
+++ b/crypto/conf/conf_test.cc
@@ -12,8 +12,10 @@
* OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
* CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */
+#include <algorithm>
#include <string>
#include <vector>
+#include <map>
#include <openssl/bio.h>
#include <openssl/conf.h>
@@ -23,29 +25,393 @@
#include "internal.h"
-TEST(ConfTest, Parse) {
- // Check that basic parsing works. (We strongly recommend that people don't
- // use the [N]CONF functions.)
+// A |CONF| is an unordered list of sections, where each section contains an
+// ordered list of (name, value) pairs.
+using ConfModel =
+ std::map<std::string, std::vector<std::pair<std::string, std::string>>>;
- static const char kConf[] = R"(
-# Comment
+static void ExpectConfEquals(const CONF *conf, const ConfModel &model) {
+ // There is always a default section, even if empty. This is an easy mistake
+ // to make in test data, so test for it.
+ EXPECT_NE(model.find("default"), model.end())
+ << "Model does not have a default section";
+
+ size_t total_values = 0;
+ for (const auto &pair : model) {
+ const std::string §ion = pair.first;
+ SCOPED_TRACE(section);
+
+ const STACK_OF(CONF_VALUE) *values =
+ NCONF_get_section(conf, section.c_str());
+ ASSERT_TRUE(values);
+ total_values += pair.second.size();
+
+ EXPECT_EQ(sk_CONF_VALUE_num(values), pair.second.size());
+
+ // If the lengths do not match, still compare up to the smaller of the two,
+ // to aid debugging.
+ size_t min_len = std::min(sk_CONF_VALUE_num(values), pair.second.size());
+ for (size_t i = 0; i < min_len; i++) {
+ SCOPED_TRACE(i);
+ const std::string &name = pair.second[i].first;
+ const std::string &value = pair.second[i].second;
+
+ const CONF_VALUE *v = sk_CONF_VALUE_value(values, i);
+ EXPECT_EQ(v->section, section);
+ EXPECT_EQ(v->name, name);
+ EXPECT_EQ(v->value, value);
+
+ const char *str = NCONF_get_string(conf, section.c_str(), name.c_str());
+ ASSERT_NE(str, nullptr);
+ EXPECT_EQ(str, value);
+
+ if (section == "default") {
+ // nullptr is interpreted as the default section.
+ str = NCONF_get_string(conf, nullptr, name.c_str());
+ ASSERT_NE(str, nullptr);
+ EXPECT_EQ(str, value);
+ }
+ }
+ }
+
+ // Unrecognized sections must return nullptr.
+ EXPECT_EQ(NCONF_get_section(conf, "must_not_appear_in_tests"), nullptr);
+ EXPECT_EQ(NCONF_get_string(conf, "must_not_appear_in_tests",
+ "must_not_appear_in_tests"),
+ nullptr);
+ if (!model.empty()) {
+ // Valid section, invalid name.
+ EXPECT_EQ(NCONF_get_string(conf, model.begin()->first.c_str(),
+ "must_not_appear_in_tests"),
+ nullptr);
+ if (!model.begin()->second.empty()) {
+ // Invalid section, valid name.
+ EXPECT_EQ(NCONF_get_string(conf, "must_not_appear_in_tests",
+ model.begin()->second.front().first.c_str()),
+ nullptr);
+ }
+ }
+
+ // There should not be any other values in |conf|. |conf| currently stores
+ // both sections and values in the same map.
+ EXPECT_EQ(lh_CONF_VALUE_num_items(conf->data), total_values + model.size());
+}
+
+TEST(ConfTest, Parse) {
+ const struct {
+ std::string in;
+ ConfModel model;
+ } kTests[] = {
+ // Test basic parsing.
+ {
+ R"(# Comment
key=value
[section_name]
key=value2
-)";
+)",
+ {
+ {"default", {{"key", "value"}}},
+ {"section_name", {{"key", "value2"}}},
+ },
+ },
- bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(kConf, sizeof(kConf) - 1));
- ASSERT_TRUE(bio);
- bssl::UniquePtr<CONF> conf(NCONF_new(nullptr));
- ASSERT_TRUE(conf);
- ASSERT_TRUE(NCONF_load_bio(conf.get(), bio.get(), nullptr));
- EXPECT_TRUE(NCONF_get_section(conf.get(), "section_name"));
- EXPECT_FALSE(NCONF_get_section(conf.get(), "other_section"));
- EXPECT_STREQ(NCONF_get_string(conf.get(), nullptr, "key"), "value");
- EXPECT_STREQ(NCONF_get_string(conf.get(), "section_name", "key"), "value2");
- EXPECT_STREQ(NCONF_get_string(conf.get(), "other_section", "key"), nullptr);
+ // If a section is listed multiple times, keys add to the existing one.
+ {
+ R"(key1 = value1
+
+[section1]
+key2 = value2
+
+[section2]
+key3 = value3
+
+[default]
+key4 = value4
+
+[section1]
+key5 = value5
+)",
+ {
+ {"default", {{"key1", "value1"}, {"key4", "value4"}}},
+ {"section1", {{"key2", "value2"}, {"key5", "value5"}}},
+ {"section2", {{"key3", "value3"}}},
+ },
+ },
+
+ // Although the CONF parser internally uses a buffer size of 512 bytes to
+ // read one line, it detects truncation and is able to parse long lines.
+ {
+ std::string(1000, 'a') + " = " + std::string(1000, 'b') + "\n",
+ {
+ {"default", {{std::string(1000, 'a'), std::string(1000, 'b')}}},
+ },
+ },
+
+ // Trailing backslashes are line continations.
+ {
+ "key=\\\nvalue\nkey2=foo\\\nbar=baz",
+ {
+ {"default", {{"key", "value"}, {"key2", "foobar=baz"}}},
+ },
+ },
+
+ // To be a line continuation, it must be at the end of the line.
+ {
+ "key=\\\nvalue\nkey2=foo\\ \nbar=baz",
+ {
+ {"default", {{"key", "value"}, {"key2", "foo"}, {"bar", "baz"}}},
+ },
+ },
+
+ // A line continuation without any following line is ignored.
+ {
+ "key=value\\",
+ {
+ {"default", {{"key", "value"}}},
+ },
+ },
+
+ // Values may have embedded whitespace, but leading and trailing
+ // whitespace is dropped.
+ {
+ "key = \t foo \t\t\tbar \t ",
+ {
+ {"default", {{"key", "foo \t\t\tbar"}}},
+ },
+ },
+
+ // Empty sections still end up in the file.
+ {
+ "[section1]\n[section2]\n[section3]\n",
+ {
+ {"default", {}},
+ {"section1", {}},
+ {"section2", {}},
+ {"section3", {}},
+ },
+ },
+
+ // Section names can contain spaces and punctuation.
+ {
+ "[This! Is. A? Section;]\nkey = value",
+ {
+ {"default", {}},
+ {"This! Is. A? Section;", {{"key", "value"}}},
+ },
+ },
+
+ // Trailing data after a section line is ignored.
+ {
+ "[section] key = value\nkey2 = value2\n",
+ {
+ {"default", {}},
+ {"section", {{"key2", "value2"}}},
+ },
+ },
+
+ // Comments may appear within a line. Escapes and quotes, however,
+ // suppress the comment character.
+ {
+ R"(
+key1 = # comment
+key2 = "# not a comment"
+key3 = '# not a comment'
+key4 = `# not a comment`
+key5 = \# not a comment
+)",
+ {
+ {"default",
+ {
+ {"key1", ""},
+ {"key2", "# not a comment"},
+ {"key3", "# not a comment"},
+ {"key4", "# not a comment"},
+ {"key5", "# not a comment"},
+ }},
+ },
+ },
+
+ // Quotes may appear in the middle of a string. Inside quotes, escape
+ // sequences like \n are not evaluated. \X always evaluates to X.
+ {
+ R"(
+key1 = mix "of" 'different' `quotes`
+key2 = "`'"
+key3 = "\r\n\b\t\""
+key4 = '\r\n\b\t\''
+key5 = `\r\n\b\t\``
+)",
+ {
+ {"default",
+ {
+ {"key1", "mix of different quotes"},
+ {"key2", "`'"},
+ {"key3", "rnbt\""},
+ {"key4", "rnbt'"},
+ {"key5", "rnbt`"},
+ }},
+ },
+ },
+
+ // Outside quotes, escape sequences like \n are evaluated. Unknown escapes
+ // turn into the character.
+ {
+ R"(
+key = \r\n\b\t\"\'\`\z
+)",
+ {
+ {"default",
+ {
+ {"key", "\r\n\b\t\"'`z"},
+ }},
+ },
+ },
+
+ // Escapes (but not quoting) work inside section names.
+ {
+ "[section\\ name]\nkey = value\n",
+ {
+ {"default", {}},
+ {"section name", {{"key", "value"}}},
+ },
+ },
+
+ // Escapes (but not quoting) are skipped over in key names, but they are
+ // left unevaluated. This is probably a bug.
+ {
+ "key\\ name = value\n",
+ {
+ {"default", {{"key\\ name", "value"}}},
+ },
+ },
+
+ // Keys can specify sections explicitly with ::.
+ {
+ R"(
+[section1]
+default::key1 = value1
+section1::key2 = value2
+section2::key3 = value3
+section1::key4 = value4
+section2::key5 = value5
+default::key6 = value6
+key7 = value7 # section1
+)",
+ {
+ {"default", {{"key1", "value1"}, {"key6", "value6"}}},
+ {"section1",
+ {{"key2", "value2"}, {"key4", "value4"}, {"key7", "value7"}}},
+ {"section2", {{"key3", "value3"}, {"key5", "value5"}}},
+ },
+ },
+
+ // Values can refer to other values with $.
+ {
+ R"(
+key1 = value1
+key2 = $key1 ${key1} $(key1)
+
+[section1]
+key3 = value3
+
+[section2]
+key4 = value4
+key5 = $key4 ${key4} $(key4)
+key6 = $default::key1 ${default::key1} $(default::key1)
+key7 = $section1::key3 ${section1::key3} $(section1::key3)
+key8 = $section2::key4 ${section2::key4} $(section2::key4)
+)",
+ {
+ {"default",
+ {
+ {"key1", "value1"},
+ {"key2", "value1 value1 value1"},
+ }},
+ {"section1",
+ {
+ {"key3", "value3"},
+ }},
+ {"section2",
+ {
+ {"key4", "value4"},
+ {"key5", "value4 value4 value4"},
+ {"key6", "value1 value1 value1"},
+ {"key7", "value3 value3 value3"},
+ {"key8", "value4 value4 value4"},
+ }},
+ },
+ },
+
+ // Punctuation is allowed in key names.
+ {
+ "key.1 = value\n",
+ {
+ {"default", {{"key.1", "value"}}},
+ },
+ },
+
+ // Punctuation is not allowed in $ references. The punctuation stops the
+ // parse.
+ {
+ R"(
+key = a
+key.1 = b
+ref = $key
+)",
+ {
+ {"default",
+ {
+ {"key", "a"},
+ {"key.1", "b"},
+ {"ref", "a"},
+ }},
+ },
+ },
+ };
+ for (const auto &t : kTests) {
+ SCOPED_TRACE(t.in);
+ bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(t.in.data(), t.in.size()));
+ ASSERT_TRUE(bio);
+ bssl::UniquePtr<CONF> conf(NCONF_new(nullptr));
+ ASSERT_TRUE(conf);
+ ASSERT_TRUE(NCONF_load_bio(conf.get(), bio.get(), nullptr));
+
+ ExpectConfEquals(conf.get(), t.model);
+ }
+
+ const char *kInvalidTests[] = {
+ // Missing equals sign.
+ "key",
+ // Unterminated section heading.
+ "[section",
+ // Section names can only contain alphanumeric characters, punctuation,
+ // and escapes. Quotes are not punctuation.
+ "[\"section\"]",
+ // Keys can only contain alphanumeric characters, punctuaion, and escapes.
+ "key name = value",
+ "\"key\" = value",
+ // Referencing a non-existent variable.
+ "key = $key2",
+ // key1 here is interpreted as a section name, not a key.
+ "key1 = value1\nkey2 = $key1::key",
+ // No self references.
+ "key = $key",
+ // Unterminated braced variable reference.
+ "key1 = value1\nkey2 = ${key1",
+ "key1 = value1\nkey2 = $(key1",
+ // Empty reference.
+ "key1 = $",
+ };
+ for (const auto &t : kInvalidTests) {
+ SCOPED_TRACE(t);
+ bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(t, strlen(t)));
+ ASSERT_TRUE(bio);
+ bssl::UniquePtr<CONF> conf(NCONF_new(nullptr));
+ ASSERT_TRUE(conf);
+ EXPECT_FALSE(NCONF_load_bio(conf.get(), bio.get(), nullptr));
+ }
}
TEST(ConfTest, ParseList) {
diff --git a/crypto/conf/internal.h b/crypto/conf/internal.h
index 6075548..04359ad 100644
--- a/crypto/conf/internal.h
+++ b/crypto/conf/internal.h
@@ -17,11 +17,19 @@
#include <openssl/base.h>
+#include "../lhash/internal.h"
+
#if defined(__cplusplus)
extern "C" {
#endif
+DEFINE_LHASH_OF(CONF_VALUE)
+
+struct conf_st {
+ LHASH_OF(CONF_VALUE) *data;
+};
+
// CONF_VALUE_new returns a freshly allocated and zeroed |CONF_VALUE|.
CONF_VALUE *CONF_VALUE_new(void);
diff --git a/include/openssl/conf.h b/include/openssl/conf.h
index 908c16e..7529190 100644
--- a/include/openssl/conf.h
+++ b/include/openssl/conf.h
@@ -77,7 +77,10 @@
// [section_name]
// key2=value2
//
-// Config files are represented by a |CONF|.
+// Config files are represented by a |CONF|. Use of this module is strongly
+// discouraged. It is a remnant of the OpenSSL command-line tool. Parsing an
+// untrusted input as a config file risks string injection and denial of service
+// vulnerabilities.
struct conf_value_st {
char *section;
