Recusa caminho que sai do próprio pacote nos outros três ZIPs (issue #242)

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.

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.
      *

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);

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);

app/Services/SafeZipExtractor.php

@@ -0,0 +1,69 @@
+<?php
+
+namespace App\Services;
+
+use ZipArchive;
+
+/**
+ * Issue #242 -- extrair ZIP enviado por alguem sem deixar um caminho
+ * reescrito em silencio.
+ *
+ * Medi o que o `extractTo()` do PHP faz antes de escrever isto. Ele NAO
+ * deixa escapar do diretorio de destino: `../a.txt`, `x/../../c.txt` e
+ * `/tmp/abs.txt` caem todos dentro do destino. Nao e fuga de diretorio.
+ *
+ * O que sobra e pior de encontrar. Ele resolve a travessia REESCREVENDO o
+ * caminho, sem erro e sem aviso: `x/../../data/secret/01.ans` vira
+ * `data/secret/01.ans` e passa por cima de um arquivo legitimo do proprio
+ * pacote. Num pacote de problema isso troca um caso de teste -- o pacote e
+ * aceito, o problema e importado, e o sintoma aparece longe da causa: uma
+ * submissao correta reprovada DURANTE A PROVA.
+ *
+ * Entao a recusa e nossa, e nao da biblioteca. Nao depender da limpeza do
+ * libzip tambem e o que mantem a garantia verdadeira se a versao da imagem
+ * mudar.
+ */
+class SafeZipExtractor
+{
+    /**
+     * O primeiro nome que nao pode ser extraido como esta, ou null.
+     *
+     * Absoluto ou com um segmento `..`. Dois pontos NO MEIO de um nome
+     * (`a..b.pdf`) sao legitimos e passam -- uma guarda que os recusasse
+     * rejeitaria pacotes bons sem dizer por que.
+     */
+    public function escapingEntry(ZipArchive $zip): ?string
+    {
+        for ($i = 0; $i < $zip->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);
+    }
+}

app/Services/ZipPathException.php

@@ -0,0 +1,12 @@
+<?php
+
+namespace App\Services;
+
+/**
+ * Issue #242 -- um caminho que sai do proprio diretorio.
+ *
+ * Excecao propria pela mesma razao que a IcpcPackageException: um arquivo
+ * ruim e erro de quem enviou e volta como frase legivel, enquanto uma falha
+ * de disco e nossa e vira 500.
+ */
+class ZipPathException extends \RuntimeException {}

docs/specs/242-zip-caminhos.md

@@ -0,0 +1,41 @@
+# Caminhos que saem do próprio pacote (issue #242)
+
+Quatro lugares extraem um ZIP que alguém enviou:
+
+| porta | serviço | como a recusa aparece |
+|---|---|---|
+| `backend.import-package` | `ProblemPackageImportController` | frase na sessão, formulário mantém o que foi digitado |
+| `backend.import-boca/upload` | `BocaImporterService` | frase na sessão |
+| `POST /api/problems` | `Api\ProblemController` | 422 com a mensagem |
+| — | `ProblemPackageService::importFromZip` | `ZipPathException` |
+
+## Não é fuga de diretório
+
+Medi antes de escrever a guarda. O `extractTo()` do PHP **não deixa escapar** do destino (PHP 8.5, libzip 1.22.8):
+
+| entrada no ZIP | onde caiu |
+|---|---|
+| `../a.txt` | `alvo/a.txt` |
+| `../../b.txt` | `alvo/b.txt` |
+| `x/../../c.txt` | `alvo/c.txt` |
+| `/tmp/abs-d.txt` | `alvo/tmp/abs-d.txt` |
+
+Nada saiu. Chamar isto de Zip Slip seria descrever errado o que acontece — e um conserto guiado pela descrição errada mediria a coisa errada.
+
+## O que sobra
+
+O PHP resolve a travessia **reescrevendo o caminho em silêncio**. Sem erro, sem aviso.
+
+`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.
+
+`Tests\Unit\SafeZipExtractorTest::test_php_flattens_instead_of_escaping_which_is_why_the_guard_exists` guarda essa medição como teste, 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.
+
+## A guarda
+
+`SafeZipExtractor::escapingEntry()` percorre os nomes antes de qualquer escrita e devolve o primeiro que for absoluto ou tiver um segmento `..`. `extractTo()` recusa o arquivo **inteiro** nesse caso: meia extração no disco da máquina que serve a prova é pior que nenhuma.
+
+O arquivo 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.
+
+## O que **não** se recusa
+
+Dois pontos no meio de um nome são legítimos: `problem_statement/a..b.pdf`, `x/y..z/w.txt`, `..oculto/x.txt`. Uma guarda que os recusasse rejeitaria pacotes bons e a recusa não diria por quê. São cinco controles positivos no teste da guarda, mais um em cada ponta do fluxo de importação.