diff --git a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py index 0d687a77b..b648491a8 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py +++ b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py @@ -348,6 +348,29 @@ TM2_PATTERNS = TM2_CODE_PATTERNS + TM2_PROSE_PATTERNS # TM3: Unsafe Defaults — overly permissive default settings +# +# A umask leaves new files world-writable when its world-write bit (0o002) is unset. +# In octal digits that is a last digit of 0, 1, 4 or 5. +_UMASK_OCTAL_WORLD_WRITE = r"[0-7]*[0145]\b" +# Python, JavaScript and C read a number as octal only with a 0o prefix or a leading +# 0, so a bare 14 is 0o016 (world-write masked) and a bare 8 is 0o010 (unmasked). A +# decimal N leaves the bit unset when N % 4 is 0 or 1, which its last two digits decide. +_UMASK_NUMBER_WORLD_WRITE = ( + rf"(?:0(?:o?{_UMASK_OCTAL_WORLD_WRITE})?\b" + r"|[1-9]\d*+(?<=[02468][014589]|[13579][2367]|\D[14589])\b)" +) +# umask 000, or an assigned value such as umask = 0o000 or umask = 8. +_TM3_UMASK = ( + rf"umask(?:[ \t]+(?:0o)?{_UMASK_OCTAL_WORLD_WRITE}|\s*=\s*{_UMASK_NUMBER_WORLD_WRITE})", + 0.8, +) +# Shell reads every umask value as octal, including a variable such as UMASK=14 that +# is later passed to the umask command. +_TM3_SHELL_UMASK = (rf"umask(?:\s*=\s*|[ \t]+)(?:0o)?{_UMASK_OCTAL_WORLD_WRITE}", 0.8) +# umask(0) as a statement or inside an expression. `old = os.umask(0)` is how Python +# reads the umask before restoring it, so analyze() skips a call whose result is +# assigned (see _is_assigned_value). +_TM3_UMASK_CALL = (rf"(?=?:.]|(?:and|or)\b))))", + 0.8, + ), + _TM3_UMASK, + _TM3_UMASK_CALL, (r"world[_-]?(?:readable|writable|executable)", 0.7), # Debug/dev mode in production (r"(?:debug|dev|development)[_-]?mode\s*=\s*(?:True|true|1|on|yes|enable)", 0.6), @@ -388,6 +424,7 @@ ), ] TM3_PATTERNS = TM3_CODE_PATTERNS + TM3_PROSE_PATTERNS +_TM3_SHELL_PATTERNS = [_TM3_SHELL_UMASK if entry == _TM3_UMASK else entry for entry in TM3_PATTERNS] # TM4: Privileged Kubernetes Workload — manifest/CLI primitives that grant # node/host takeover (the cluster-scale counterpart of a privileged container). @@ -4978,6 +5015,21 @@ def _line_containing(content: str, start: int, end: int) -> str: return content[line_start:line_end] +def _is_assigned_value(content: str, start: int) -> bool: + """Return whether the expression at *start* is the value of an assignment. + + Whitespace, line continuations and opening parentheses after the ``=`` are + skipped, so ``old = (os.umask(0))`` reads like ``old = os.umask(0)``. + ``==``, ``!=``, ``<=`` and ``>=`` compare rather than assign. + """ + index = start + while index and content[index - 1] in " \t\r\n\\(": + index -= 1 + if not index or content[index - 1] != "=": + return False + return index < 2 or content[index - 2] not in "=!<>" + + def _bounded_context(content: str, match_start: int) -> str: """Return a fixed-size context centered on one direct shell call.""" left = max(0, match_start - _MAX_DIRECT_SHELL_CONTEXT_CHARS // 2) @@ -5266,13 +5318,16 @@ def ctx(start: int) -> str: complete_match=match.group(0), ) ) - for pattern, confidence in TM3_PATTERNS: + for pattern, confidence in _TM3_SHELL_PATTERNS if file_type == "shell" else TM3_PATTERNS: matches = ( static_runner.iter_paragraph_matches if (pattern, confidence) in TM3_PROSE_PATTERNS else re.finditer ) + skip_assigned = (pattern, confidence) == _TM3_UMASK_CALL for match in matches(pattern, content, re.IGNORECASE | re.MULTILINE): + if skip_assigned and _is_assigned_value(content, match.start()): + continue line_num = line_number(match.start()) findings.append( AnalyzerFinding( diff --git a/tests/unit/test_patterns_new.py b/tests/unit/test_patterns_new.py index c45afdb72..b6a5df4c5 100644 --- a/tests/unit/test_patterns_new.py +++ b/tests/unit/test_patterns_new.py @@ -1790,6 +1790,200 @@ def test_tm1_rm_outside_dockerfile_stays_high(self) -> None: def test_tm3_detected(self, content: str, filename: str, filetype: str) -> None: assert any(f.rule_id == "TM3" for f in tm_mod.analyze(content, filename, filetype)) + @pytest.mark.parametrize( + "content,filename,filetype,expected", + [ + pytest.param("mode = 0o777", "fs.py", "python", "mode = 0o777", id="mode_777"), + pytest.param( + "permission = 0o666", "fs.py", "python", "permission = 0o666", id="permission_666" + ), + pytest.param( + "mode = 0o777 & ~umask | 0o777", + "fs.py", + "python", + "mode = 0o777", + id="bits_added_after_umask_mask", + ), + pytest.param( + "mode = 0o777 & ~0", "fs.py", "python", "mode = 0o777", id="mask_is_not_the_umask" + ), + pytest.param("umask 000", "run.sh", "shell", "umask 000", id="shell_umask_000"), + pytest.param("UMASK=0000", "env.sh", "shell", "UMASK=0000", id="env_umask_0000"), + pytest.param("umask = 0o000", "cfg.py", "python", "umask = 0o000", id="assign_0o000"), + pytest.param("umask 004", "run.sh", "shell", "umask 004", id="world_write_unmasked"), + pytest.param( + "umask=0o7770", "cfg.py", "python", "umask=0o7770", id="long_value_ending_in_0" + ), + pytest.param("os.umask(0)", "fs.py", "python", "os.umask(0)", id="py_umask_call"), + pytest.param( + "process.umask(0o000);", "fs.js", "javascript", "process.umask(0o000)", id="js_call" + ), + pytest.param("umask(0);", "main.c", "c", "umask(0)", id="c_umask_call"), + pytest.param( + "fd = os.open(p, flags, mode=0o666 & ~os.umask(0))", + "fs.py", + "python", + "os.umask(0)", + id="umask_zero_inside_mask_expression", + ), + pytest.param( + "mode = 0o666 & ~current_umask // 512", + "fs.py", + "python", + "mode = 0o666", + id="floor_division_after_umask_mask", + ), + pytest.param( + "os.chmod(path, mode=0o666 & ~umask\n | 0o666)", + "fs.py", + "python", + "mode=0o666", + id="bits_added_on_continued_line", + ), + pytest.param( + "os.chmod(path, mode=0o666 & ~umask\r\n | 0o666)", + "fs.py", + "python", + "mode=0o666", + id="bits_added_on_continued_crlf_line", + ), + pytest.param( + "os.chmod(path, mode=0o666 & ~umask # keep the umask\n | 0o666)", + "fs.py", + "python", + "mode=0o666", + id="bits_added_after_comment_line", + ), + pytest.param( + "os.chmod(path, mode=0o666 & ~umask\n or 0o777)", + "fs.py", + "python", + "mode=0o666", + id="or_on_continued_line", + ), + pytest.param( + "changed = old == os.umask(0)", + "fs.py", + "python", + "os.umask(0)", + id="umask_call_compared_not_assigned", + ), + pytest.param("umask = 8", "cfg.py", "python", "umask = 8", id="decimal_8_is_0o010"), + pytest.param("os.umask(8)", "fs.py", "python", "os.umask(8)", id="decimal_call_0o010"), + pytest.param("umask 14", "run.sh", "shell", "umask 14", id="shell_umask_is_octal"), + pytest.param("UMASK=14", "env.sh", "shell", "UMASK=14", id="shell_variable_is_octal"), + ], + ) + def test_tm3_world_writable_mode_or_umask_detected( + self, content: str, filename: str, filetype: str, expected: str + ) -> None: + """A world-writable mode, or a umask that leaves world-write unmasked, is TM3.""" + findings = tm_mod.analyze(content, filename, filetype) + assert [f.matched_text for f in findings if f.rule_id == "TM3"] == [expected] + + @pytest.mark.parametrize( + "content,filename,filetype", + [ + pytest.param("mode = 0o666 & ~umask", "fs.py", "python", id="mode_masked_by_umask"), + pytest.param( + "os.chmod(path, mode=0o777 & ~current_umask)", + "fs.py", + "python", + id="mode_masked_in_call", + ), + pytest.param( + "const mode = 0o666 & ~process.umask();", + "fs.js", + "javascript", + id="mode_masked_by_process_umask", + ), + pytest.param("umask = 0o777", "cfg.py", "python", id="most_restrictive_umask"), + pytest.param("UMASK=0777", "env.sh", "shell", id="restrictive_env_umask"), + pytest.param("umask 077", "run.sh", "shell", id="umask_077"), + pytest.param("umask 022", "run.sh", "shell", id="umask_022"), + pytest.param("os.umask(0o022)", "fs.py", "python", id="umask_call_022"), + pytest.param("os.umask(18)", "fs.py", "python", id="decimal_umask_call_022"), + pytest.param( + "umask = os.umask(0)\nos.umask(umask)\nos.chmod(tmp, 0o666 & ~umask)", + "fs.py", + "python", + id="read_and_restore_umask", + ), + pytest.param( + "const previous = process.umask(0);", "fs.js", "javascript", id="js_read_umask" + ), + pytest.param( + "mode = 0o666 & ~umask\r\nfd = os.open(path, flags, mode)\r\n", + "fs.py", + "python", + id="mode_masked_by_umask_crlf", + ), + pytest.param( + "mode = 0o666 & ~umask # respect the umask\nfd = os.open(path, flags, mode)\n", + "fs.py", + "python", + id="mode_masked_by_umask_with_comment", + ), + pytest.param( + "mode = 0o666 & ~umask # respect the umask\r\nif exists:\r\n pass\r\n", + "fs.py", + "python", + id="mode_masked_by_umask_with_comment_crlf", + ), + pytest.param( + "os.chmod(path, mode=0o666 & ~umask\n )", + "fs.py", + "python", + id="mask_ends_before_closing_line", + ), + pytest.param( + "old = os.umask(0)\nos.umask(old)", "fs.py", "python", id="read_umask_two_spaces" + ), + pytest.param( + "old =\tos.umask(0)\nos.umask(old)", "fs.py", "python", id="read_umask_tab" + ), + pytest.param( + "old = (os.umask(0))\nos.umask(old)", + "fs.py", + "python", + id="read_umask_parenthesized", + ), + pytest.param("umask = 14", "cfg.py", "python", id="decimal_14_is_0o016"), + ], + ) + def test_tm3_umask_masked_mode_or_restrictive_umask_not_flagged( + self, content: str, filename: str, filetype: str + ) -> None: + """A mode masked with the umask and a restrictive umask are not unsafe defaults.""" + findings = tm_mod.analyze(content, filename, filetype) + assert not [f.matched_text for f in findings if f.rule_id == "TM3"] + + @pytest.mark.parametrize( + "content", + [ + pytest.param("mode=0o777 & ~" + "a" * 50_000, id="long_mask_operand"), + pytest.param("mode=0o777 &" + " " * 50_000 + "~umask |", id="long_gap_before_mask"), + pytest.param("mode=0o777 & ~os.umask(" + "0" * 50_000, id="unclosed_mask_call"), + pytest.param("umask " + "0" * 50_000 + "2", id="long_umask_value"), + pytest.param("umask(0" + "0" * 50_000, id="unclosed_umask_call"), + pytest.param("a." * 25_000 + "umask(", id="long_dotted_name"), + pytest.param("umask umask " * 4_000, id="repeated_umask_words"), + pytest.param("old =" + " " * 50_000 + "os.umask(0)", id="long_gap_after_assignment"), + pytest.param("(" * 50_000 + "os.umask(0)", id="long_paren_run_before_call"), + pytest.param("old = (os.umask(0))\n" * 10_000, id="repeated_assigned_umask_calls"), + pytest.param("umask = 1" + "0" * 50_000 + "4", id="long_decimal_umask"), + pytest.param( + "mode=0o777 & ~umask\n" + " " * 50_000 + "|", id="long_indent_on_continued_line" + ), + pytest.param("mode=0o777 & ~umask #" + "c" * 50_000, id="long_comment_after_mask"), + pytest.param("mode=0o777 & ~umask # c\n" * 5_000, id="repeated_commented_masks"), + ], + ) + def test_tm3_permission_patterns_are_linear(self, content: str) -> None: + started = time.perf_counter() + tm_mod.analyze(content, "fs.py", "python") + assert time.perf_counter() - started < 1.0 + @pytest.mark.parametrize( "content,filename,filetype", [