Skip to content
This repository was archived by the owner on Mar 3, 2020. It is now read-only.

Commit 780071b

Browse files
justinwrayjustinwray
authored
Attachment Security Update (#590)
* Attachment Security Update * Attachments have been moved out of a web accessible direcotry (`/var/www/fbctf/src/data/`) and now reside in `/var/www/fbctf/attachments`. * Attachment downloads are now handled by an endpoint in `/data`: `attachment.php`. * All links to attachments now refer to the correct `attachment.php` endpoint location. * The `tar` command within the Attachment Import function now sets the mode to 600 at time of extraction. * The Attachment Import functionality will no longer change or attempt to change permissions on the current or parent directories. * The Attachment Import functionality will no longer change permissions on any directories, though subdirectories are not supported. * Attachment filenames will no longer be altered, excluding the inclusion of the file hash. * The provision script has been updated to support the new Attachment directory location. * Attachment specific directives are no longer set in the Mult-Server Nginx configuration. * Attachment location information has been updated in the `.gitignore` configuration. * Fixed an issue with the deletion path.
1 parent 80da145 commit 780071b

File tree

11 files changed

+119
-60
lines changed

11 files changed

+119
-60
lines changed

‎.gitignore

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,10 @@ www.pid
3131
*.swo
3232

3333
# Attachments directory
34-
src/data/attachments/*
35-
!src/data/attachments/index.php
36-
src/data/attachments/deleted/*
37-
!src/data/attachments/deleted/index.php
34+
attachments/*
35+
!attachments/index.php
36+
attachments/deleted/*
37+
!attachments/deleted/index.php
3838

3939
# Custom logos directory
4040
src/data/customlogos/*

‎extra/nginx/nginx.conf

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,6 @@ server {
3838
root CTFPATH;
3939
index index.php;
4040

41-
location /data/attachments/ {
42-
fastcgi_param SCRIPT_FILENAME $document_root$fastcgi_script_name;
43-
include fastcgi_params;
44-
fastcgi_pass HHVMSERVER:9000;
45-
}
46-
4741
location /data/customlogos/ {
4842
fastcgi_param SCRIPT_FILENAME $document_root$fastcgi_script_name;
4943
include fastcgi_params;

‎extra/provision.sh

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -326,10 +326,10 @@ fi
326326
fi
327327

328328
log "Creating attachments folder, and setting ownership to www-data"
329-
sudo sudo mkdir -p "$CTF_PATH/src/data/attachments"
330-
sudo sudo mkdir -p "$CTF_PATH/src/data/attachments/deleted"
331-
sudo chown -R www-data:www-data "$CTF_PATH/src/data/attachments"
332-
sudo chown -R www-data:www-data "$CTF_PATH/src/data/attachments/deleted"
329+
sudo sudo mkdir -p "$CTF_PATH/attachments"
330+
sudo sudo mkdir -p "$CTF_PATH/attachments/deleted"
331+
sudo chown -R www-data:www-data "$CTF_PATH/attachments"
332+
sudo chown -R www-data:www-data "$CTF_PATH/attachments/deleted"
333333

334334
log "Creating custom logos folder, and setting ownership to www-data"
335335
sudo mkdir -p "$CTF_PATH/src/data/customlogos"

‎src/controllers/AdminController.php

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ class AdminController extends Controller {
44
<<__Override>>
55
protected function getTitle(): string {
66
$custom_org = \HH\Asio\join(Configuration::gen('custom_org'));
7-
return tr($custom_org->getValue()). ' '. tr('CTF'). ' | '. tr('Admin');
7+
return tr($custom_org->getValue()).' '.tr('CTF').' | '.tr('Admin');
88
}
99

1010
<<__Override>>
@@ -157,8 +157,8 @@ class="fb--conf--registration_type"
157157
$select = <select name="fb--conf--password_type"></select>;
158158
foreach ($types as $type) {
159159
$select->appendChild(
160-
<option
161-
class="fb--conf--password_type"
160+
<option
161+
class="fb--conf--password_type"
162162
value={strval($type->getField())}
163163
selected={($type->getField() === $config->getField())}>
164164
{$type->getDescription()}
@@ -476,15 +476,15 @@ class="fb-cta cta--yellow"
476476
$custom_logo_xhp =
477477
<div class="form-el el--block-label el--full-text">
478478
<label for="">{tr('Logo')}</label>
479-
<img
480-
id="custom-logo-image"
481-
class="icon--badge"
479+
<img
480+
id="custom-logo-image"
481+
class="icon--badge"
482482
src={$custom_logo_image->getValue()}
483483
/>
484-
<br/>
485-
<h6>
484+
<br />
485+
<h6>
486486
<a class="icon-text" href="#" id="custom-logo-link">
487-
{tr('Change')}
487+
{tr('Change')}
488488
</a>
489489
</h6>
490490
<input
@@ -2014,7 +2014,7 @@ class="fb-cta cta--yellow"
20142014
value={$attachment->getFilename()}
20152015
disabled={true}
20162016
/>
2017-
<a href={$attachment->getFilename()} target="_blank">
2017+
<a href={$attachment->getFileLink()} target="_blank">
20182018
{tr('Link')}
20192019
</a>
20202020
</div>
@@ -2547,7 +2547,7 @@ class="fb-cta cta--yellow"
25472547
value={$attachment->getFilename()}
25482548
disabled={true}
25492549
/>
2550-
<a href={$attachment->getFilename()} target="_blank">
2550+
<a href={$attachment->getFileLink()} target="_blank">
25512551
{tr('Link')}
25522552
</a>
25532553
</div>

‎src/data/attachment.php

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
<?hh // strict
2+
3+
require_once ($_SERVER['DOCUMENT_ROOT'].'/../vendor/autoload.php');
4+
5+
class AttachmentDataController extends DataController {
6+
public async function genGenerateData(): Awaitable<void> {
7+
8+
/* HH_IGNORE_ERROR[1002] */
9+
SessionUtils::sessionStart();
10+
SessionUtils::enforceLogin();
11+
12+
await tr_start();
13+
14+
$data = tr('File Does Not Exist');
15+
$filename = tr('error');
16+
17+
$attachment_id = idx(Utils::getGET(), 'id', '');
18+
if (intval($attachment_id) !== 0) {
19+
$attachment_exists =
20+
await Attachment::genCheckExists(intval($attachment_id));
21+
if ($attachment_exists === true) {
22+
$attachment = await Attachment::gen(intval($attachment_id));
23+
$filename = $attachment->getFilename();
24+
25+
// Remove all non alpahnum characters from filename - allow international chars, dash, underscore, and period
26+
$filename = preg_replace('/[^\p{L}\p{N}_\-.]+/u', '_', $filename);
27+
28+
$data = readfile(Attachment::attachmentsDir.$filename);
29+
}
30+
}
31+
32+
header('Content-Type: application/octet-stream');
33+
header("Content-Transfer-Encoding: Binary");
34+
header('Content-disposition: attachment; filename="'.$filename.'"');
35+
print $data;
36+
}
37+
}
38+
39+
/* HH_IGNORE_ERROR[1002] */
40+
$attachment_file = new AttachmentDataController();
41+
\HH\Asio\join($attachment_file->genGenerateData());

‎src/data/attachments/deleted/index.php

Lines changed: 0 additions & 3 deletions
This file was deleted.

‎src/data/attachments/index.php

Lines changed: 0 additions & 3 deletions
This file was deleted.

‎src/data/country-data.php

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,10 @@ class CountryDataController extends DataController {
6363
$all_attachments =
6464
await Attachment::genAllAttachments($level->getId());
6565
foreach ($all_attachments as $attachment) {
66-
array_push($attachments_list, $attachment->getFilename());
66+
$attachment_details = array();
67+
$attachment_details['filename'] = $attachment->getFilename();
68+
$attachment_details['file_link'] = $attachment->getFileLink();
69+
array_push($attachments_list, $attachment_details);
6770
}
6871
}
6972

‎src/models/Attachment.php

Lines changed: 44 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
class Attachment extends Model {
44
// TODO: Configure this
5-
const string attachmentsDir = '/data/attachments/';
5+
const string attachmentsDir = '/var/www/fbctf/attachments/';
66

77
protected static string $MC_KEY = 'attachments:';
88

@@ -17,6 +17,7 @@ private function __construct(
1717
private int $id,
1818
private int $levelId,
1919
private string $filename,
20+
private string $link,
2021
private string $type,
2122
) {}
2223

@@ -28,6 +29,10 @@ public function getFilename(): string {
2829
return $this->filename;
2930
}
3031

32+
public function getFileLink(): string {
33+
return $this->link;
34+
}
35+
3136
public function getType(): string {
3237
return $this->type;
3338
}
@@ -44,7 +49,8 @@ public function getLevelId(): int {
4449
): Awaitable<bool> {
4550
$db = await self::genDb();
4651
$type = '';
47-
$local_filename = self::attachmentsDir;
52+
$file_path = self::attachmentsDir;
53+
$local_filename = '';
4854

4955
$files = Utils::getFILES();
5056
$server = Utils::getSERVER();
@@ -63,30 +69,21 @@ public function getLevelId(): int {
6369
$local_filename .= '.'.$extension;
6470
}
6571

66-
// Avoid php shells
67-
if (ends_with($local_filename, '.php')) {
68-
$local_filename .= 's'; // Make the extension 'phps'
69-
}
70-
move_uploaded_file(
71-
$tmp_name,
72-
must_have_string($server, 'DOCUMENT_ROOT').$local_filename,
73-
);
72+
// Remove all non alpahnum characters from filename - allow international chars, dash, underscore, and period
73+
$local_filename =
74+
preg_replace('/[^\p{L}\p{N}_\-.]+/u', '_', $local_filename);
75+
76+
move_uploaded_file($tmp_name, $file_path.$local_filename);
7477

7578
// Force 0600 Permissions
76-
$chmod = chmod(
77-
must_have_string($server, 'DOCUMENT_ROOT').$local_filename,
78-
0600,
79-
);
79+
$chmod = chmod($file_path.$local_filename, 0600);
8080
invariant(
8181
$chmod === true,
8282
'Failed to set attachment file permissions to 0600',
8383
);
8484

8585
// Force ownership to www-data
86-
$chown = chown(
87-
must_have_string($server, 'DOCUMENT_ROOT').$local_filename,
88-
'www-data',
89-
);
86+
$chown = chown($file_path.$local_filename, 'www-data');
9087
invariant(
9188
$chown === true,
9289
'Failed to set attachment file ownership to www-data',
@@ -132,7 +129,7 @@ public function getLevelId(): int {
132129

133130
// Copy file to deleted folder
134131
$attachment = await self::gen($attachment_id);
135-
$filename = $attachment->getFilename();
132+
$filename = self::attachmentsDir.$attachment->getFilename();
136133
$parts = pathinfo($filename);
137134
error_log(
138135
'Copying from '.
@@ -142,9 +139,8 @@ public function getLevelId(): int {
142139
'/deleted/'.
143140
$parts['basename'],
144141
);
145-
$root = strval($server['DOCUMENT_ROOT']);
146-
$origin = $root.$filename;
147-
$dest = $root.$parts['dirname'].'/deleted/'.$parts['basename'];
142+
$origin = $filename;
143+
$dest = $parts['dirname'].'/deleted/'.$parts['basename'];
148144
copy($origin, $dest);
149145

150146
// Delete file.
@@ -242,6 +238,31 @@ public function getLevelId(): int {
242238
}
243239
}
244240

241+
public static async function genCheckExists(
242+
int $attachment_id,
243+
bool $refresh = false,
244+
): Awaitable<bool> {
245+
$mc_result = self::getMCRecords('ATTACHMENTS');
246+
if (!$mc_result || count($mc_result) === 0 || $refresh) {
247+
$db = await self::genDb();
248+
$attachments = Map {};
249+
$result = await $db->queryf('SELECT * FROM attachments');
250+
foreach ($result->mapRows() as $row) {
251+
$attachments->add(
252+
Pair {intval($row->get('id')), self::attachmentFromRow($row)},
253+
);
254+
}
255+
self::setMCRecords('ATTACHMENTS', $attachments);
256+
return $attachments->contains($attachment_id);
257+
} else {
258+
invariant(
259+
$mc_result instanceof Map,
260+
'cache return should be of type Map',
261+
);
262+
return $mc_result->contains($attachment_id);
263+
}
264+
}
265+
245266
// Check if a level has attachments.
246267
public static async function genHasAttachments(
247268
int $level_id,
@@ -304,6 +325,7 @@ private static function attachmentFromRow(
304325
intval(must_have_idx($row, 'id')),
305326
intval(must_have_idx($row, 'level_id')),
306327
must_have_idx($row, 'filename'),
328+
strval('/data/attachment.php?id='.intval(must_have_idx($row, 'id'))),
307329
must_have_idx($row, 'type'),
308330
);
309331
}

‎src/models/Control.php

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -422,14 +422,17 @@ class Control extends Model {
422422
$filename =
423423
strval(BinaryImporterController::getFilename('attachments_file'));
424424
$document_root = must_have_string(Utils::getSERVER(), 'DOCUMENT_ROOT');
425-
$directory = $document_root.Attachment::attachmentsDir;
426-
$cmd = "tar -zx -C $directory -f $filename";
425+
$directory = Attachment::attachmentsDir;
426+
$cmd = "tar -zx --mode=600 -C $directory -f $filename";
427427
exec($cmd, $output, $status);
428428
if (intval($status) !== 0) {
429429
return false;
430430
}
431-
$directory_files = scandir($directory);
431+
$directory_files = array_slice(scandir($directory), 2);
432432
foreach ($directory_files as $file) {
433+
if (is_dir($file) === true) {
434+
continue;
435+
}
433436
$chmod = chmod($directory.$file, 0600);
434437
invariant(
435438
$chmod === true,
@@ -507,7 +510,7 @@ class Control extends Model {
507510
header('Content-Type: application/x-tgz');
508511
header('Content-Disposition: attachment; filename="'.$filename.'"');
509512
$document_root = must_have_string(Utils::getSERVER(), 'DOCUMENT_ROOT');
510-
$directory = $document_root.Attachment::attachmentsDir;
513+
$directory = Attachment::attachmentsDir;
511514
$cmd = "tar -cz -C $directory . ";
512515
passthru($cmd);
513516
exit();

0 commit comments

Comments
 (0)