src: avoid dereference without existence check

bmeck · TimothyGu · commit fc18a6992c70 · 2017-08-05T16:47:06.000+08:00
Currently the URL API is only used from the JS binding, which always initializes `base` regardless of `has_base`. Therefore, there is no actual security risk right now, but would be had we made other C++ parts of Node.js use this API. Refs: #14369 (comment)
diff --git a/src/node_url.cc b/src/node_url.cc
@@ -1283,7 +1283,7 @@ void URL::Parse(const char* input,
         }
         break;
       case kNoScheme:
-        cannot_be_base = base->flags & URL_FLAGS_CANNOT_BE_BASE;
+        cannot_be_base = has_base && (base->flags & URL_FLAGS_CANNOT_BE_BASE);
         if (!has_base || (cannot_be_base && ch != '#')) {
           url->flags |= URL_FLAGS_FAILED;
           return;
diff --git a/test/cctest/test_url.cc b/test/cctest/test_url.cc
@@ -4,6 +4,7 @@
 #include "gtest/gtest.h"
 
 using node::url::URL;
+using node::url::URL_FLAGS_FAILED;
 
 class URLTest : public ::testing::Test {
  protected:
@@ -20,6 +21,7 @@ class URLTest : public ::testing::Test {
 TEST_F(URLTest, Simple) {
   URL simple("https://example.org:81/a/b/c?query#fragment");
 
+  EXPECT_FALSE(simple.flags() & URL_FLAGS_FAILED);
   EXPECT_EQ(simple.protocol(), "https:");
   EXPECT_EQ(simple.host(), "example.org");
   EXPECT_EQ(simple.port(), 81);
@@ -32,6 +34,7 @@ TEST_F(URLTest, Simple2) {
   const char* input = "https://example.org:81/a/b/c?query#fragment";
   URL simple(input, strlen(input));
 
+  EXPECT_FALSE(simple.flags() & URL_FLAGS_FAILED);
   EXPECT_EQ(simple.protocol(), "https:");
   EXPECT_EQ(simple.host(), "example.org");
   EXPECT_EQ(simple.port(), 81);
@@ -40,10 +43,17 @@ TEST_F(URLTest, Simple2) {
   EXPECT_EQ(simple.fragment(), "fragment");
 }
 
+TEST_F(URLTest, NoBase1) {
+  URL error("123noscheme");
+  EXPECT_TRUE(error.flags() & URL_FLAGS_FAILED);
+}
+
 TEST_F(URLTest, Base1) {
   URL base("http://example.org/foo/bar");
-  URL simple("../baz", &base);
+  ASSERT_FALSE(base.flags() & URL_FLAGS_FAILED);
 
+  URL simple("../baz", &base);
+  EXPECT_FALSE(simple.flags() & URL_FLAGS_FAILED);
   EXPECT_EQ(simple.protocol(), "http:");
   EXPECT_EQ(simple.host(), "example.org");
   EXPECT_EQ(simple.path(), "/baz");
@@ -52,6 +62,7 @@ TEST_F(URLTest, Base1) {
 TEST_F(URLTest, Base2) {
   URL simple("../baz", "http://example.org/foo/bar");
 
+  EXPECT_FALSE(simple.flags() & URL_FLAGS_FAILED);
   EXPECT_EQ(simple.protocol(), "http:");
   EXPECT_EQ(simple.host(), "example.org");
   EXPECT_EQ(simple.path(), "/baz");
@@ -63,6 +74,7 @@ TEST_F(URLTest, Base3) {
 
   URL simple(input, strlen(input), base, strlen(base));
 
+  EXPECT_FALSE(simple.flags() & URL_FLAGS_FAILED);
   EXPECT_EQ(simple.protocol(), "http:");
   EXPECT_EQ(simple.host(), "example.org");
   EXPECT_EQ(simple.path(), "/baz");

Original file line number	Diff line number	Diff line change
`@@ -1283,7 +1283,7 @@ void URL::Parse(const char* input,`
`1283`	`1283`	`}`
`1284`	`1284`	`break;`
`1285`	`1285`	`case kNoScheme:`
`1286`		`- cannot_be_base = base->flags & URL_FLAGS_CANNOT_BE_BASE;`
	`1286`	`+ cannot_be_base = has_base && (base->flags & URL_FLAGS_CANNOT_BE_BASE);`
`1287`	`1287`	`if (!has_base \|\| (cannot_be_base && ch != '#')) {`
`1288`	`1288`	`url->flags \|= URL_FLAGS_FAILED;`
`1289`	`1289`	`return;`