-
Notifications
You must be signed in to change notification settings - Fork 8.1k
ext/soap: Fix SOAP classmap validation for integer keys #22884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
6daf60a
b49aaf4
a2232d4
b92b376
2c3a3d8
e44f283
d7dbe19
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -930,6 +930,18 @@ static HashTable* soap_create_typemap(sdlPtr sdl, HashTable *ht) /* {{{ */ | |
| } | ||
| /* }}} */ | ||
|
|
||
| static bool soap_class_map_has_only_string_keys(const HashTable *class_map) | ||
| { | ||
| zend_string *key; | ||
| ZEND_HASH_FOREACH_STR_KEY(class_map, key) { | ||
| if (UNEXPECTED(key == NULL)) { | ||
| return false; | ||
| } | ||
| } ZEND_HASH_FOREACH_END(); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| /* {{{ SoapServer constructor */ | ||
| PHP_METHOD(SoapServer, __construct) | ||
| { | ||
|
|
@@ -1007,8 +1019,7 @@ PHP_METHOD(SoapServer, __construct) | |
| zend_argument_type_error(2, "\"classmap\" option must be of type array, %s given", zend_zval_type_name(class_map_zv)); | ||
| goto cleanup; | ||
| } | ||
| // TODO: this still accepts mixed keys arrays and not all numerically indexed arrays are packed | ||
| if (UNEXPECTED(HT_IS_PACKED(Z_ARRVAL_P(class_map_zv)))) { | ||
| if (UNEXPECTED(!soap_class_map_has_only_string_keys(Z_ARRVAL_P(class_map_zv)))) { | ||
| zend_argument_value_error(2, "\"classmap\" option must be an associative array"); | ||
| goto cleanup; | ||
| } | ||
|
|
@@ -2233,8 +2244,12 @@ PHP_METHOD(SoapClient, __construct) | |
| } | ||
| if ((tmp = zend_hash_str_find(ht, "classmap", sizeof("classmap")-1)) != NULL && | ||
| Z_TYPE_P(tmp) == IS_ARRAY) { | ||
| if (UNEXPECTED(HT_IS_PACKED(Z_ARRVAL_P(tmp)))) { | ||
| php_error_docref(NULL, E_ERROR, "'classmap' option must be an associative array"); | ||
| if (UNEXPECTED(!soap_class_map_has_only_string_keys(Z_ARRVAL_P(tmp)))) { | ||
| zend_argument_value_error(2, "\"classmap\" option must be an associative array"); | ||
| if (context) { | ||
| zend_list_delete(context->res); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we throw error here, the code won't goes to ZVAL_RES(). So we need to manually delete this. context = php_stream_context_from_zval(tmp, 1);
Z_ADDREF_P(tmp);
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes but in this case we can avoid this gymnastic by checking this before instantiating the stream context. |
||
| } | ||
| goto finish; | ||
| } | ||
| ZVAL_COPY(Z_CLIENT_CLASSMAP_P(this_ptr), tmp); | ||
| } | ||
|
|
@@ -2313,6 +2328,8 @@ PHP_METHOD(SoapClient, __construct) | |
| if (typemap_ht) { | ||
| soap_client_object_fetch(Z_OBJ_P(this_ptr))->typemap = soap_create_typemap(sdl, typemap_ht); | ||
| } | ||
|
|
||
| finish: | ||
| SOAP_CLIENT_END_CODE(); | ||
| } | ||
| /* }}} */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| --TEST-- | ||
| SoapClient and SoapServer classmap options must only contain string keys | ||
| --EXTENSIONS-- | ||
| soap | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| $emptyHash = ['type' => 'stdClass']; | ||
| unset($emptyHash['type']); | ||
|
|
||
| $cases = [ | ||
| 'empty' => [], | ||
| 'empty hash' => $emptyHash, | ||
| 'packed' => ['stdClass'], | ||
| 'sparse numeric' => [100 => 'stdClass'], | ||
| 'numeric string' => ['1' => 'stdClass'], | ||
| 'mixed' => ['type' => 'stdClass', 1 => 'stdClass'], | ||
| 'associative' => ['type' => 'stdClass'], | ||
| ]; | ||
|
|
||
| foreach ($cases as $name => $classmap) { | ||
| echo "-- $name --\n"; | ||
|
|
||
| try { | ||
| new SoapClient(null, [ | ||
| 'location' => 'http://example.com/', | ||
| 'uri' => 'urn:test', | ||
| 'classmap' => $classmap, | ||
| ]); | ||
| echo "SoapClient: OK\n"; | ||
| } catch (Throwable $e) { | ||
| echo $e->getMessage(), "\n"; | ||
|
LamentXU123 marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| try { | ||
| new SoapServer(null, [ | ||
| 'uri' => 'urn:test', | ||
| 'classmap' => $classmap, | ||
| ]); | ||
| echo "SoapServer: OK\n"; | ||
| } catch (Throwable $e) { | ||
| echo $e->getMessage(), "\n"; | ||
|
LamentXU123 marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
|
|
||
| ?> | ||
| --EXPECT-- | ||
| -- empty -- | ||
| SoapClient: OK | ||
| SoapServer: OK | ||
| -- empty hash -- | ||
| SoapClient: OK | ||
| SoapServer: OK | ||
| -- packed -- | ||
| SoapClient::__construct(): Argument #2 ($options) "classmap" option must be an associative array | ||
| SoapServer::__construct(): Argument #2 ($options) "classmap" option must be an associative array | ||
| -- sparse numeric -- | ||
| SoapClient::__construct(): Argument #2 ($options) "classmap" option must be an associative array | ||
| SoapServer::__construct(): Argument #2 ($options) "classmap" option must be an associative array | ||
| -- numeric string -- | ||
| SoapClient::__construct(): Argument #2 ($options) "classmap" option must be an associative array | ||
| SoapServer::__construct(): Argument #2 ($options) "classmap" option must be an associative array | ||
| -- mixed -- | ||
| SoapClient::__construct(): Argument #2 ($options) "classmap" option must be an associative array | ||
| SoapServer::__construct(): Argument #2 ($options) "classmap" option must be an associative array | ||
| -- associative -- | ||
| SoapClient: OK | ||
| SoapServer: OK | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| --TEST-- | ||
| SoapClient and SoapServer report an invalid classmap option as ValueError | ||
| --EXTENSIONS-- | ||
| soap | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| $classmap = ['type' => 'stdClass', 1 => 'stdClass']; | ||
|
|
||
| try { | ||
| new SoapClient(null, [ | ||
| 'location' => 'http://example.com/', | ||
| 'uri' => 'urn:test', | ||
| 'classmap' => $classmap, | ||
| ]); | ||
| } catch (Throwable $e) { | ||
| echo 'SoapClient: ', $e::class, ': ', $e->getMessage(), PHP_EOL; | ||
| } | ||
|
|
||
| try { | ||
| new SoapServer(null, [ | ||
| 'uri' => 'urn:test', | ||
| 'classmap' => $classmap, | ||
| ]); | ||
| } catch (Throwable $e) { | ||
| echo 'SoapServer: ', $e::class, ': ', $e->getMessage(), PHP_EOL; | ||
| } | ||
|
|
||
| ?> | ||
| --EXPECT-- | ||
| SoapClient: ValueError: SoapClient::__construct(): Argument #2 ($options) "classmap" option must be an associative array | ||
| SoapServer: ValueError: SoapServer::__construct(): Argument #2 ($options) "classmap" option must be an associative array |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| --TEST-- | ||
| SoapClient reports an invalid classmap option as ValueError when exceptions are disabled | ||
| --EXTENSIONS-- | ||
| soap | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| try { | ||
| new SoapClient(null, [ | ||
| 'location' => 'http://example.com/', | ||
| 'uri' => 'urn:test', | ||
| 'exceptions' => false, | ||
| 'classmap' => ['type' => 'stdClass', 1 => 'stdClass'], | ||
| ]); | ||
| } catch (Throwable $e) { | ||
| echo $e::class, ': ', $e->getMessage(), PHP_EOL; | ||
| } | ||
|
|
||
| ?> | ||
| --EXPECT-- | ||
| ValueError: SoapClient::__construct(): Argument #2 ($options) "classmap" option must be an associative array |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| --TEST-- | ||
| SoapClient must not read packed private classmap as string-keyed map | ||
| --EXTENSIONS-- | ||
| soap | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| class LocalSoapClient extends SoapClient { | ||
| public function __doRequest($request, $location, $action, $version, $one_way = false, ?string $uriParserClass = null): string { | ||
| echo "__doRequest called\n"; | ||
|
|
||
| return <<<'XML' | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <SOAP-ENV:Envelope xmlns:SOAP-ENV="http://schemas.xmlsoap.org/soap/envelope/"> | ||
| <SOAP-ENV:Body> | ||
| <SOAP-ENV:Fault> | ||
| <faultcode>SOAP-ENV:Server</faultcode> | ||
| <faultstring>expected fault</faultstring> | ||
| </SOAP-ENV:Fault> | ||
| </SOAP-ENV:Body> | ||
| </SOAP-ENV:Envelope> | ||
| XML; | ||
| } | ||
| } | ||
|
|
||
| class Foo {} | ||
|
|
||
| $client = new LocalSoapClient(null, [ | ||
| 'location' => 'http://example.org/', | ||
| 'uri' => 'http://example.org/', | ||
| ]); | ||
|
|
||
| $property = new ReflectionProperty(SoapClient::class, '_classmap'); | ||
| $property->setValue($client, ['Foo']); | ||
|
|
||
| try { | ||
| $client->__soapCall('foo', [new Foo()]); | ||
| } catch (SoapFault) { | ||
| echo "SOAP Fault thrown\n"; | ||
| } | ||
|
|
||
| ?> | ||
| --EXPECT-- | ||
| __doRequest called | ||
| SOAP Fault thrown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be preferable to throw if this is not an array