Merge pull request #37 from frappe/security_rules

feat: Security rules

Author: ankush

rules/security.yml

@@ -0,0 +1,12 @@
+rules:
+- id: relaxed-permissions
+  patterns:
+  - pattern: |
+      {
+        "role": "All",
+        ...
+      }
+  message: |
+    Avoid using "All" role. It's available to every user, including website user.
+  languages: [json]
+  severity: WARNING

rules/security/authorization.yml

@@ -1,6 +1,3 @@
 def function_name(input):
 	# ruleid: frappe-codeinjection-eval
 	eval(input)
-
-# ok: frappe-codeinjection-eval
-eval("1 + 1")

rules/security.py rules/security/rce.pyrules/security.py renamed to rules/security/rce.py

@@ -0,0 +1,22 @@
+rules:
+- id: frappe-codeinjection-eval
+  patterns:
+  - pattern-either:
+    - pattern: eval(...)
+    - pattern: exec(...)
+    - pattern: safe_exec(...)
+    - pattern: safe_eval(...)
+  message: |
+    Detected the use of functions that can be  dangerous if used to evaluate
+    dynamic content. This code should be manually audited by security team.
+  languages: [python]
+  severity: ERROR
+
+- id: frappe-ssti
+  patterns:
+  - pattern: render_template($ARG, ...)
+  message: |
+    Detected the use of render_template, make sure $ARG comes from trusted
+    source. This code should be audited by security team.
+  languages: [python]
+  severity: ERROR

rules/security/rce.yml

@@ -0,0 +1,22 @@
+rules:
+  - id: frappe-sql-format-injection
+    languages: [python]
+    severity: INFO
+    message: >-
+      Detected use of '.format()' or f-string in a Frappe SQL call.
+      This can lead to SQL injection. Use parameterized queries instead.
+    patterns:
+      - pattern-either:
+          - pattern: frappe.db.sql("...".format(...), ...)
+          - pattern: frappe.db.sql(f"...", ...)
+          - pattern: frappe.db.multisql("...".format(...), ...)
+          - pattern: frappe.db.multisql(f"...", ...)
+          # Matches cases where the string is formatted beforehand
+          - pattern: |
+              $QUERY = "...".format(...)
+              ...
+              frappe.db.sql($QUERY, ...)
+          - pattern: |
+              $QUERY = f"..."
+              ...
+              frappe.db.sql($QUERY, ...)

rules/security/sql.yml

@@ -0,0 +1,30 @@
+from typing import Any
+import frappe
+
+
+
+# ruleid: missing-argument-type-hint
+@frappe.whitelist()
+def function_name(inject, abc):
+	pass
+
+# ok: missing-argument-type-hint
+@frappe.whitelist()
+def function_name(inject: str, abc: str):
+	pass
+
+# ok: missing-argument-type-hint
+@frappe.whitelist()
+def function_name():
+	pass
+
+# ok: missing-argument-type-hint
+@frappe.whitelist()
+def function_name(abc: Any):
+	pass
+
+
+# ruleid: missing-argument-type-hint
+@frappe.whitelist()
+def function_name(inject, abc: str): # only one typed
+	pass

rules/security/whitelisted.py

@@ -0,0 +1,24 @@
+rules:
+  - id: missing-argument-type-hint
+    languages: [python]
+    severity: WARNING
+    patterns:
+      - pattern: |
+          @frappe.whitelist(...)
+          def $FUNC(..., $ARG, ...): ...
+      - pattern-not: |
+          @frappe.whitelist(...)
+          def $FUNC(..., $ARG: $TYPE, ...): ...
+      - metavariable-regex:
+          metavariable: $ARG
+          # Exclude 'self' and 'cls' as they typically don't take hints
+          regex: ^(?!self$|cls$).*$
+    message: "The argument '$ARG' in function '$FUNC' is missing a type hint."
+  - id: guest-whitelisted-method
+    languages: [python]
+    severity: WARNING
+    patterns:
+      - pattern: |
+          @frappe.whitelist(..., allow_guest=True, ...)
+          def $FUNC(...): ...
+    message: "Whitelisted method that's accesible to guest should be manually reviewed by security team."