update_engine: Reject XML with internal entity declarations.
This helps avoid resource exhaustion problems.
BUG=chromium:406546
TEST=New unit test + unit tests pass.
Change-Id: Ib54f378cf533c200631b274c0414075c2ea4ff67
Reviewed-on: https://chromium-review.googlesource.com/214291
Reviewed-by: Chris Masone <cmasone@chromium.org>
Reviewed-by: Alex Deymo <deymo@chromium.org>
Commit-Queue: David Zeuthen <zeuthen@chromium.org>
Tested-by: David Zeuthen <zeuthen@chromium.org>
diff --git a/error_code.h b/error_code.h
index 567534a..6a47784 100644
--- a/error_code.h
+++ b/error_code.h
@@ -57,6 +57,7 @@
kPostinstallFirmwareRONotUpdatable = 43,
kUnsupportedMajorPayloadVersion = 44,
kUnsupportedMinorPayloadVersion = 45,
+ kOmahaRequestXMLHasEntityDecl = 46,
// VERY IMPORTANT! When adding new error codes:
//
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 76eeee7..1bd5041 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -256,8 +256,14 @@
// Struct used for holding data obtained when parsing the XML.
struct OmahaParserData {
+ explicit OmahaParserData(XML_Parser _xml_parser) : xml_parser(_xml_parser) {}
+
+ // Pointer to the expat XML_Parser object.
+ XML_Parser xml_parser;
+
// This is the state of the parser as it's processing the XML.
bool failed = false;
+ bool entity_decl = false;
string current_path;
// These are the values extracted from the XML.
@@ -338,6 +344,29 @@
data->current_path.resize(data->current_path.size() - path_suffix.size());
}
+// Callback function invoked by expat.
+//
+// This is called for entity declarations. Since Omaha is guaranteed
+// to never return any XML with entities our course of action is to
+// just stop parsing. This avoids potential resource exhaustion
+// problems AKA the "billion laughs". CVE-2013-0340.
+void ParserHandlerEntityDecl(void *user_data,
+ const XML_Char *entity_name,
+ int is_parameter_entity,
+ const XML_Char *value,
+ int value_length,
+ const XML_Char *base,
+ const XML_Char *system_id,
+ const XML_Char *public_id,
+ const XML_Char *notation_name) {
+ OmahaParserData* data = reinterpret_cast<OmahaParserData*>(user_data);
+
+ LOG(ERROR) << "XML entities are not supported. Aborting parsing.";
+ data->failed = true;
+ data->entity_decl = true;
+ XML_StopParser(data->xml_parser, false);
+}
+
} // namespace
// Escapes text so it can be included as character data and attribute
@@ -757,18 +786,23 @@
}
XML_Parser parser = XML_ParserCreate(nullptr);
- OmahaParserData parser_data;
+ OmahaParserData parser_data(parser);
XML_SetUserData(parser, &parser_data);
XML_SetElementHandler(parser, ParserHandlerStart, ParserHandlerEnd);
+ XML_SetEntityDeclHandler(parser, ParserHandlerEntityDecl);
XML_Status res = XML_Parse(parser, &response_buffer_[0],
response_buffer_.size(), XML_TRUE);
XML_ParserFree(parser);
if (res != XML_STATUS_OK || parser_data.failed) {
LOG(ERROR) << "Omaha response not valid XML";
- completer.set_code(response_buffer_.empty() ?
- ErrorCode::kOmahaRequestEmptyResponseError :
- ErrorCode::kOmahaRequestXMLParseError);
+ ErrorCode error_code = ErrorCode::kOmahaRequestXMLParseError;
+ if (response_buffer_.empty()) {
+ error_code = ErrorCode::kOmahaRequestEmptyResponseError;
+ } else if (parser_data.entity_decl) {
+ error_code = ErrorCode::kOmahaRequestXMLHasEntityDecl;
+ }
+ completer.set_code(error_code);
return;
}
diff --git a/omaha_request_action_unittest.cc b/omaha_request_action_unittest.cc
index 8036f46..6d960e2 100644
--- a/omaha_request_action_unittest.cc
+++ b/omaha_request_action_unittest.cc
@@ -75,6 +75,18 @@
"status=\"ok\"/><updatecheck status=\"noupdate\"/></app></response>";
}
+string GetNoUpdateResponseWithEntity(const string& app_id) {
+ return string(
+ "<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
+ "<!DOCTYPE response ["
+ "<!ENTITY CrOS \"ChromeOS\">"
+ "]>"
+ "<response protocol=\"3.0\">"
+ "<daystart elapsed_seconds=\"100\"/>"
+ "<app appid=\"") + app_id + "\" status=\"ok\"><ping "
+ "status=\"ok\"/><updatecheck status=\"noupdate\"/></app></response>";
+}
+
string GetUpdateResponse2(const string& app_id,
const string& version,
const string& more_info_url,
@@ -328,6 +340,26 @@
*out_post_data = fetcher->post_data();
}
+TEST(OmahaRequestActionTest, RejectEntities) {
+ OmahaResponse response;
+ ASSERT_FALSE(
+ TestUpdateCheck(NULL, // prefs
+ NULL, // payload_state
+ NULL, // p2p_manager
+ NULL, // connection_manager
+ &kDefaultTestParams,
+ GetNoUpdateResponseWithEntity(OmahaRequestParams::kAppId),
+ -1,
+ false, // ping_only
+ ErrorCode::kOmahaRequestXMLHasEntityDecl,
+ metrics::CheckResult::kParsingError,
+ metrics::CheckReaction::kUnset,
+ metrics::DownloadErrorCode::kUnset,
+ &response,
+ NULL));
+ EXPECT_FALSE(response.update_exists);
+}
+
TEST(OmahaRequestActionTest, NoUpdateTest) {
OmahaResponse response;
ASSERT_TRUE(
diff --git a/payload_state.cc b/payload_state.cc
index a981546..1dd2ee6 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -328,6 +328,7 @@
case ErrorCode::kOmahaUpdateDeferredForBackoff:
case ErrorCode::kPostinstallPowerwashError:
case ErrorCode::kUpdateCanceledByChannelChange:
+ case ErrorCode::kOmahaRequestXMLHasEntityDecl:
LOG(INFO) << "Not incrementing URL index or failure count for this error";
break;
diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index 17f7aea..6646e56 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -91,7 +91,7 @@
class PayloadStateTest : public ::testing::Test { };
TEST(PayloadStateTest, DidYouAddANewErrorCode) {
- if (static_cast<int>(ErrorCode::kUmaReportedMax) != 46) {
+ if (static_cast<int>(ErrorCode::kUmaReportedMax) != 47) {
LOG(ERROR) << "The following failure is intentional. If you added a new "
<< "ErrorCode enum value, make sure to add it to the "
<< "PayloadState::UpdateFailed method and then update this test "
diff --git a/update_manager/chromeos_policy.cc b/update_manager/chromeos_policy.cc
index af1d8ac..0dc0ad6 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -115,6 +115,7 @@
case ErrorCode::kOmahaUpdateDeferredForBackoff:
case ErrorCode::kPostinstallPowerwashError:
case ErrorCode::kUpdateCanceledByChannelChange:
+ case ErrorCode::kOmahaRequestXMLHasEntityDecl:
LOG(INFO) << "Not changing URL index or failure count due to error "
<< chromeos_update_engine::utils::CodeToString(err_code)
<< " (" << static_cast<int>(err_code) << ")";
diff --git a/utils.cc b/utils.cc
index 2dcc8ca..ed57c14 100644
--- a/utils.cc
+++ b/utils.cc
@@ -1014,6 +1014,7 @@
case ErrorCode::kOmahaUpdateDeferredForBackoff:
case ErrorCode::kPostinstallPowerwashError:
case ErrorCode::kUpdateCanceledByChannelChange:
+ case ErrorCode::kOmahaRequestXMLHasEntityDecl:
return metrics::AttemptResult::kInternalError;
// Special flags. These can't happen (we mask them out above) but
@@ -1111,6 +1112,7 @@
case ErrorCode::kPostinstallFirmwareRONotUpdatable:
case ErrorCode::kUnsupportedMajorPayloadVersion:
case ErrorCode::kUnsupportedMinorPayloadVersion:
+ case ErrorCode::kOmahaRequestXMLHasEntityDecl:
break;
// Special flags. These can't happen (we mask them out above) but
@@ -1328,6 +1330,8 @@
return "ErrorCode::kUnsupportedMajorPayloadVersion";
case ErrorCode::kUnsupportedMinorPayloadVersion:
return "ErrorCode::kUnsupportedMinorPayloadVersion";
+ case ErrorCode::kOmahaRequestXMLHasEntityDecl:
+ return "ErrorCode::kOmahaRequestXMLHasEntityDecl";
// Don't add a default case to let the compiler warn about newly added
// error codes which should be added here.
}