From e96ca62480c6fc14952c81a3e24835b91d6c849e Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Sat, 5 Aug 2017 16:57:10 +0800 Subject: [PATCH] src: avoid dereference without existence check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 . PR-URL: https://github.com/nodejs/node/pull/14591 Refs: https://github.com/nodejs/node/pull/14369#discussion_r128767221 Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Tobias Nießen --- src/node_url.cc | 2 +- test/cctest/test_url.cc | 14 +++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/node_url.cc b/src/node_url.cc index 54dbc3a053b3a1..01e46eb7643e53 100644 --- 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 index 1b17ddf78c1c46..2cede1a8a3deb3 100644 --- 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");