From 612be64ffcaa276f659bc1a4d98005d90a3e19f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Thu, 16 May 2024 16:10:26 +0200 Subject: [PATCH] Consider actor and association checks as bypassable checks ONLY for issueOps --- .../CWE-367/UntrustedCheckoutTOCTOUCritical.ql | 10 ++++++++-- ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql | 10 ++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUCritical.ql b/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUCritical.ql index a3fcc9e0403..b7b8a3cf956 100644 --- a/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUCritical.ql +++ b/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUCritical.ql @@ -15,11 +15,17 @@ import actions import codeql.actions.security.UntrustedCheckoutQuery import codeql.actions.security.PoisonableSteps -from LabelControlCheck check, MutableRefCheckoutStep checkout +from ControlCheck check, MutableRefCheckoutStep checkout where // the mutable checkout step is protected by an access check check = [checkout.getIf(), checkout.getEnclosingJob().getIf()] and // the checked-out code may lead to arbitrary code execution - checkout.getAFollowingStep() instanceof PoisonableStep + checkout.getAFollowingStep() instanceof PoisonableStep and + ( + check instanceof LabelControlCheck + or + (check instanceof AssociationControlCheck or check instanceof ActorControlCheck) and + check.getEnclosingJob().getATriggerEvent().getName().matches("%_comment") + ) select checkout, "The checked-out code can be changed after the authorization check o step $@.", check, check.toString() diff --git a/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql b/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql index 562fc0809b7..65887922231 100644 --- a/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql +++ b/ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql @@ -15,11 +15,17 @@ import actions import codeql.actions.security.UntrustedCheckoutQuery import codeql.actions.security.PoisonableSteps -from LabelControlCheck check, MutableRefCheckoutStep checkout +from ControlCheck check, MutableRefCheckoutStep checkout where // the mutable checkout step is protected by an access check check = [checkout.getIf(), checkout.getEnclosingJob().getIf()] and // there are no evidences that the checked-out code can lead to arbitrary code execution - not checkout.getAFollowingStep() instanceof PoisonableStep + not checkout.getAFollowingStep() instanceof PoisonableStep and + ( + check instanceof LabelControlCheck + or + (check instanceof AssociationControlCheck or check instanceof ActorControlCheck) and + check.getEnclosingJob().getATriggerEvent().getName().matches("%_comment") + ) select checkout, "The checked-out code can be changed after the authorization check o step $@.", check, check.toString()