From 211b7e728993ec299ca6d9feecae6704f91ca9f4 Mon Sep 17 00:00:00 2001
From: twistor <twistor@473738.no-reply.drupal.org>
Date: Tue, 16 Jun 2015 19:24:44 -0700
Subject: [PATCH] Issue #2495145 by twistor, cashwilliams, greggles, klausi:
Possible XSS in PuSHSubscriber.inc
---
feeds.info | 1 +
libraries/PuSHSubscriber.inc | 92 +++++++++++++++++++-----------------
tests/feeds_push.test | 47 ++++++++++++++++++
3 files changed, 97 insertions(+), 43 deletions(-)
create mode 100644 tests/feeds_push.test
diff --git a/feeds.info b/feeds.info
index 57eaabf8..11934e0d 100644
--- a/feeds.info
+++ b/feeds.info
@@ -51,6 +51,7 @@ files[] = tests/feeds_parser_syndication.test
files[] = tests/feeds_processor_node.test
files[] = tests/feeds_processor_term.test
files[] = tests/feeds_processor_user.test
+files[] = tests/feeds_push.test
files[] = tests/feeds_scheduler.test
files[] = tests/feeds_mapper_link.test
files[] = tests/feeds_mapper_summary.test
diff --git a/libraries/PuSHSubscriber.inc b/libraries/PuSHSubscriber.inc
index c5926def..38f88867 100644
--- a/libraries/PuSHSubscriber.inc
+++ b/libraries/PuSHSubscriber.inc
@@ -161,7 +161,7 @@ class PuSHSubscriber {
if (isset($_SERVER['HTTP_X_HUB_SIGNATURE']) && ($sub = $this->subscription())) {
$result = array();
parse_str($_SERVER['HTTP_X_HUB_SIGNATURE'], $result);
- if (isset($result['sha1']) && $result['sha1'] == hash_hmac('sha1', $raw, $sub->secret)) {
+ if (isset($result['sha1']) && $result['sha1'] === hash_hmac('sha1', $raw, $sub->secret)) {
return $raw;
}
else {
@@ -183,47 +183,44 @@ class PuSHSubscriber {
* method handles the challenge.
*/
public function verifyRequest() {
- if (isset($_GET['hub_challenge'])) {
- /**
- * If a subscription is present, compare the verify token. If the token
- * matches, set the status on the subscription record and confirm
- * positive.
- *
- * If we cannot find a matching subscription and the hub checks on
- * 'unsubscribe' confirm positive.
- *
- * In all other cases confirm negative.
- */
- if ($sub = $this->subscription()) {
- if ($_GET['hub_verify_token'] == $sub->post_fields['hub.verify_token']) {
- if ($_GET['hub_mode'] == 'subscribe' && $sub->status == 'subscribe') {
- $sub->status = 'subscribed';
- $sub->post_fields = array();
- $sub->save();
- $this->log('Verified "subscribe" request.');
- $verify = TRUE;
- }
- elseif ($_GET['hub_mode'] == 'unsubscribe' && $sub->status == 'unsubscribe') {
- $sub->status = 'unsubscribed';
- $sub->post_fields = array();
- $sub->save();
- $this->log('Verified "unsubscribe" request.');
- $verify = TRUE;
- }
- }
- }
- elseif ($_GET['hub_mode'] == 'unsubscribe') {
- $this->log('Verified "unsubscribe" request.');
- $verify = TRUE;
- }
- if ($verify) {
- header('HTTP/1.1 200 "Found"', NULL, 200);
- print $_GET['hub_challenge'];
- drupal_exit();
- }
+ if (!isset($_GET['hub_challenge'])) {
+ return $this->rejectRequest();
}
- header('HTTP/1.1 404 "Not Found"', NULL, 404);
- $this->log('Could not verify subscription.', 'error');
+
+ // Don't accept modes of 'subscribed' or 'unsubscribed'.
+ if ($_GET['hub_mode'] !== 'subscribe' && $_GET['hub_mode'] !== 'unsubscribe') {
+ return $this->rejectRequest();
+ }
+
+ // No available subscription.
+ if (!$sub = $this->subscription()) {
+ return $this->rejectRequest();
+ }
+
+ // Not what we asked for.
+ if ($_GET['hub_mode'] !== $sub->status) {
+ return $this->rejectRequest();
+ }
+
+ // Wrong URL.
+ if ($_GET['hub_topic'] !== $sub->topic) {
+ return $this->rejectRequest();
+ }
+
+ if ($sub->status === 'subscribe') {
+ $sub->status = 'subscribed';
+ $this->log('Verified "subscribe" request.');
+ }
+ else {
+ $sub->status = 'unsubscribed';
+ $this->log('Verified "unsubscribe" request.');
+ }
+
+ $sub->post_fields = array();
+ $sub->save();
+
+ header('HTTP/1.1 200 "Found"', NULL, 200);
+ print check_plain($_GET['hub_challenge']);
drupal_exit();
}
@@ -244,7 +241,7 @@ class PuSHSubscriber {
* @todo Make concurrency safe.
*/
protected function request($hub, $topic, $mode, $callback_url) {
- $secret = hash('sha1', uniqid(rand(), TRUE));
+ $secret = drupal_random_key(40);
$post_fields = array(
'hub.callback' => $callback_url,
'hub.mode' => $mode,
@@ -252,7 +249,6 @@ class PuSHSubscriber {
'hub.verify' => 'sync',
'hub.lease_seconds' => '', // Permanent subscription.
'hub.secret' => $secret,
- 'hub.verify_token' => md5(session_id() . rand()),
);
$sub = new $this->subscription_class($this->domain, $this->subscriber_id, $hub, $topic, $secret, $mode, $post_fields);
$sub->save();
@@ -310,6 +306,16 @@ class PuSHSubscriber {
protected function log($msg, $level = 'status') {
$this->env->log("{$this->domain}:{$this->subscriber_id}\t$msg", $level);
}
+
+ /**
+ * Rejects a request subscription request.
+ */
+ protected function rejectRequest() {
+ header('HTTP/1.1 404 "Not Found"', NULL, 404);
+ $this->log('Could not verify subscription.', 'error');
+ drupal_exit();
+ }
+
}
/**
diff --git a/tests/feeds_push.test b/tests/feeds_push.test
new file mode 100644
index 00000000..ffe3ec8f
--- /dev/null
+++ b/tests/feeds_push.test
@@ -0,0 +1,47 @@
+<?php
+
+/**
+ * @file
+ * Contains FeedsPushTest.
+ */
+
+/**
+ * Tests for PubSubHubbub support in feeds.
+ */
+class FeedsPushTest extends FeedsWebTestCase {
+
+ public static function getInfo() {
+ return array(
+ 'name' => 'PubSubHubbub',
+ 'description' => 'Tests for PubSubHubbub support.',
+ 'group' => 'Feeds',
+ );
+ }
+
+ /**
+ * Tests that PubSubHubbub challenge is escaped.
+ */
+ public function test() {
+ $this->createImporterConfiguration('Push', 'push');
+
+ $subscription = new PuSHSubscription('push', 0, 'http://example.com', 'http://example.com/feed', 'secret', 'unsubscribe', array());
+ $subscription->save();
+
+ $challenge = '<script>alert();</script>';
+
+ $options = array(
+ 'query' => array(
+ 'hub.mode' => 'unsubscribe',
+ 'hub.challenge' => $challenge,
+ 'hub.topic' => 'http://example.com/feed',
+ ),
+ );
+
+ $this->drupalGet("feeds/importer/push/0", $options);
+
+ $this->assertResponse(200);
+ $this->assertRaw(check_plain($challenge), 'Challenge was escaped.');
+ $this->assertNoRaw($challenge, 'Raw challenge not found.');
+ }
+
+}
--
GitLab