From 7d74125508aaf237af675369655dbce2a0ae1b3d Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 7 Mar 2024 15:17:49 +0100 Subject: [PATCH 1/4] Go: Promote go/uncontrolled-allocation-size --- .../security/UncontrolledAllocationSize.qll | 34 +++++++++++ ...controlledAllocationSizeCustomizations.qll | 33 +++++++++++ .../CWE-770/UncontrolledAllocationSize.qhelp | 36 +++++++++++ .../CWE-770/UncontrolledAllocationSize.ql | 22 +++++++ .../CWE-770/UncontrolledAllocationSizeBad.go} | 0 .../UncontrolledAllocationSizeGood.go} | 0 ...2024-03-07-uncontrolled-allocation-size.md | 4 ++ .../CWE-770/DenialOfService.qhelp | 32 ---------- .../experimental/CWE-770/DenialOfService.ql | 59 ------------------- .../CWE-770/DenialOfService.expected | 18 ------ .../CWE-770/DenialOfService.qlref | 1 - .../UncontrolledAllocationSize.expected | 0 .../CWE-770/UncontrolledAllocationSize.ql | 4 ++ .../CWE-770/UncontrolledAllocationSizeBad.go} | 2 +- .../UncontrolledAllocationSizeGood.go} | 0 15 files changed, 134 insertions(+), 111 deletions(-) create mode 100644 go/ql/lib/semmle/go/security/UncontrolledAllocationSize.qll create mode 100644 go/ql/lib/semmle/go/security/UncontrolledAllocationSizeCustomizations.qll create mode 100644 go/ql/src/Security/CWE-770/UncontrolledAllocationSize.qhelp create mode 100644 go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql rename go/ql/src/{experimental/CWE-770/DenialOfServiceBad.go => Security/CWE-770/UncontrolledAllocationSizeBad.go} (100%) rename go/ql/src/{experimental/CWE-770/DenialOfServiceGood.go => Security/CWE-770/UncontrolledAllocationSizeGood.go} (100%) create mode 100644 go/ql/src/change-notes/2024-03-07-uncontrolled-allocation-size.md delete mode 100644 go/ql/src/experimental/CWE-770/DenialOfService.qhelp delete mode 100644 go/ql/src/experimental/CWE-770/DenialOfService.ql delete mode 100644 go/ql/test/experimental/CWE-770/DenialOfService.expected delete mode 100644 go/ql/test/experimental/CWE-770/DenialOfService.qlref create mode 100644 go/ql/test/query-tests/Security/CWE-770/UncontrolledAllocationSize.expected create mode 100644 go/ql/test/query-tests/Security/CWE-770/UncontrolledAllocationSize.ql rename go/ql/test/{experimental/CWE-770/DenialOfServiceBad.go => query-tests/Security/CWE-770/UncontrolledAllocationSizeBad.go} (89%) rename go/ql/test/{experimental/CWE-770/DenialOfServiceGood.go => query-tests/Security/CWE-770/UncontrolledAllocationSizeGood.go} (100%) diff --git a/go/ql/lib/semmle/go/security/UncontrolledAllocationSize.qll b/go/ql/lib/semmle/go/security/UncontrolledAllocationSize.qll new file mode 100644 index 00000000000..885aa7a7053 --- /dev/null +++ b/go/ql/lib/semmle/go/security/UncontrolledAllocationSize.qll @@ -0,0 +1,34 @@ +/** + * Provides a taint-tracking configuration for reasoning about uncontrolled allocation size issues. + */ + +import go + +/** + * Provides a taint-tracking flow for reasoning about uncontrolled allocation size issues. + */ +module UncontrolledAllocationSize { + private import UncontrolledAllocationSizeCustomizations::UncontrolledAllocationSize + + /** + * Module for defining predicates and tracking taint flow related to uncontrolled allocation size issues. + */ + module Config implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof Source } + + predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer } + + predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(Function f, DataFlow::CallNode cn | cn = f.getACall() | + f.hasQualifiedName("strconv", ["Atoi", "ParseInt", "ParseUint", "ParseFloat"]) and + node1 = cn.getArgument(0) and + node2 = cn.getResult(0) + ) + } + } + + /** Tracks taint flow for reasoning about uncontrolled allocation size issues. */ + module Flow = TaintTracking::Global; +} diff --git a/go/ql/lib/semmle/go/security/UncontrolledAllocationSizeCustomizations.qll b/go/ql/lib/semmle/go/security/UncontrolledAllocationSizeCustomizations.qll new file mode 100644 index 00000000000..1237971dde1 --- /dev/null +++ b/go/ql/lib/semmle/go/security/UncontrolledAllocationSizeCustomizations.qll @@ -0,0 +1,33 @@ +/** + * Provides default sources, sinks, and sanitizers for reasoning about uncontrolled allocation size issues, + * as well as extension points for adding your own. + */ + +import go +private import semmle.go.security.AllocationSizeOverflow + +/** + * Provides extension points for customizing the taint-tracking configuration for reasoning + * about uncontrolled allocation size issues. + */ +module UncontrolledAllocationSize { + /** A data flow source for uncontrolled allocation size vulnerabilities. */ + abstract class Source extends DataFlow::Node { } + + /** A data flow sink for uncontrolled allocation size vulnerabilities. */ + abstract class Sink extends DataFlow::Node { } + + /** A sanitizer for uncontrolled allocation size vulnerabilities. */ + abstract class Sanitizer extends DataFlow::Node { } + + /** A source of untrusted data, considered as a taint source for uncontrolled size allocation vulnerabilities. */ + private class UntrustedFlowAsSource extends Source instanceof UntrustedFlowSource { } + + /** The size argument of a memory allocation function. */ + private class AllocationSizeAsSink extends Sink instanceof AllocationSizeOverflow::AllocationSize { + } + + /** A check that a value is below some upper limit. */ + private class SizeCheckSanitizer extends Sanitizer instanceof AllocationSizeOverflow::AllocationSizeCheckBarrier + { } +} diff --git a/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.qhelp b/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.qhelp new file mode 100644 index 00000000000..b4029e93e1e --- /dev/null +++ b/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.qhelp @@ -0,0 +1,36 @@ + + + + +

Using untrusted input to allocate slices with the built-in make function could + lead to excessive memory allocation and potentially cause the program to crash due to running + out of memory. This vulnerability could be exploited to perform a denial-of-service attack by + consuming all available server resources.

+ + + +

Implement a maximum allowed value for size allocations with the built-in make + function to prevent excessively large allocations.

+ + + +

In the following example snippet, the n parameter is user-controlled.

+

If the external user provides an excessively large value, the application allocates a slice + of size n without further verification, potentially exhausting all the available + memory.

+ + + +

One way to prevent this vulnerability is by implementing a maximum allowed value for the + user-controlled input, as seen in the following example:

+ + + + + +
  • OWASP: Denial + of Service Cheat Sheet +
    • +     +     \ No newline at end of file diff --git a/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql b/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql new file mode 100644 index 00000000000..2be09c6901b --- /dev/null +++ b/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql @@ -0,0 +1,22 @@ +/** + * @name Slice memory allocation with excessive size value + * @description Allocating memory for slices with the built-in make function from user-controlled sources + * can lead to a denial of service. + * @kind path-problem + * @problem.severity warning + * @security-severity 6.0 + * @precision high + * @id go/uncontrolled-allocation-size + * @tags security + * external/cwe/cwe-770 + */ + +import go +import semmle.go.security.UncontrolledAllocationSize +import UncontrolledAllocationSize::Flow::PathGraph + +from + UncontrolledAllocationSize::Flow::PathNode source, UncontrolledAllocationSize::Flow::PathNode sink +where UncontrolledAllocationSize::Flow::flowPath(source, sink) +select sink, source, sink, "This memory allocation depends on a $@.", source.getNode(), + "user-provided value" diff --git a/go/ql/src/experimental/CWE-770/DenialOfServiceBad.go b/go/ql/src/Security/CWE-770/UncontrolledAllocationSizeBad.go similarity index 100% rename from go/ql/src/experimental/CWE-770/DenialOfServiceBad.go rename to go/ql/src/Security/CWE-770/UncontrolledAllocationSizeBad.go diff --git a/go/ql/src/experimental/CWE-770/DenialOfServiceGood.go b/go/ql/src/Security/CWE-770/UncontrolledAllocationSizeGood.go similarity index 100% rename from go/ql/src/experimental/CWE-770/DenialOfServiceGood.go rename to go/ql/src/Security/CWE-770/UncontrolledAllocationSizeGood.go diff --git a/go/ql/src/change-notes/2024-03-07-uncontrolled-allocation-size.md b/go/ql/src/change-notes/2024-03-07-uncontrolled-allocation-size.md new file mode 100644 index 00000000000..663932005eb --- /dev/null +++ b/go/ql/src/change-notes/2024-03-07-uncontrolled-allocation-size.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* The query "Slice memory allocation with excessive size value" (`go/uncontrolled-allocation-size`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @Malayke](https://github.com/github/codeql/pull/15130). diff --git a/go/ql/src/experimental/CWE-770/DenialOfService.qhelp b/go/ql/src/experimental/CWE-770/DenialOfService.qhelp deleted file mode 100644 index b91f1f7e3b0..00000000000 --- a/go/ql/src/experimental/CWE-770/DenialOfService.qhelp +++ /dev/null @@ -1,32 +0,0 @@ - - - - -

    Using untrusted input to created with the built-in make function - could lead to excessive memory allocation and potentially cause the program to crash due - to running out of memory. This vulnerability could be exploited to perform a DoS attack by consuming all available server resources.

    -     - - -

    Implement a maximum allowed value for creates a slice with the built-in make function to prevent excessively large allocations. - For instance, you could restrict it to a reasonable upper limit.

    -     - - -

    In the following example snippet, the n field is user-controlled.

    -

    The server trusts that n has an acceptable value, however when using a maliciously large value, - it allocates a slice of n of strings before filling the slice with data.

    - - - -

    One way to prevent this vulnerability is by implementing a maximum allowed value for the user-controlled input:

    - - -     - - -
  • - OWASP: Denial of Service Cheat Sheet -
    • -     -     \ No newline at end of file diff --git a/go/ql/src/experimental/CWE-770/DenialOfService.ql b/go/ql/src/experimental/CWE-770/DenialOfService.ql deleted file mode 100644 index 199cd0df552..00000000000 --- a/go/ql/src/experimental/CWE-770/DenialOfService.ql +++ /dev/null @@ -1,59 +0,0 @@ -/** - * @name Denial Of Service - * @description slices created with the built-in make function from user-controlled sources using a - * maliciously large value possibly leading to a denial of service. - * @kind path-problem - * @problem.severity error - * @security-severity 9 - * @precision high - * @id go/denial-of-service - * @tags security - * experimental - * external/cwe/cwe-770 - */ - -import go - -/** - * Holds if the guard `g` on its branch `branch` checks that `e` is not constant and is less than some other value. - */ -predicate denialOfServiceSanitizerGuard(DataFlow::Node g, Expr e, boolean branch) { - exists(DataFlow::Node lesser | - e = lesser.asExpr() and - g.(DataFlow::RelationalComparisonNode).leq(branch, lesser, _, _) and - not e.isConst() - ) -} - -/** - * Module for defining predicates and tracking taint flow related to denial of service issues. - */ -module Config implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource } - - predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(Function f, DataFlow::CallNode cn | cn = f.getACall() | - f.hasQualifiedName("strconv", ["Atoi", "ParseInt", "ParseUint", "ParseFloat"]) and - node1 = cn.getArgument(0) and - node2 = cn.getResult(0) - ) - } - - predicate isBarrier(DataFlow::Node node) { - node = DataFlow::BarrierGuard::getABarrierNode() - } - - predicate isSink(DataFlow::Node sink) { sink = Builtin::make().getACall().getArgument(0) } -} - -/** - * Tracks taint flow for reasoning about denial of service, where source is - * user-controlled and unchecked. - */ -module Flow = TaintTracking::Global; - -import Flow::PathGraph - -from Flow::PathNode source, Flow::PathNode sink -where Flow::flowPath(source, sink) -select sink, source, sink, "This variable might be leading to denial of service." diff --git a/go/ql/test/experimental/CWE-770/DenialOfService.expected b/go/ql/test/experimental/CWE-770/DenialOfService.expected deleted file mode 100644 index 4a2ae9d6646..00000000000 --- a/go/ql/test/experimental/CWE-770/DenialOfService.expected +++ /dev/null @@ -1,18 +0,0 @@ -edges -| DenialOfServiceBad.go:11:12:11:16 | selection of URL | DenialOfServiceBad.go:11:12:11:24 | call to Query | provenance | | -| DenialOfServiceBad.go:11:12:11:24 | call to Query | DenialOfServiceBad.go:13:15:13:20 | source | provenance | | -| DenialOfServiceBad.go:13:15:13:20 | source | DenialOfServiceBad.go:13:15:13:29 | call to Get | provenance | | -| DenialOfServiceBad.go:13:15:13:29 | call to Get | DenialOfServiceBad.go:14:28:14:36 | sourceStr | provenance | | -| DenialOfServiceBad.go:14:2:14:37 | ... := ...[0] | DenialOfServiceBad.go:20:27:20:30 | sink | provenance | | -| DenialOfServiceBad.go:14:28:14:36 | sourceStr | DenialOfServiceBad.go:14:2:14:37 | ... := ...[0] | provenance | | -nodes -| DenialOfServiceBad.go:11:12:11:16 | selection of URL | semmle.label | selection of URL | -| DenialOfServiceBad.go:11:12:11:24 | call to Query | semmle.label | call to Query | -| DenialOfServiceBad.go:13:15:13:20 | source | semmle.label | source | -| DenialOfServiceBad.go:13:15:13:29 | call to Get | semmle.label | call to Get | -| DenialOfServiceBad.go:14:2:14:37 | ... := ...[0] | semmle.label | ... := ...[0] | -| DenialOfServiceBad.go:14:28:14:36 | sourceStr | semmle.label | sourceStr | -| DenialOfServiceBad.go:20:27:20:30 | sink | semmle.label | sink | -subpaths -#select -| DenialOfServiceBad.go:20:27:20:30 | sink | DenialOfServiceBad.go:11:12:11:16 | selection of URL | DenialOfServiceBad.go:20:27:20:30 | sink | This variable might be leading to denial of service. | diff --git a/go/ql/test/experimental/CWE-770/DenialOfService.qlref b/go/ql/test/experimental/CWE-770/DenialOfService.qlref deleted file mode 100644 index e5896bb61df..00000000000 --- a/go/ql/test/experimental/CWE-770/DenialOfService.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/CWE-770/DenialOfService.ql \ No newline at end of file diff --git a/go/ql/test/query-tests/Security/CWE-770/UncontrolledAllocationSize.expected b/go/ql/test/query-tests/Security/CWE-770/UncontrolledAllocationSize.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/go/ql/test/query-tests/Security/CWE-770/UncontrolledAllocationSize.ql b/go/ql/test/query-tests/Security/CWE-770/UncontrolledAllocationSize.ql new file mode 100644 index 00000000000..18add3a4881 --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-770/UncontrolledAllocationSize.ql @@ -0,0 +1,4 @@ +import go +import semmle.go.security.UncontrolledAllocationSize +import TestUtilities.InlineFlowTest +import FlowTest diff --git a/go/ql/test/experimental/CWE-770/DenialOfServiceBad.go b/go/ql/test/query-tests/Security/CWE-770/UncontrolledAllocationSizeBad.go similarity index 89% rename from go/ql/test/experimental/CWE-770/DenialOfServiceBad.go rename to go/ql/test/query-tests/Security/CWE-770/UncontrolledAllocationSizeBad.go index 2d61cdbdafc..0ae70436bde 100644 --- a/go/ql/test/experimental/CWE-770/DenialOfServiceBad.go +++ b/go/ql/test/query-tests/Security/CWE-770/UncontrolledAllocationSizeBad.go @@ -17,7 +17,7 @@ func OutOfMemoryBad(w http.ResponseWriter, r *http.Request) { return } - result := make([]string, sink) + result := make([]string, sink) // $hasTaintFlow="sink" for i := 0; i < sink; i++ { result[i] = fmt.Sprintf("Item %d", i+1) } diff --git a/go/ql/test/experimental/CWE-770/DenialOfServiceGood.go b/go/ql/test/query-tests/Security/CWE-770/UncontrolledAllocationSizeGood.go similarity index 100% rename from go/ql/test/experimental/CWE-770/DenialOfServiceGood.go rename to go/ql/test/query-tests/Security/CWE-770/UncontrolledAllocationSizeGood.go From 138ce42cf6f4f65c04e829448831bef548eeaec4 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 7 Mar 2024 15:22:46 +0100 Subject: [PATCH 2/4] Fix qhelp --- go/ql/src/Security/CWE-770/UncontrolledAllocationSize.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.qhelp b/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.qhelp index b4029e93e1e..14930944bb6 100644 --- a/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.qhelp +++ b/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.qhelp @@ -19,12 +19,12 @@ of size n without further verification, potentially exhausting all the available memory.

    - +

    One way to prevent this vulnerability is by implementing a maximum allowed value for the user-controlled input, as seen in the following example:

    - + From a09eb9f4c570184a8d899529bb558c5ef89cb888 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 11 Mar 2024 08:58:59 +0100 Subject: [PATCH 3/4] Update go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com> --- go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql b/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql index 2be09c6901b..c6d2091cc53 100644 --- a/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql +++ b/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql @@ -1,7 +1,6 @@ /** * @name Slice memory allocation with excessive size value - * @description Allocating memory for slices with the built-in make function from user-controlled sources - * can lead to a denial of service. + * @description Allocating memory for slices with the built-in make function from user-controlled sources can lead to a denial of service. * @kind path-problem * @problem.severity warning * @security-severity 6.0 From ff2d78d2c8bc15bd36bf24ae204b444ba9feb30d Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 11 Mar 2024 15:53:40 +0100 Subject: [PATCH 4/4] Update go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql --- go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql b/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql index c6d2091cc53..eabfa3333ec 100644 --- a/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql +++ b/go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql @@ -2,8 +2,8 @@ * @name Slice memory allocation with excessive size value * @description Allocating memory for slices with the built-in make function from user-controlled sources can lead to a denial of service. * @kind path-problem - * @problem.severity warning - * @security-severity 6.0 + * @problem.severity error + * @security-severity 7.5 * @precision high * @id go/uncontrolled-allocation-size * @tags security