Merge pull request #1634 from sjinks/beanstalk-job

Safe unserialization of Phalcon\Queue\Beanstalk\Job

ext/queue/beanstalk/job.c

@@ -64,18 +64,19 @@ PHALCON_INIT_CLASS(Phalcon_Queue_Beanstalk_Job){
  *
  * @param Phalcon\Queue\Beanstalk $queue
  * @param string $id
- * @param string $body
+ * @param mixed $body
  */
 PHP_METHOD(Phalcon_Queue_Beanstalk_Job, __construct){
 
-	zval *queue, *id, *body;
+	zval **queue, **id, **body;
 
-	phalcon_fetch_params(0, 3, 0, &queue, &id, &body);
-
-	phalcon_update_property_this(this_ptr, SL("_queue"), queue TSRMLS_CC);
-	phalcon_update_property_this(this_ptr, SL("_id"), id TSRMLS_CC);
-	phalcon_update_property_this(this_ptr, SL("_body"), body TSRMLS_CC);
-
+	phalcon_fetch_params_ex(3, 0, &queue, &id, &body);
+	PHALCON_VERIFY_CLASS_EX(*queue, phalcon_queue_beanstalk_ce, phalcon_exception_ce, 0);
+	PHALCON_ENSURE_IS_STRING(id);
+
+	phalcon_update_property_this(this_ptr, SL("_queue"), *queue TSRMLS_CC);
+	phalcon_update_property_this(this_ptr, SL("_id"),    *id    TSRMLS_CC);
+	phalcon_update_property_this(this_ptr, SL("_body"),  *body  TSRMLS_CC);
 }
 
 /**
@@ -92,7 +93,7 @@ PHP_METHOD(Phalcon_Queue_Beanstalk_Job, getId){
 /**
  * Returns the job body
  *
- * @return string
+ * @return mixed
  */
 PHP_METHOD(Phalcon_Queue_Beanstalk_Job, getBody){
 
@@ -103,7 +104,7 @@ PHP_METHOD(Phalcon_Queue_Beanstalk_Job, getBody){
 /**
  * Removes a job from the server entirely
  *
- * @param integer $id
+ * @param string $id
  * @return boolean
  */
 PHP_METHOD(Phalcon_Queue_Beanstalk_Job, delete){
@@ -112,14 +113,11 @@ PHP_METHOD(Phalcon_Queue_Beanstalk_Job, delete){
 
 	PHALCON_MM_GROW();
 
-	PHALCON_OBS_VAR(id);
-	phalcon_read_property_this(&id, this_ptr, SL("_id"), PH_NOISY_CC);
+	id    = phalcon_fetch_nproperty_this(this_ptr, SL("_id"), PH_NOISY_CC);
+	queue = phalcon_fetch_nproperty_this(this_ptr, SL("_queue"), PH_NOISY_CC);
 
-	PHALCON_INIT_VAR(command);
+	PHALCON_ALLOC_GHOST_ZVAL(command);
 	PHALCON_CONCAT_SV(command, "delete ", id);
-
-	PHALCON_OBS_VAR(queue);
-	phalcon_read_property_this(&queue, this_ptr, SL("_queue"), PH_NOISY_CC);
 	phalcon_call_method_p1_noret(queue, "write", command);
 
 	PHALCON_INIT_VAR(response);
@@ -134,3 +132,14 @@ PHP_METHOD(Phalcon_Queue_Beanstalk_Job, delete){
 	RETURN_MM_FALSE;
 }
 
+PHP_METHOD(Phalcon_Queue_Beanstalk_Job, __wakeup) {
+
+	zval *id    = phalcon_fetch_nproperty_this(this_ptr, SL("_id"), PH_NOISY_CC);
+	zval *queue = phalcon_fetch_nproperty_this(this_ptr, SL("_queue"), PH_NOISY_CC);
+
+	PHALCON_VERIFY_CLASS_EX(queue, phalcon_queue_beanstalk_ce, phalcon_exception_ce, 0);
+
+	if (Z_TYPE_P(id) != IS_STRING) {
+		zend_throw_exception_ex(phalcon_exception_ce, 0 TSRMLS_CC, "Unexpected inconsistency in %s - possible break-in attempt!", "Phalcon\\Queue\\Beanstalk\\Job::__wakeup()");
+	}
+}

ext/queue/beanstalk/job.h

@@ -25,6 +25,7 @@ PHP_METHOD(Phalcon_Queue_Beanstalk_Job, __construct);
 PHP_METHOD(Phalcon_Queue_Beanstalk_Job, getId);
 PHP_METHOD(Phalcon_Queue_Beanstalk_Job, getBody);
 PHP_METHOD(Phalcon_Queue_Beanstalk_Job, delete);
+PHP_METHOD(Phalcon_Queue_Beanstalk_Job, __wakeup);
 
 ZEND_BEGIN_ARG_INFO_EX(arginfo_phalcon_queue_beanstalk_job___construct, 0, 0, 3)
 	ZEND_ARG_INFO(0, queue)
@@ -37,6 +38,6 @@ PHALCON_INIT_FUNCS(phalcon_queue_beanstalk_job_method_entry){
 	PHP_ME(Phalcon_Queue_Beanstalk_Job, getId, NULL, ZEND_ACC_PUBLIC) 
 	PHP_ME(Phalcon_Queue_Beanstalk_Job, getBody, NULL, ZEND_ACC_PUBLIC) 
 	PHP_ME(Phalcon_Queue_Beanstalk_Job, delete, NULL, ZEND_ACC_PUBLIC) 
+	PHP_ME(Phalcon_Queue_Beanstalk_Job, __wakeup, NULL, ZEND_ACC_PUBLIC)
 	PHP_FE_END
 };
-

ext/tests/issue-1634.phpt

@@ -0,0 +1,57 @@
+--TEST--
+Safe unserialization of Phalcon\Queue\Beanstalk\Job - https://github.com/phalcon/cphalcon/pull/1634
+--SKIPIF--
+<?php include('skipif.inc'); ?>
+--FILE--
+<?php
+
+class Test extends \Phalcon\Queue\Beanstalk\Job
+{
+	public function setId($id)
+	{
+		$this->_id = $id;
+	}
+
+	public function setQueue($queue)
+	{
+		$this->_queue = $queue;
+	}
+}
+
+try {
+	$j = new \Test(null, 'id', 'body');
+	assert(false);
+}
+catch (\Phalcon\Exception $e) {
+	echo $e->getMessage(), PHP_EOL;
+}
+
+/*
+$j = new \Test(new \Phalcon\Queue\Beanstalk(), 'id', 'body');
+$j->setQueue(new stdClass);
+echo base64_encode(serialize($j)), PHP_EOL;
+*/
+
+// O:4:"Test":3:{s:9:"*_queue";O:23:"Phalcon\Queue\Beanstalk":2:{s:14:"*_connection";N;s:14:"*_parameters";a:2:{s:4:"host";s:9:"127.0.0.1";s:4:"port";i:11300;}}s:6:"*_id";a:0:{}s:8:"*_body";s:4:"body";}
+$s = base64_decode('Tzo0OiJUZXN0IjozOntzOjk6IgAqAF9xdWV1ZSI7TzoyMzoiUGhhbGNvblxRdWV1ZVxCZWFuc3RhbGsiOjI6e3M6MTQ6IgAqAF9jb25uZWN0aW9uIjtOO3M6MTQ6IgAqAF9wYXJhbWV0ZXJzIjthOjI6e3M6NDoiaG9zdCI7czo5OiIxMjcuMC4wLjEiO3M6NDoicG9ydCI7aToxMTMwMDt9fXM6NjoiACoAX2lkIjthOjA6e31zOjg6IgAqAF9ib2R5IjtzOjQ6ImJvZHkiO30=');
+try {
+	unserialize($s);
+}
+catch (\Phalcon\Exception $e) {
+	echo $e->getMessage(), PHP_EOL;
+}
+
+// O:4:"Test":3:{s:9:"*_queue";O:8:"stdClass":0:{}s:6:"*_id";s:2:"id";s:8:"*_body";s:4:"body";}
+$s = base64_decode('Tzo0OiJUZXN0IjozOntzOjk6IgAqAF9xdWV1ZSI7Tzo4OiJzdGRDbGFzcyI6MDp7fXM6NjoiACoAX2lkIjtzOjI6ImlkIjtzOjg6IgAqAF9ib2R5IjtzOjQ6ImJvZHkiO30=');
+try {
+	unserialize($s);
+}
+catch (\Phalcon\Exception $e) {
+	echo $e->getMessage(), PHP_EOL;
+}
+
+?>
+--EXPECT--
+Unexpected value type: expected object of type Phalcon\Queue\Beanstalk, null given
+Unexpected inconsistency in Phalcon\Queue\Beanstalk\Job::__wakeup() - possible break-in attempt!
+Unexpected value type: expected object of type Phalcon\Queue\Beanstalk, object of type stdClass given