src: avoid dereference without existence check

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. An earlier version of this patch was created by Bradley Farias <[email protected]>. PR-URL: #14591 Refs: #14369 (comment) Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
nodejs · Aug 6, 2017 · e96ca62 · e96ca62
1 parent ff65a2e
commit e96ca62
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 2 deletions.
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");