Repository navigation
Recusa caminho que sai do próprio pacote nos outros três ZIPs (issue #242) - #243
Merged
Merged
Conversation
…242) 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pvf2R8xtgbWQm1wRvjBqQY
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fecha #242.
Não é fuga de diretório, e a distinção importa
Medi antes de consertar. O
extractTo()do PHP não deixa escapar do destino (PHP 8.5, libzip 1.22.8):../a.txtalvo/a.txt../../b.txtalvo/b.txtx/../../c.txtalvo/c.txt/tmp/abs-d.txtalvo/tmp/abs-d.txtNada saiu. Chamar isto de Zip Slip descreveria 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.ansviradata/secret/01.anse 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.SafeZipExtractorTest::test_php_flattens_instead_of_escaping_which_is_why_the_guard_existsguarda 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, e as quatro portas
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 — 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.backend.import-packagebackend.import-boca/uploadPOST /api/problemsProblemPackageService::importFromZipZipPathExceptionO guardião local que o #239 tinha criado saiu, substituído pelo compartilhado.
Mutações
..extractToBocaImporterServicevolta aoextractTocruProblemPackageServicevolta ao cruApi\ProblemControllervolta ao cruControles positivos
Uma guarda larga demais passaria em todos os testes de recusa acima. Então:
problem_statement/a..b.pdf,x/y..z/w.txte..oculto/x.txtsão nomes legítimos e continuam passando, e um pacote bem formado continua sendo importado pela API com 201.Verificação
./vendor/bin/phpunit --no-coverage— 1608 testes, verdeApi\ProblemControllervoltou a passar (eu tinha quebrado a ordem dos imports).BocaImporterServiceeProblemPackageServicejá falhavam no master antes deste PR e ficaram como estão — reformatá-los inteiros esconderia esta mudança num diff de estilo.🤖 Generated with Claude Code
https://claude.ai/code/session_01Pvf2R8xtgbWQm1wRvjBqQY