Skip to content

server-core-worker: validateEntry skips the bucket-membership check for every non-file tar entry #1864

Description

@tada5hi

Found while auditing #1862. Pre-existing and byte-identical to master — not introduced by any recent change.

What

apps/server-core-worker/src/app/components/analysis-builder/handlers/execute/module.ts:301

validateEntry: (entry) => {
    if (!entry.type || entry.type !== 'file') {
        return;                                   // <-- everything non-'file' skips validation
    }

    const index = analysisBucketFiles.findIndex((f) => f.path === entry.name);
    if (index === -1) {
        throw new Error(`Bucket file ${entry.name} is not a valid analysis bucket file.`);
    }
},

This is the check that rejects tar entries not belonging to the analysis bucket. Every entry whose type is not exactly 'file' — symlinks, directories, and entries with an unrecognised typeflag — returns early and is never checked against analysisBucketFiles.

It compounds: tar-stream's headers.js toType() returns null for any unrecognised typeflag, and pack.js:148 then rewrites a null-typed entry back to a regular file on the way out. So an entry can skip validation as "not a file" and still land in the container as one. This was demonstrated end-to-end during the audit with a crafted typeflag V.

Bounded, which is why this is not urgent

The only producer of that tar is server-storage's packFile(), which always emits type: 'file'. Reaching the bypass requires injecting raw tar bytes into the stream between storage and the worker, not merely uploading a crafted bucket file.

Suggested fix

Invert to an allow-list rather than an early-return deny-list, and validate names for directories too:

validateEntry: (entry) => {
    if (entry.type !== 'file' && entry.type !== 'directory') {
        throw new Error(`Unsupported tar entry type ${entry.type} for ${entry.name}.`);
    }
    // ... existing membership check, for both kinds
},

One note for whoever picks this up

Keep the !entry.type arm. It is logically redundant (!t implies t !== 'file' for every value — verified over the full truth table), but it is the last textual hint in this codebase that null occurs at runtime. tar-stream's own bundled declarations claim type is a required non-nullable union of 8 literals, while the decoder produces 13 possibilities including null; the older @types/tar-stream model had all 13. See #1862 for that discrepancy.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions