Skip to content

Commit

Permalink
Merge pull request from GHSA-w6jr-wj64-mc9x
Browse files Browse the repository at this point in the history
fix: Deserialization of Untrusted Data in old()
  • Loading branch information
MGatner authored Jan 3, 2022
2 parents bfa2262 + 025a63b commit ce95ed5
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 12 deletions.
1 change: 1 addition & 0 deletions SECURITY.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ This person will coordinate the fix and release process, involving the following
- Confirm the problem and determine the affected versions.
- Audit code to find any potential similar problems.
- Prepare fixes for all releases still under maintenance. These fixes will be released as fast as possible.
- Publish security advisories at https://github.com/codeigniter4/CodeIgniter4/security/advisories

## Comments on this Policy

Expand Down
5 changes: 0 additions & 5 deletions system/Common.php
Original file line number Diff line number Diff line change
Expand Up @@ -810,11 +810,6 @@ function old(string $key, $default = null, $escape = 'html')
return $default;
}

// If the result was serialized array or string, then unserialize it for use...
if (is_string($value) && (strpos($value, 'a:') === 0 || strpos($value, 's:') === 0)) {
$value = unserialize($value);
}

return $escape === false ? $value : esc($value, $escape);
}
}
Expand Down
37 changes: 35 additions & 2 deletions tests/system/CommonFunctionsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -301,15 +301,48 @@ public function testOldInput()
$_GET = ['foo' => 'bar'];
$_POST = [
'bar' => 'baz',
'zibble' => serialize('fritz'),
'zibble' => 'fritz',
];

$response = new RedirectResponse(new App());
$response->withInput();

$this->assertSame('bar', old('foo')); // regular parameter
$this->assertSame('doo', old('yabba dabba', 'doo')); // non-existing parameter
$this->assertSame('fritz', old('zibble')); // serialized parameter
$this->assertSame('fritz', old('zibble'));
}

/**
* @runInSeparateProcess
* @preserveGlobalState disabled
*/
public function testOldInputSerializeData()
{
$this->injectSessionMock();
// setup from RedirectResponseTest...
$_SERVER['REQUEST_METHOD'] = 'GET';

$this->config = new App();
$this->config->baseURL = 'http://example.com/';

$this->routes = new RouteCollection(Services::locator(), new Modules());
Services::injectMock('routes', $this->routes);

$this->request = new MockIncomingRequest($this->config, new URI('http://example.com'), null, new UserAgent());
Services::injectMock('request', $this->request);

// setup & ask for a redirect...
$_SESSION = [];
$_GET = [];
$_POST = [
'zibble' => serialize('fritz'),
];

$response = new RedirectResponse(new App());
$response->withInput();

// serialized parameters are only HTML-escaped.
$this->assertSame('s:5:"fritz";', old('zibble'));
}

/**
Expand Down
12 changes: 7 additions & 5 deletions tests/system/HTTP/IncomingRequestTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,10 @@ public function testMissingOldInput()
$this->assertNull($this->request->getOldInput('pineapple.name'));
}

// Reference: https://github.com/codeigniter4/CodeIgniter4/issues/1492
public function testCanGetOldInputArray()
/**
* @see https://github.com/codeigniter4/CodeIgniter4/issues/1492
*/
public function testCanGetOldInputArrayWithSESSION()
{
$_SESSION['_ci_old_input'] = [
'get' => ['apple' => ['name' => 'two']],
Expand All @@ -119,13 +121,13 @@ public function testCanGetOldInputArray()
$this->assertSame(['name' => 'foo'], $this->request->getOldInput('banana'));
}

// Reference: https://github.com/codeigniter4/CodeIgniter4/issues/1492

/**
* @see https://github.com/codeigniter4/CodeIgniter4/issues/1492
*
* @runInSeparateProcess
* @preserveGlobalState disabled
*/
public function testCanSerializeOldArray()
public function testCanGetOldInputArrayWithSessionService()
{
$locations = [
'AB' => 'Alberta',
Expand Down
5 changes: 5 additions & 0 deletions user_guide_src/source/changelogs/v4.1.6.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ Release Date: Not released
:local:
:depth: 2

SECURITY
********

- *Deserialization of Untrusted Data* found in the ``old()`` function was fixed. See the `Security advisory <https://github.com/codeigniter4/CodeIgniter4/security/advisories/GHSA-w6jr-wj64-mc9x>`_ for more information.

BREAKING
********

Expand Down

0 comments on commit ce95ed5

Please sign in to comment.