Skip to content

fix(tm3): skip umask-masked modes and report permissive umask values - #783

Open
ilaigold wants to merge 2 commits into
NVIDIA:mainfrom
ilaigold:fix/tm3-umask
Open

ilaigold wants to merge 2 commits into
NVIDIA:mainfrom
ilaigold:fix/tm3-umask

Conversation

@ilaigold

@ilaigold ilaigold commented Oct 7, 2026

Copy link
Copy Markdown

Refs #644 (item B6)

I was going through false positives from a --no-llm scan of my skills and looked at the TM3 permission pattern for #644 B6, where mode = 0o666 & ~umask gets flagged even though that's the umask-respecting default a plain open() uses. The same pattern also has umask backwards. A umask lists the bits to remove, so umask = 0o777 is the most restrictive value, and it gets flagged. Meanwhile umask 000 and os.umask(0), which make new files world-writable, don't.

Repro with skillspector scan <dir> --no-llm --format json: put mode = 0o666 & ~CURRENT_UMASK, umask = 0o777 and os.umask(0) in a Python script and umask 000 in a shell script. main @ 5485cda flags the first two, this branch flags the last two.

The one pattern is now three, with the same confidence and severity as before:

  • mode/permission 777 or 666: skipped when the rest of the expression is only & ~<name ending in umask> or a call like os.umask(...). Bits added after the mask (& ~umask | 0o777) or a mask that isn't the umask (& ~0) still get flagged.
  • umask assignment or shell command: flagged when the last octal digit leaves world-write unmasked (0, 1, 4 or 5). So 000 is flagged and 022, 077 and 0777 aren't.
  • umask(...) call: same rule for 0 or a leading-0 octal. A bare non-zero number in a call is decimal, so os.umask(18) (0o022) isn't flagged.

The one you'll probably ask about: an assigned call like old = os.umask(0) isn't flagged, because that's the standard way to read the current umask before restoring it (anthropics/claude-plugins-official does exactly this in plugins/code-modernization/scripts/compare.py). Code that creates files between that call and the restore would slip through, but main doesn't catch that either.

Detections: nothing world-writable that main catches is lost. The only lines that stop matching are 777/666 modes masked by the umask and umasks that already mask world-write. On 20,000 generated <key> = <value><suffix> lines, 7,392 lose TM3 and all of them fall into those two cases.

Tests: paired cases in tests/unit/test_patterns_new.py::TestToolMisuse, 13 malicious with exact matched_text and 11 benign (masked modes, restrictive umasks, the read-and-restore idiom in Python and JS).

ReDoS: no nested quantifiers over overlapping text, the lookbehinds are fixed-width, and the mask lookahead stays on one line. test_tm3_permission_patterns_are_linear runs 7 adversarial ~50k-character inputs with a 1 s limit each. Outside the suite, the new regexes took at most 0.01 s on 10 adversarial ~50k-character inputs and grew linearly up to 800k characters.

Checked on main @ 5485cda:

  • uv run pytest -m "not integration and not provider" tests/ -q: 10336 passed, 0 failed
  • uv run pytest -m "not integration and not provider" tests/unit/test_patterns_new.py -k "tm3 or TM3" -q: 38 passed, and 14 of them fail with only static_patterns_tool_misuse.py reverted to main
  • uv run make lint and uv run make format-check pass

🤖 Generated with Claude Code

The TM3 "Unsafe Defaults" permission pattern
`(?:mode|permission|umask)\s*=\s*(?:0?o?777|0?o?666)` reported
`mode = 0o666 & ~umask`, which is the umask-respecting default (NVIDIA#644,
item B6). It also read umask values backwards: `umask = 0o777`, the
most restrictive value, was reported, while `umask 000` and
`os.umask(0)`, which make new files world-writable, were not.

- mode/permission: a 777/666 mode followed only by `& ~<name>umask` (or
  `& ~<name>.umask(...)`) up to the end of the expression is skipped.
  Bits added after the mask (`& ~umask | 0o777`) and masks that are not
  the umask (`& ~0`) are still reported.
- umask assignment or shell command: report a value whose last octal
  digit leaves the world-write bit unmasked (0, 000, 0000, 004).
- umask(...) call: report a zero or leading-0 octal argument with the
  same rule, except when the result is assigned (`old = os.umask(0)`),
  which is how Python reads the current umask before restoring it.

Signed-off-by: Ilai Goldschmidt <117302862+ilaigold@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@yashrajp22 yashrajp22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usual masked-mode and restrictive-umask cases now behave as intended, but the four inline cases still need attention. The unsafe-expression and assignment-formatting cases regress detection, while CRLF leaves the B6 false positive unresolved.

Validation: 159 selected tests pass against HEAD in both source and fresh wheel installs. Completed 48 corpus scans and 40 focused scans across BASE b359c66 and HEAD 3eb3cd9, with source/wheel results agreeing. The expected 14 BASE failures confirm the intended changes. Main-only differences were assessed separately from the authored diff at 025c65e. Greptile feedback was adjudicated; the decimal case and additional multiline trigger are source-confirmed. These checks cover offline behavior, with existing partial-analysis limitations retained.

# like a plain open(), unless more bits are added after the mask.
(
r"(?:mode|permission)\s*=\s*(?:0?o?777|0?o?666)"
r"(?![ \t]*&[ \t]*~[ \t]*[\w.]*umask\b(?:\([^()\n]*\))?[ \t]*(?:[),;\]}#]|//|$))",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the whole expression before exempting a masked mode. In Python, // is floor division: with current_umask = 0o022, mode = 0o666 & ~current_umask // 512 evaluates to 0o666. Both source and wheel scans report TM3 on BASE but lose it on this branch.

The multiline $ also accepts a physical newline inside a continued call. Source inspection shows this expression is exempted even though the next line restores world-write:

os.chmod(path, mode=0o666 & ~umask
         | 0o666)

Please preserve detection when arithmetic or a continued expression changes the final mode.

# umask(0) as a statement or inside an expression. In a call, a number without a
# leading 0 is decimal. `old = os.umask(0)` is how Python reads the umask before
# restoring it, so a call whose result is assigned is not reported.
(r"(?<![\w.])(?<!=)(?<!= )(?:\w+\.)*umask\(\s*0(?:o?[0-7]*[0145])?\s*\)", 0.8),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The read-and-restore exemption changes with formatting. old = os.umask(0) and old = (os.umask(0)), each immediately followed by os.umask(old), now produce TM3 findings while the one-space form does not. A tab after = has the same problem. I confirmed this through both source and wheel scans. Please recognize these equivalent assignments so harmless formatting does not introduce a warning.

# like a plain open(), unless more bits are added after the mask.
(
r"(?:mode|permission)\s*=\s*(?:0?o?777|0?o?666)"
r"(?![ \t]*&[ \t]*~[ \t]*[\w.]*umask\b(?:\([^()\n]*\))?[ \t]*(?:[),;\]}#]|//|$))",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The masked-mode fix still flags mode = 0o666 & ~umask in a CRLF file, although the LF version is exempt. [ \t]* leaves the \r before the multiline $, and the CLI preserves those bytes. Both source and wheel scans reproduce the warning, so B6 remains unfixed for this common line ending. Please handle CRLF endings too.

),
# A umask whose last octal digit leaves the world-write bit (0o002) unmasked, such
# as 000, makes new files world-writable. A umask such as 0o777 is restrictive.
(r"umask(?:\s*=\s*|[ \t]+)(?:0o)?[0-7]*[0145]\b", 0.8),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bare Python numbers are decimal, so umask = 14 means 0o016 and masks the world-write bit. This pattern instead treats the trailing decimal 4 as an octal indicator and introduces a false TM3 finding. Confirmed by source inspection. Please interpret the literal according to its language and base before checking 0o002.

…ues by base

Review follow-ups for the TM3 umask patterns:

- mode/permission: `//` no longer ends the masked expression, because
  in Python it is floor division (`0o666 & ~current_umask // 512` is
  0o666). The end of a line, also after a # comment, ends it only when
  the next line does not start with an operator, `and` or `or`, so a
  call continued with `| 0o666` on the next line is reported. The
  comment is matched up to 240 characters to keep the check linear; a
  longer one is reported as before. A CRLF line ending is accepted the
  same way as LF, so `mode = 0o666 & ~umask` in a CRLF file is no
  longer reported.
- umask(...) call: the read-and-restore exemption skips spaces, tabs,
  line breaks and opening parentheses back to the `=`, so
  `old =  os.umask(0)` and `old = (os.umask(0))` behave like
  `old = os.umask(0)`. `==`, `!=`, `<=` and `>=` are comparisons and
  are still reported.
- umask values: outside shell files a number is octal only with a 0o
  prefix or a leading 0. A bare number is decimal, so `umask = 14`
  (0o016) is not reported and `umask = 8` (0o010) is, in assignments
  and calls alike. Shell files and the `umask NNN` command still read
  every value as octal.

Signed-off-by: Ilai Goldschmidt <117302862+ilaigold@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ilaigold

ilaigold commented Oct 7, 2026

Copy link
Copy Markdown
Author

Thanks for the careful review, all four were real. Fixed in a new commit on top of 3eb3cd9.

  1. Whole expression: // no longer ends the masked expression, so 0o666 & ~current_umask // 512 is reported again. A line end (also after a # comment) only ends it when the next line doesn't start with an operator, and or or, so your continued | 0o666 call is reported, while a mask followed by a closing ) on the next line is still skipped. Tests: floor_division_after_umask_mask, bits_added_on_continued_line, bits_added_after_comment_line, or_on_continued_line, mask_ends_before_closing_line.

  2. Read and restore: I replaced the fixed-width lookbehinds with a small helper that steps back over spaces, tabs, line breaks, \ and ( to the =, so the two-space, tab and parenthesized forms behave like the one-space one. ==, !=, <= and >= are comparisons and are still reported. Tests: read_umask_two_spaces, read_umask_tab, read_umask_parenthesized, umask_call_compared_not_assigned.

  3. CRLF: the line end now accepts \r\n, so mode = 0o666 & ~umask in a CRLF file is skipped like the LF one, and a CRLF line continued with | 0o666 is still reported. Tests: mode_masked_by_umask_crlf, mode_masked_by_umask_with_comment_crlf, bits_added_on_continued_crlf_line.

  4. Decimal: outside shell files a number is octal only with a 0o prefix or a leading 0, and a bare number is read as decimal. So umask = 14 (0o016) is no longer reported, while umask = 8 and os.umask(8) (0o010) are. .sh files and the umask NNN command still read every value as octal. Tests: decimal_14_is_0o016, decimal_8_is_0o010, decimal_call_0o010, shell_umask_is_octal, shell_variable_is_octal.

The new paths have linear-time cases next to the existing ones, including a long comment after the mask. The comment check stops at 240 characters, so a longer comment is reported like before.

Two things I left as they are. A JavaScript line like mode = 0o666 & ~process.umask() // note without a semicolon is reported again, because the same // is floor division in Python and the pattern can't tell the languages apart. With a semicolon it's still skipped. And a conditional continued on the next line (if safe else 0o777) still reads as the end of the mask, since line by line it looks like a following if statement, and reporting that would bring the B6 false positive back for ordinary code.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants