Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions go/ql/lib/semmle/go/security/UncontrolledAllocationSize.qll
Original file line number Diff line number Diff line change
@@ -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

@owen-mc owen-mc Mar 7, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note that we have the Strconv::IntegerParser class, which covers 3 out of 4 of these. I assume ParseFloat is in this list because the float could later be cast to an integer?

I also note that there is an identical isAdditionalFlowStep for DivideByZero.ql. I don't think we should try to share the code, though, unless you can think of something very nice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How harmful would it be to make these models-as-data summaries (i.e. globally applicable)? I'd expect that queries not interested in numeric types already discard them in some way (even if implicitly by having incompatible sources/sinks), so the impact shouldn't be too high.

But if we did this I think it should be handled in a different PR to better analyze the impact.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that would be a good idea. If you control the input to one of those functions then you control the output.

node1 = cn.getArgument(0) and
node2 = cn.getResult(0)
)
}
}

/** Tracks taint flow for reasoning about uncontrolled allocation size issues. */
module Flow = TaintTracking::Global<Config>;
}
Original file line number Diff line number Diff line change
@@ -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
{ }
}
36 changes: 36 additions & 0 deletions go/ql/src/Security/CWE-770/UncontrolledAllocationSize.qhelp
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">

<qhelp>
<overview>
<p>Using untrusted input to allocate slices with the built-in <code>make</code> 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.</p>
</overview>

<recommendation>
<p>Implement a maximum allowed value for size allocations with the built-in <code>make</code>
function to prevent excessively large allocations.</p>
</recommendation>

<example>
<p>In the following example snippet, the <code>n</code> parameter is user-controlled.</p>
<p>If the external user provides an excessively large value, the application allocates a slice
of size <code>n</code> without further verification, potentially exhausting all the available
memory.</p>

<sample src="UncontrolledAllocationSizeBad.go" />

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

<sample src="UncontrolledAllocationSizeGood.go" />
</example>

<references>
<li> OWASP: <a
href="https://cheatsheetseries.owasp.org/cheatsheets/Denial_of_Service_Cheat_Sheet.html">Denial
of Service Cheat Sheet</a>
</li>
</references>
</qhelp>
21 changes: 21 additions & 0 deletions go/ql/src/Security/CWE-770/UncontrolledAllocationSize.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/**
* @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 error
* @security-severity 7.5
* @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"
Original file line number Diff line number Diff line change
@@ -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).
32 changes: 0 additions & 32 deletions go/ql/src/experimental/CWE-770/DenialOfService.qhelp

This file was deleted.

59 changes: 0 additions & 59 deletions go/ql/src/experimental/CWE-770/DenialOfService.ql

This file was deleted.

18 changes: 0 additions & 18 deletions go/ql/test/experimental/CWE-770/DenialOfService.expected

This file was deleted.

1 change: 0 additions & 1 deletion go/ql/test/experimental/CWE-770/DenialOfService.qlref

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import go
import semmle.go.security.UncontrolledAllocationSize
import TestUtilities.InlineFlowTest
import FlowTest<UncontrolledAllocationSize::Config, UncontrolledAllocationSize::Config>
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down