From 23156661db7acefe30334edfc2d67ac8d8e6fa4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matheus=20Bregu=C3=AAz?= Date: Wed, 16 Sep 2026 10:51:03 -0300 Subject: [PATCH] =?UTF-8?q?Recusa=20caminho=20que=20sai=20do=20pr=C3=B3pri?= =?UTF-8?q?o=20pacote=20nos=20outros=20tr=C3=AAs=20ZIPs=20(issue=20#242)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Não é fuga de diretório, e a distinção importa: medi o que o `extractTo()` do PHP faz com `../a.txt`, `x/../../c.txt` e `/tmp/abs-d.txt`, e nenhum escapa do destino. Um conserto guiado pela descrição errada mediria a coisa errada. O que sobra é pior de encontrar. O PHP resolve a travessia REESCREVENDO o caminho em silêncio: `x/../../data/secret/01.ans` vira `data/secret/01.ans` e passa por cima de um arquivo legítimo do próprio pacote. Num pacote de problema isso troca um caso de teste — o pacote é aceito, o problema é importado, e o sintoma aparece longe da causa: uma submissão correta reprovada durante a prova, com o placar já andando. `SafeZipExtractor` percorre os nomes antes de qualquer escrita e recusa o arquivo INTEIRO se algum for absoluto ou tiver um segmento `..`. Inteiro, e não só a entrada ruim, porque um pacote que tenta reescrever caminhos não é um pacote do qual se queira aproveitar o resto — e meia extração no disco da máquina que serve a prova é pior que nenhuma. As quatro portas passam a usá-lo, cada uma devolvendo a recusa no formato dela: frase na sessão no formulário de pacote ICPC e no do BOCA, 422 em `POST /api/problems`, `ZipPathException` em `ProblemPackageService`. O guardião local que o #239 tinha criado saiu, substituído pelo compartilhado. A medição que motiva tudo isso virou teste (`test_php_flattens_instead_of_escaping_which_is_why_the_guard_exists`) para que a razão não vire folclore. Se um dia ela falhar, a biblioteca mudou — e a guarda continua correta, porque não depende dela. Seis mutações, cada uma derrubando pelo menos um teste: tirando o ramo do caminho absoluto, tirando o do `..`, tirando a guarda do `extractTo`, e voltando cada uma das três portas ao `extractTo` cru. Controles positivos onde a guarda poderia ser larga demais: `a..b.pdf`, `x/y..z/w.txt` e `..oculto/x.txt` são nomes legítimos e continuam passando, e um pacote bem formado continua sendo importado pela API. Sem eles, uma guarda que recusasse tudo passaria nos testes de recusa do mesmo jeito. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Pvf2R8xtgbWQm1wRvjBqQY --- .../Controllers/Api/ProblemController.php | 15 +- .../ProblemPackageImportController.php | 48 ++---- app/Services/BocaImporterService.php | 21 ++- app/Services/ProblemPackageService.php | 12 +- app/Services/SafeZipExtractor.php | 69 ++++++++ app/Services/ZipPathException.php | 12 ++ docs/specs/242-zip-caminhos.md | 41 +++++ tests/Feature/ZipPathRefusalTest.php | 139 ++++++++++++++++ tests/Unit/SafeZipExtractorTest.php | 155 ++++++++++++++++++ 9 files changed, 465 insertions(+), 47 deletions(-) create mode 100644 app/Services/SafeZipExtractor.php create mode 100644 app/Services/ZipPathException.php create mode 100644 docs/specs/242-zip-caminhos.md create mode 100644 tests/Feature/ZipPathRefusalTest.php create mode 100644 tests/Unit/SafeZipExtractorTest.php diff --git a/app/Http/Controllers/Api/ProblemController.php b/app/Http/Controllers/Api/ProblemController.php index b3fcfcde..4570b7b8 100644 --- a/app/Http/Controllers/Api/ProblemController.php +++ b/app/Http/Controllers/Api/ProblemController.php @@ -8,6 +8,8 @@ use App\Services\Icpc\IcpcPackageException; use App\Services\Icpc\IcpcPackageImporter; use App\Services\ProblemPackageService; +use App\Services\SafeZipExtractor; +use App\Services\ZipPathException; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Support\Facades\Storage; @@ -93,9 +95,16 @@ private function importPackage(Contest $contest, $file, array $overrides): Probl abort(422, 'Nao foi possivel abrir o arquivo ZIP.'); } - @mkdir($extractDir, 0o755, true); - $zip->extractTo($extractDir); - $zip->close(); + try { + // Issue #242 -- um caminho com `..` seria reescrito em + // silencio e passaria por cima de outro arquivo do proprio + // pacote. 422 e nao 500: e erro de quem enviou. + app(SafeZipExtractor::class)->extractTo($zip, $extractDir); + } catch (ZipPathException $e) { + abort(422, $e->getMessage()); + } finally { + $zip->close(); + } if (! $this->looksLikeIcpcPackage($extractDir)) { // Formato do BOCA: o caminho que ja existia, intacto. diff --git a/app/Http/Controllers/Backend/ProblemPackageImportController.php b/app/Http/Controllers/Backend/ProblemPackageImportController.php index 3e5f7a13..f075f708 100644 --- a/app/Http/Controllers/Backend/ProblemPackageImportController.php +++ b/app/Http/Controllers/Backend/ProblemPackageImportController.php @@ -8,6 +8,8 @@ use App\Services\Icpc\IcpcPackageException; use App\Services\Icpc\IcpcPackageImporter; use App\Services\Icpc\IcpcPackageReader; +use App\Services\SafeZipExtractor; +use App\Services\ZipPathException; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; use Illuminate\View\View; @@ -40,7 +42,7 @@ public function show(Request $request): View ]); } - public function store(Request $request, IcpcPackageReader $reader, IcpcPackageImporter $importer): RedirectResponse + public function store(Request $request, IcpcPackageReader $reader, IcpcPackageImporter $importer, SafeZipExtractor $extractor): RedirectResponse { $validated = $request->validate([ 'contest_id' => 'required|exists:contests,id', @@ -64,16 +66,16 @@ public function store(Request $request, IcpcPackageReader $reader, IcpcPackageIm return back()->with('error', 'Não foi possível abrir o arquivo ZIP.')->withInput(); } - if (($fora = $this->nomeQueEscapa($zip)) !== null) { + try { + // Issue #242 -- a guarda roda ANTES de qualquer escrita: um + // pacote recusado nao pode deixar meia extracao no disco. + $extractor->extractTo($zip, $extractDir); + } catch (ZipPathException $e) { + return back()->with('error', $e->getMessage())->withInput(); + } finally { $zip->close(); - - return back()->with('error', "O pacote tem um caminho que sai do próprio diretório: {$fora}")->withInput(); } - @mkdir($extractDir, 0o755, true); - $zip->extractTo($extractDir); - $zip->close(); - try { $pacote = $reader->read($extractDir); } catch (IcpcPackageException $e) { @@ -109,36 +111,6 @@ public function store(Request $request, IcpcPackageReader $reader, IcpcPackageIm } } - /** - * O primeiro nome do ZIP que sai do diretório de extração, ou null. - * - * O `extractTo()` do PHP não deixa escapar -- medi: `../a.txt`, - * `x/../../c.txt` e `/tmp/abs-d.txt` caem todos DENTRO do destino. Mas - * ele resolve isso REESCREVENDO o caminho em silêncio, e é aí que mora o - * problema que sobra: `x/../../data/secret/01.ans` vira - * `data/secret/01.ans` e passa por cima de um caso de teste legítimo do - * mesmo pacote. O pacote é aceito, o problema é importado, e o caso de - * teste trocado só aparece quando uma submissão correta é reprovada na - * prova. - * - * Então a recusa é nossa, e não do PHP: um pacote com caminho assim - * volta como frase, sem nada extraído. Não depender da limpeza da - * biblioteca também é o que mantém isto verdadeiro se a versão do libzip - * da imagem mudar. - */ - private function nomeQueEscapa(ZipArchive $zip): ?string - { - for ($i = 0; $i < $zip->numFiles; $i++) { - $nome = (string) $zip->getNameIndex($i); - - if (str_starts_with($nome, '/') || preg_match('#(^|[\\\\/])\.\.([\\\\/]|$)#', $nome)) { - return $nome; - } - } - - return null; - } - /** * O que o pacote diz sobre si, para a pessoa conferir ANTES de aceitar. * diff --git a/app/Services/BocaImporterService.php b/app/Services/BocaImporterService.php index 46a5471a..ab3ca7e6 100644 --- a/app/Services/BocaImporterService.php +++ b/app/Services/BocaImporterService.php @@ -13,9 +13,14 @@ class BocaImporterService protected array $errors = []; protected array $imported = []; - public function __construct() + protected SafeZipExtractor $safeZip; + + public function __construct(?SafeZipExtractor $safeZip = null) { $this->tempPath = storage_path('app/temp/boca_import'); + // Opcional porque esta classe ainda e construida com `new` na rota + // de importacao do BOCA; o padrao mantem aquele caminho funcionando. + $this->safeZip = $safeZip ?? new SafeZipExtractor; } /** @@ -45,8 +50,18 @@ public function importFromZip(string $zipPath, ?int $contestId = null): array return $this->getResult(); } - $zip->extractTo($extractPath); - $zip->close(); + // Issue #242 -- um caminho com `..` seria reescrito em silencio pelo + // extractTo() e passaria por cima de outro arquivo do proprio + // pacote. A recusa e nossa, e vem antes de qualquer escrita. + try { + $this->safeZip->extractTo($zip, $extractPath); + } catch (ZipPathException $e) { + $this->errors[] = $e->getMessage(); + + return $this->getResult(); + } finally { + $zip->close(); + } // Find problem directories (BOCA structure) $problemDirs = $this->findProblemDirectories($extractPath); diff --git a/app/Services/ProblemPackageService.php b/app/Services/ProblemPackageService.php index 8d422875..aec3ad9e 100644 --- a/app/Services/ProblemPackageService.php +++ b/app/Services/ProblemPackageService.php @@ -32,9 +32,15 @@ public function importFromZip(Contest $contest, UploadedFile $file, array $overr // Extract to temp directory $tempExtractDir = storage_path('app/temp/extract_' . uniqid()); - mkdir($tempExtractDir, 0755, true); - $zip->extractTo($tempExtractDir); - $zip->close(); + + // Issue #242 -- um caminho com `..` seria reescrito em silencio + // pelo extractTo() e passaria por cima de outro arquivo do + // proprio pacote, trocando um caso de teste sem dizer nada. + try { + (new SafeZipExtractor)->extractTo($zip, $tempExtractDir); + } finally { + $zip->close(); + } // Parse problem.info $problemInfo = $this->parseProblemInfo($tempExtractDir); diff --git a/app/Services/SafeZipExtractor.php b/app/Services/SafeZipExtractor.php new file mode 100644 index 00000000..769db048 --- /dev/null +++ b/app/Services/SafeZipExtractor.php @@ -0,0 +1,69 @@ +numFiles; $i++) { + $name = (string) $zip->getNameIndex($i); + + if (str_starts_with($name, '/') || preg_match('#(^|[\\\\/])\.\.([\\\\/]|$)#', $name)) { + return $name; + } + } + + return null; + } + + /** + * Extrai, ou recusa o arquivo inteiro sem escrever nada. + * + * A verificacao vem antes de qualquer escrita de proposito: um pacote + * recusado nao pode deixar meia extracao no disco da maquina que serve + * a prova. + * + * @throws ZipPathException + */ + public function extractTo(ZipArchive $zip, string $directory): void + { + if (($offending = $this->escapingEntry($zip)) !== null) { + throw new ZipPathException("O arquivo tem um caminho que sai do próprio diretório: {$offending}"); + } + + if (! is_dir($directory)) { + @mkdir($directory, 0o755, true); + } + + $zip->extractTo($directory); + } +} diff --git a/app/Services/ZipPathException.php b/app/Services/ZipPathException.php new file mode 100644 index 00000000..279b1434 --- /dev/null +++ b/app/Services/ZipPathException.php @@ -0,0 +1,12 @@ +workspace = storage_path('app/temp/zippath-'.uniqid()); + @mkdir($this->workspace, 0o755, true); + } + + protected function tearDown(): void + { + $this->removeTree($this->workspace); + parent::tearDown(); + } + + private function removeTree(string $dir): void + { + if (! is_dir($dir)) { + return; + } + + foreach (glob($dir.'/*') ?: [] as $path) { + is_dir($path) ? $this->removeTree($path) : @unlink($path); + } + + @rmdir($dir); + } + + /** + * Um ZIP que, extraído como está, escreveria por cima de um arquivo + * legítimo do próprio pacote. + */ + private function travessia(): UploadedFile + { + $path = $this->workspace.'/travessia-'.uniqid().'.zip'; + $zip = new ZipArchive; + $zip->open($path, ZipArchive::CREATE); + $zip->addFromString('problem.info', "basename=soma\nfullname=Soma\n"); + $zip->addFromString('x/../../limits/c', 'limite trocado'); + $zip->close(); + + return new UploadedFile($path, 'pacote.zip', 'application/zip', null, true); + } + + public function test_the_boca_import_route_refuses_and_says_why(): void + { + $this->actingAs($this->createAdminUser()) + ->from(route('backend.import-boca')) + ->post('/backend/import-boca/upload', ['boca_zip' => $this->travessia()]) + ->assertRedirect(route('backend.import-boca')) + ->assertSessionHas('error', fn (string $erro) => str_contains($erro, 'sai do próprio diretório')); + + $this->assertSame(0, ProblemBank::count(), 'nada pode ter sido importado'); + } + + public function test_the_api_import_answers_422_and_not_500(): void + { + $contest = Contest::factory()->notStarted()->create(); + Sanctum::actingAs($this->createAdminUser()); + + $this->postJson('/api/problems', [ + 'contest_id' => $contest->id, + 'package' => $this->travessia(), + ]) + ->assertStatus(422) + ->assertJsonFragment(['message' => 'O arquivo tem um caminho que sai do próprio diretório: x/../../limits/c']); + + $this->assertSame(0, Problem::where('contest_id', $contest->id)->count()); + } + + public function test_the_package_service_throws_instead_of_importing(): void + { + $contest = Contest::factory()->notStarted()->create(); + + $this->expectException(ZipPathException::class); + + try { + app(ProblemPackageService::class)->importFromZip($contest, $this->travessia()); + } finally { + $this->assertSame(0, Problem::where('contest_id', $contest->id)->count()); + } + } + + /** + * Controle positivo das três portas: um pacote bem formado continua + * passando. Sem isto, uma guarda larga demais recusaria tudo e os testes + * acima passariam do mesmo jeito. + */ + public function test_a_clean_package_still_gets_through(): void + { + $contest = Contest::factory()->notStarted()->create(); + + $path = $this->workspace.'/limpo.zip'; + $zip = new ZipArchive; + $zip->open($path, ZipArchive::CREATE); + $zip->addFromString('problem.yaml', "problem_format_version: legacy-icpc\nname: Soma\nlimits:\n time_limit: 1\n"); + $zip->addFromString('data/sample/01.in', "1 2\n"); + $zip->addFromString('data/sample/01.ans', "3\n"); + $zip->addFromString('data/secret/01.in', "3 4\n"); + $zip->addFromString('data/secret/01.ans', "7\n"); + $zip->close(); + + Sanctum::actingAs($this->createAdminUser()); + + $this->postJson('/api/problems', [ + 'contest_id' => $contest->id, + 'package' => new UploadedFile($path, 'limpo.zip', 'application/zip', null, true), + ])->assertCreated(); + + $this->assertSame(1, Problem::where('contest_id', $contest->id)->count()); + + $this->removeTree(storage_path("app/problems/{$contest->id}")); + } +} diff --git a/tests/Unit/SafeZipExtractorTest.php b/tests/Unit/SafeZipExtractorTest.php new file mode 100644 index 00000000..42bee89b --- /dev/null +++ b/tests/Unit/SafeZipExtractorTest.php @@ -0,0 +1,155 @@ +workspace = storage_path('app/temp/safezip-'.uniqid()); + @mkdir($this->workspace, 0o755, true); + } + + protected function tearDown(): void + { + $this->removeTree($this->workspace); + parent::tearDown(); + } + + private function removeTree(string $dir): void + { + if (! is_dir($dir)) { + return; + } + + foreach (glob($dir.'/*') ?: [] as $path) { + is_dir($path) ? $this->removeTree($path) : @unlink($path); + } + + @rmdir($dir); + } + + /** + * @param array $entries + */ + private function zip(array $entries): ZipArchive + { + $path = $this->workspace.'/'.uniqid('z').'.zip'; + $zip = new ZipArchive; + $zip->open($path, ZipArchive::CREATE); + + foreach ($entries as $name) { + $zip->addFromString($name, 'conteudo'); + } + + $zip->close(); + + $aberto = new ZipArchive; + $aberto->open($path); + + return $aberto; + } + + // -- o que se recusa ----------------------------------------------------- + + #[TestWith(['../a.txt'])] + #[TestWith(['../../b.txt'])] + #[TestWith(['x/../../c.txt'])] + #[TestWith(['/tmp/abs-d.txt'])] + #[TestWith(['./../f.txt'])] + #[TestWith(['x/./../../g.txt'])] + #[TestWith(['a/..'])] + public function test_an_entry_that_does_not_stay_put_is_named(string $entrada): void + { + $this->assertSame($entrada, (new SafeZipExtractor)->escapingEntry($this->zip([$entrada]))); + } + + /** + * Controle positivo, e o mais importante desta classe: dois pontos NO + * MEIO de um nome são legítimos. Uma guarda que os recusasse rejeitaria + * pacotes bons, e a recusa não diria por quê. + */ + #[TestWith(['problem_statement/a..b.pdf'])] + #[TestWith(['data/secret/01.ans'])] + #[TestWith(['submissions/accepted/solucao.cpp'])] + #[TestWith(['..oculto/x.txt'])] + #[TestWith(['x/y..z/w.txt'])] + public function test_a_legitimate_name_is_not_mistaken_for_traversal(string $entrada): void + { + $this->assertNull((new SafeZipExtractor)->escapingEntry($this->zip([$entrada]))); + } + + // -- a extração ---------------------------------------------------------- + + public function test_refusing_writes_nothing(): void + { + $destino = $this->workspace.'/destino'; + + try { + (new SafeZipExtractor)->extractTo($this->zip(['ok.txt', 'x/../../fora.txt']), $destino); + $this->fail('um caminho que sai do diretório tinha que ser recusado'); + } catch (ZipPathException $e) { + $this->assertStringContainsString('x/../../fora.txt', $e->getMessage()); + } + + // Nem o `ok.txt`, que é legítimo: o arquivo é recusado inteiro, e a + // verificação vem antes de qualquer escrita de propósito -- meia + // extração no disco da máquina que serve a prova é pior que nenhuma. + $this->assertFalse(is_dir($destino), 'nada pode ter sido escrito'); + } + + public function test_a_clean_archive_extracts(): void + { + $destino = $this->workspace.'/limpo'; + + (new SafeZipExtractor)->extractTo($this->zip(['problem.yaml', 'data/secret/01.ans']), $destino); + + $this->assertFileExists($destino.'/problem.yaml'); + $this->assertFileExists($destino.'/data/secret/01.ans'); + } + + /** + * A medição que motivou a guarda, guardada como teste para que a razão + * não vire folclore. + * + * O `extractTo()` do PHP NÃO deixa escapar do destino -- o que ele faz é + * reescrever o caminho em silêncio. É por isso que um pacote com + * `x/../../data/secret/01.ans` passaria por cima de um caso de teste + * legítimo do próprio pacote sem erro e sem aviso, e é isso que a guarda + * impede. + * + * Se um dia esta asserção falhar, a biblioteca mudou de comportamento -- + * e a guarda continua correta de qualquer forma, porque não depende dela. + */ + public function test_php_flattens_instead_of_escaping_which_is_why_the_guard_exists(): void + { + $destino = $this->workspace.'/medicao'; + @mkdir($destino, 0o755, true); + + $zip = $this->zip(['x/../../data/secret/01.ans']); + $zip->extractTo($destino); + $zip->close(); + + $this->assertFileExists( + $destino.'/data/secret/01.ans', + 'o PHP achatou o caminho para dentro do destino, sobrescrevendo um caminho legítimo' + ); + $this->assertFileDoesNotExist( + dirname($destino).'/data/secret/01.ans', + 'e não escapou -- a guarda não é sobre fuga de diretório' + ); + } +}