Skip to content

Commit f27fdcc

Browse files
tvdijentvdijen
committed
Merge branch 'master' into release-7.0
2 parents 008afc5 + b84f3e4 commit f27fdcc

6 files changed

Lines changed: 311 additions & 4 deletions

File tree

‎src/Cas/Ticket/FileSystemTicketStore.php‎

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727

2828
use Exception;
2929
use SimpleSAML\Configuration;
30+
use SimpleSAML\XML\Assert\Assert;
3031

3132
class FileSystemTicketStore extends TicketStore
3233
{
@@ -66,12 +67,15 @@ public function __construct(Configuration $config)
6667
*/
6768
public function getTicket(string $ticketId): ?array
6869
{
69-
$filename = $this->pathToTicketDirectory . '/' . $ticketId;
70+
$filename = $this->validateTicketPath($ticketId);
7071

7172
if (file_exists($filename)) {
7273
$content = file_get_contents($filename);
7374

74-
return unserialize($content);
75+
$result = unserialize($content, ['allowed_classes' => false]);
76+
Assert::isArray($result, "Failed to unserialize ticket.");
77+
78+
return $result;
7579
} else {
7680
return null;
7781
}
@@ -83,7 +87,7 @@ public function getTicket(string $ticketId): ?array
8387
*/
8488
public function addTicket(array $ticket): void
8589
{
86-
$filename = $this->pathToTicketDirectory . '/' . $ticket['id'];
90+
$filename = $this->validateTicketPath($ticket['id']);
8791
file_put_contents($filename, serialize($ticket));
8892
}
8993

@@ -93,10 +97,42 @@ public function addTicket(array $ticket): void
9397
*/
9498
public function deleteTicket(string $ticketId): void
9599
{
96-
$filename = $this->pathToTicketDirectory . '/' . $ticketId;
100+
$filename = $this->validateTicketPath($ticketId);
97101

98102
if (file_exists($filename)) {
99103
unlink($filename);
100104
}
101105
}
106+
107+
108+
/**
109+
* @param string $ticketId
110+
* @return string
111+
* @throws \Exception
112+
*/
113+
private function validateTicketPath(string $ticketId): string
114+
{
115+
if ($ticketId === '') {
116+
throw new Exception('Invalid ticketId provided.');
117+
}
118+
119+
// Prevent directory traversal and path separators.
120+
// CAS tickets are typically like: ST-..., PT-..., PGT-..., etc. Hyphens are ok.
121+
if (
122+
str_contains($ticketId, '/')
123+
|| str_contains($ticketId, '\\')
124+
|| str_contains($ticketId, '..')
125+
) {
126+
throw new Exception('Invalid ticketId provided.');
127+
}
128+
129+
$baseDir = realpath($this->pathToTicketDirectory);
130+
if ($baseDir === false) {
131+
throw new Exception('Ticket storage directory is not accessible.');
132+
}
133+
134+
// Store tickets as hashed filenames inside the ticket directory.
135+
// (Avoids issues with special chars / very long filenames.)
136+
return $baseDir . '/' . sha1($ticketId);
137+
}
102138
}

‎src/Controller/Cas10Controller.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,14 @@ public function validate(
101101
);
102102
}
103103

104+
if ($ticket === '' || str_contains($ticket, "\0")) {
105+
Logger::debug('casserver: Illegal value for ticket parameter.');
106+
return new Response(
107+
$this->cas10Protocol->getValidateFailureResponse(),
108+
Response::HTTP_BAD_REQUEST,
109+
);
110+
}
111+
104112
try {
105113
// Get the service ticket
106114
// `getTicket` uses the unserializable method and Objects may throw Throwables in their

‎src/Controller/Traits/TicketValidatorTrait.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,16 @@ public function validate(
5151
);
5252
}
5353

54+
if ($ticket === '' || str_contains($ticket, "\0")) {
55+
$message = 'Illegal value for ticket parameter.';
56+
Logger::debug($message);
57+
58+
return new XmlResponse(
59+
(string) $this->cas20Protocol->getValidateFailureResponse(C::ERR_INVALID_REQUEST, $message),
60+
Response::HTTP_BAD_REQUEST,
61+
);
62+
}
63+
5464
try {
5565
// Get the service ticket
5666
// `getTicket` uses the unserializable method and Objects may throw "Throwables" in their

‎tests/src/Controller/Cas10ControllerTest.php‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,43 @@ public function testReturnBadRequestOnEmptyServiceOrTicket(array $params): void
116116
}
117117

118118

119+
public static function illegalTicketValues(): array
120+
{
121+
return [
122+
'Ticket is empty string' => [
123+
['service' => 'https://myservice.com/abcd', 'ticket' => ''],
124+
],
125+
'Ticket contains NUL byte' => [
126+
['service' => 'https://myservice.com/abcd', 'ticket' => "ST-\0-123"],
127+
],
128+
];
129+
}
130+
131+
132+
/**
133+
* @param array $params
134+
*
135+
* @return void
136+
* @throws \Exception
137+
*/
138+
#[DataProvider('illegalTicketValues')]
139+
public function testReturnBadRequestOnIllegalTicketValue(array $params): void
140+
{
141+
$config = Configuration::loadFromArray($this->moduleConfig);
142+
143+
$request = Request::create(
144+
uri: 'http://localhost',
145+
parameters: $params,
146+
);
147+
148+
$cas10Controller = new Cas10Controller($this->sspConfig, $config);
149+
$response = $cas10Controller->validate($request, ...$params);
150+
151+
$this->assertEquals(400, $response->getStatusCode());
152+
$this->assertEquals("no\n\n", $response->getContent());
153+
}
154+
155+
119156
/**
120157
* @return void
121158
* @throws \Exception

‎tests/src/Controller/Cas20ControllerTest.php‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -406,6 +406,50 @@ public function testServiceValidateFailing(array $requestParams, string $message
406406
}
407407

408408

409+
public static function validateFailsForIllegalTicketValue(): array
410+
{
411+
return [
412+
'Ticket is empty string' => [
413+
['service' => 'http://localhost', 'ticket' => ''],
414+
'Illegal value for ticket parameter.',
415+
],
416+
'Ticket contains NUL byte' => [
417+
['service' => 'http://localhost', 'ticket' => "ST-\0-123"],
418+
'Illegal value for ticket parameter.',
419+
],
420+
];
421+
}
422+
423+
424+
#[DataProvider('validateFailsForIllegalTicketValue')]
425+
public function testServiceValidateFailsForIllegalTicketValue(array $requestParams, string $message): void
426+
{
427+
$casconfig = Configuration::loadFromArray($this->moduleConfig);
428+
$request = Request::create(
429+
uri: '/',
430+
parameters: $requestParams,
431+
);
432+
433+
$cas20Controller = new Cas20Controller(
434+
$this->sspConfig,
435+
$casconfig,
436+
);
437+
438+
$response = $cas20Controller->serviceValidate($request, ...$requestParams);
439+
440+
$this->assertEquals(Response::HTTP_BAD_REQUEST, $response->getStatusCode());
441+
$this->assertStringContainsString($message, (string) $response->getContent());
442+
443+
$xml = simplexml_load_string((string) $response->getContent());
444+
$xml->registerXPathNamespace('cas', 'serviceResponse');
445+
$this->assertEquals('serviceResponse', $xml->getName());
446+
$this->assertEquals(
447+
C::ERR_INVALID_REQUEST,
448+
$xml->xpath('//cas:authenticationFailure')[0]->attributes()['code'],
449+
);
450+
}
451+
452+
409453
public function testReturn500OnDeleteTicketThatThrows(): void
410454
{
411455
$config = Configuration::loadFromArray($this->moduleConfig);

‎tests/src/Ticket/FileSystemTicketStoreTest.php‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace SimpleSAML\Module\casserver\Tests\Cas\Ticket;
6+
7+
use Exception;
8+
use PHPUnit\Framework\TestCase;
9+
use SimpleSAML\Configuration;
10+
use SimpleSAML\Module\casserver\Cas\Ticket\FileSystemTicketStore;
11+
12+
final class FileSystemTicketStoreTest extends TestCase
13+
{
14+
private string $ticketDir;
15+
16+
17+
protected function setUp(): void
18+
{
19+
parent::setUp();
20+
21+
$base = rtrim(sys_get_temp_dir(), DIRECTORY_SEPARATOR)
22+
. DIRECTORY_SEPARATOR
23+
. 'casserver-ticketstore-test-'
24+
. bin2hex(random_bytes(8));
25+
26+
if (!mkdir($base, 0700, true) && !is_dir($base)) {
27+
self::fail('Failed to create test directory: ' . $base);
28+
}
29+
30+
$this->ticketDir = $base;
31+
}
32+
33+
34+
protected function tearDown(): void
35+
{
36+
$this->rmrf($this->ticketDir);
37+
parent::tearDown();
38+
}
39+
40+
41+
public function testPathTraversalCannotReadOrUnserializeOutsideTicketDirectory(): void
42+
{
43+
$outsideFile = dirname($this->ticketDir) . DIRECTORY_SEPARATOR . 'outside-' . bin2hex(random_bytes(6)) . '.ser';
44+
file_put_contents($outsideFile, serialize(['pwn' => true]));
45+
46+
$store = $this->createStore($this->ticketDir);
47+
48+
$this->expectException(Exception::class);
49+
$this->expectExceptionMessage('Invalid ticketId provided.');
50+
$store->getTicket('../' . basename($outsideFile));
51+
}
52+
53+
54+
public function testValidateTicketPathThrowsOnEmptyTicketId(): void
55+
{
56+
$store = $this->createStore($this->ticketDir);
57+
58+
$this->expectException(Exception::class);
59+
$this->expectExceptionMessage('Invalid ticketId provided.');
60+
$this->invokeValidateTicketPath($store, '');
61+
}
62+
63+
64+
public function testValidateTicketPathThrowsOnForwardSlash(): void
65+
{
66+
$store = $this->createStore($this->ticketDir);
67+
68+
$this->expectException(Exception::class);
69+
$this->expectExceptionMessage('Invalid ticketId provided.');
70+
$this->invokeValidateTicketPath($store, 'ST-123/456');
71+
}
72+
73+
74+
public function testValidateTicketPathThrowsOnBackslash(): void
75+
{
76+
$store = $this->createStore($this->ticketDir);
77+
78+
$this->expectException(Exception::class);
79+
$this->expectExceptionMessage('Invalid ticketId provided.');
80+
$this->invokeValidateTicketPath($store, 'ST-123\\456');
81+
}
82+
83+
84+
public function testValidateTicketPathThrowsOnDotDot(): void
85+
{
86+
$store = $this->createStore($this->ticketDir);
87+
88+
$this->expectException(Exception::class);
89+
$this->expectExceptionMessage('Invalid ticketId provided.');
90+
$this->invokeValidateTicketPath($store, 'ST-..-123');
91+
}
92+
93+
94+
public function testValidateTicketPathThrowsWhenTicketStorageDirectoryIsNotAccessible(): void
95+
{
96+
$store = $this->createStore($this->ticketDir);
97+
98+
$rp = new \ReflectionProperty($store, 'pathToTicketDirectory');
99+
$rp->setAccessible(true);
100+
$rp->setValue($store, $this->ticketDir . DIRECTORY_SEPARATOR . 'does-not-exist-' . bin2hex(random_bytes(4)));
101+
102+
$this->expectException(Exception::class);
103+
$this->expectExceptionMessage('Ticket storage directory is not accessible.');
104+
$this->invokeValidateTicketPath($store, 'ST-12345');
105+
}
106+
107+
108+
public function testValidateTicketPathReturnsAFileInsideTicketDirectory(): void
109+
{
110+
$store = $this->createStore($this->ticketDir);
111+
112+
$ticketId = 'ST-' . bin2hex(random_bytes(8));
113+
$path = $this->invokeValidateTicketPath($store, $ticketId);
114+
115+
$realBase = realpath($this->ticketDir);
116+
self::assertIsString($realBase);
117+
118+
self::assertStringStartsWith($realBase . DIRECTORY_SEPARATOR, $path);
119+
self::assertSame($realBase . DIRECTORY_SEPARATOR . sha1($ticketId), $path);
120+
}
121+
122+
123+
private function createStore(string $ticketDir): FileSystemTicketStore
124+
{
125+
$config = Configuration::loadFromArray([
126+
'ticketstore' => [
127+
'directory' => $ticketDir,
128+
],
129+
]);
130+
131+
return new FileSystemTicketStore($config);
132+
}
133+
134+
135+
private function invokeValidateTicketPath(FileSystemTicketStore $store, string $ticketId): string
136+
{
137+
$rm = new \ReflectionMethod($store, 'validateTicketPath');
138+
$rm->setAccessible(true);
139+
140+
$result = $rm->invoke($store, $ticketId);
141+
self::assertIsString($result);
142+
143+
return $result;
144+
}
145+
146+
147+
private function rmrf(string $path): void
148+
{
149+
if ($path === '' || !file_exists($path)) {
150+
return;
151+
}
152+
153+
if (is_file($path) || is_link($path)) {
154+
@unlink($path);
155+
return;
156+
}
157+
158+
$items = scandir($path);
159+
if ($items === false) {
160+
return;
161+
}
162+
163+
foreach ($items as $item) {
164+
if ($item === '.' || $item === '..') {
165+
continue;
166+
}
167+
$this->rmrf($path . DIRECTORY_SEPARATOR . $item);
168+
}
169+
170+
@rmdir($path);
171+
}
172+
}

0 commit comments

Comments
 (0)