False Positives & False Negatives (Detection Logic & Rule Engine Flaws)

Open
  • Is something wrong?
#247 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
5/5
Estimated time
Over a week
Newbie friendliness
25/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Active
Tech stack
google-cloud, python
Domain
cloud, security

Research direction

Start by reading the three named scripts: the Cloud Armor migration validator, the YARA-L rule validator, and the IAM troubleshooting helper. Trace each reported check against its surrounding logic and existing tests; the issue describes three separate detection flaws across different tools. Done means correcting each behavior and adding coverage for the reported examples, including valid CEL attributes, CIDR lists without “cidr” in the name, and least-privilege role ordering.

Written by the indexing model from the issue text.

Description

False Positives & False Negatives (Detection Logic & Rule Engine Flaws)
A. False Positive in Cloud Armor CRS 4.22 Migration Validator
  • Finding Category: False Positive (Erroneous Detection)
  • Affected File: skills/cloud/cloud-armor-crs-rule-upgrader/scripts/upgrade_policy.py (Lines 475–480, 696)
Description

The Cloud Armor OWASP ruleset migration tool checks for dangling or deprecated signatures using the following regular expression:

_RE_SIGNATURE_NUMBER = re.compile(r'\bid(\d+)\b', re.IGNORECASE)

In line 696, check_dangling_signature_id(new_expr) executes this pattern against the entire transformed CEL expression string:

if check_dangling_signature_id(new_expr):
  dangling_id_detected = True
Impact
  • False Flagging of Valid Rules: If a custom Common Expression Language (CEL) rule evaluates HTTP attributes or custom query parameters containing id<digits> (e.g., request.headers['id99999'] or inIpRange(origin.ip, '10.0.0.0/8')), the parser treats the digits as an OWASP CRS signature number.
  • Premature Upgrade Blocking: Because the extracted identifier does not belong to the CRS 4.22 catalog, the script marks dangling_id_detected = True, generating false warning logs and incorrectly advising operators to purge valid rule conditions.
Recommended Remediation

Scope the signature check strictly to arguments inside evaluatePreconfiguredWaf() calls rather than evaluating the entire expression string.

B. False Negative in Google SecOps YARA-L 2.0 Rule Validator
  • Finding Category: False Negative (Missed Detection)
  • Affected File: skills/cloud/secops-detection-engineering/scripts/validate_yaral_rule.py (Lines 92–94)
Description

The YARA-L validator enforces syntax rules for CIDR Reference Lists, where IP comparison against CIDR lists requires the explicit in cidr operator. The validation pattern is defined as:

CIDR_BARE_LIST_RE = re.compile(
    r"\.\s*(?:ip|nat_ip)\s+in\s+%[A-Za-z0-9_]*cidr[A-Za-z0-9_]*\b"
)
Impact
  • Incomplete Syntax Enforcement: The validator relies solely on the string "cidr" being present in the reference list name (%...cidr...).
  • Compiler Failures in Production: If a security analyst uses standard enterprise list names that omit the literal word "cidr" (e.g., $e.src.ip in %corp_internal_subnets or %rfc1918_networks), the validator fails to detect the missing cidr modifier. The rule passes local linting but fails abruptly upon submission to the Google SecOps detection engine compiler.
Recommended Remediation

Track declared reference list types semantically or parse list metadata definitions instead of relying on naming conventions.

C. False Positive / Least Privilege Inversion in IAM Troubleshooting Helper
  • Finding Category: False Positive / Flawed Logic (Privilege Inversion)
  • Affected File: skills/cloud/iam-helper-for-troubleshooting/scripts/least_privileged_role.py (Lines 109–132)
Description

The utility find_least_privileged_role() is designed to identify the role with the fewest permissions that satisfies a specific API operation. However, the candidate role sorting key is defined as:

valid_roles.sort(
    key=lambda x: (
        not x["is_service_level_standard"],
        not x["is_predefined"],
        x["perm_count"],
    )
)

Where is_service_level_standard explicitly checks for roles ending in administrative suffixes:

is_service_level_standard = role_name.startswith("roles/") and (
    role_name_lower.endswith("admin")
    or role_name_lower.endswith("editor")
    or role_name_lower.endswith("viewer")
    or role_name_lower.endswith("reader")
    or role_name_lower.endswith("writer")
)
Impact
  • Recommending Administrative Roles as "Least Privileged": Because not x["is_service_level_standard"] is evaluated before x["perm_count"], any standard service role ending in admin (e.g., roles/storage.admin with dozens of destructive permissions) is sorted ahead of a tightly scoped predefined role (e.g., roles/storage.objectViewer or custom task-specific roles) that lacks the "admin" suffix.
  • Misleading Security Recommendations: Users following the script's recommendations under the assumption of implementing least privilege will inadvertently assign broad administrative capabilities to workloads and users.
Recommended Remediation

Prioritize raw perm_count ascending first, and penalize administrative role suffixes (admin, editor) rather than boosting them in the least-privilege calculation.

Dominant language
Python
Stars
23.1k
Forks
1.9k
No merged PRs in 30d

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from google/skills

All issues in google/skills

Similar issues

More Python issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.