G
YouTube TikTok Games
Skip to content

migrate: close the remaining TOCTOU between the target check and LMDB's open #1128

Open

Description

@chappie-daemon

Follow-up to the P1 security comment on #1125 (greptile, comment id 4548318952), tracked here so the PR can merge.

What the comment asked for

  1. Existing target directories must belong to the running user.
  2. Their parent must not be swappable before the store opens.

What is already done

  • (1) was in the branch before the review ran: the uid comparison over the target and <target>/lmdb (2e1627f) and over the data file's own metadata (7c4e287). Pinned by a_target_owned_by_another_user_is_refused.
  • (2) was real and is in 0066049:
    • create_target creates the target 0o700 right before the open and re-runs the full check, so a directory another user planted while the source was being probed is refused rather than adopted.
    • refuse_swappable_ancestry refuses any ancestor that is group- or world-writable without the sticky bit. The question is mode, not uid: a rename takes write permission on the holding directory and no ownership of the entry.
    • Pinned by a_target_under_a_swappable_directory_is_refused (0777 refused, 01777 allowed) and a_missing_target_is_created_and_then_judged; the first was verified to fail with the new call removed.

What remains

The check is still path-based, so a window exists in principle between the last symlink_metadata and LMDB's open. Closing it properly needs a handle-based open:

  • open the target with openat(dirfd, comp, O_DIRECTORY | O_NOFOLLOW) component by component, or O_PATH the final directory and fstat it, then hand the descriptor to LMDB;
  • heed's Env::open takes a path, not a descriptor, so this needs either an upstream change or an EnvOpenOptions path that keeps the directory handle alive across the open.

Until then the practical exposure is limited to a target under a sticky and writable ancestor (the /tmp case), where another user can create entries but cannot rename or remove ones they do not own.

Also noted

Greptile's check on #1125 stays red at 3/5 confidence against the repository's 5/5 threshold, with no inline comment or review body naming a finding on the current head. That is its scoring, not an unreported defect.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions