Go: add memoryAllocationDos query

[blue-bird1]

Try adding a new query. wip
I did not find a user-controlled int stream, and used another library sinker

[blue-bird1]

Fix review problem, but i can't make sanitizer to sanitizer guard. Please help me to get an example,

[blue-bird1]

@smowton I try

private predicate compCheckGuard(DataFlow::Node g, Expr e, boolean outcome) {
  e = g.(DataFlow::RelationalComparisonNode).getAnOperand().asExpr() and
  outcome = [true, false]
}


class CompSanitizer2 extends Sanitizer {
  CompSanitizer2() {
    this = DataFlow::BarrierGuard<compCheckGuard/3>::getABarrierNode()
  }
}

Still got an error report,

I try debug

ControlFlow::Node getABarrierNode() {
  exists(ControlFlow::ConditionGuardNode guard, SsaWithFields var | result = var.getAUse().getBasicBlock().getANode() |
    guards(_, guard, _, var) 
  )
}

ControlFlow::Node getABarrierNode2() {
  exists(ControlFlow::ConditionGuardNode guard, SsaWithFields var | result = guard |
    guards(_, guard, _, var) 
  )
}

ControlFlow::Node getABarrierNode3() {
  exists(ControlFlow::ConditionGuardNode guard, SsaWithFields var | result = var.getAUse().getBasicBlock() |
    guards(_, guard, _, var) 
  )
}

predicate dominates(ControlFlow::ConditionGuardNode guard, ReachableBasicBlock bb){
  guard = bb.getANode() or
  dominates(guard,bb.getImmediateDominator())
}

after quick query , I found that there is no _ = make([]int, c) in var.getAUse().getBasicBlock().getANode().
so BarrierNode not work in _ = make([]int, c)

[blue-bird1]

ok, I used some tricks. It works, but I'm not sure if it's the right way to do it

[owen-mc]

Please can you add some tests? You can look at other experimental queries to see how they've done it. You can start with the two example .go files here.

[smowton]

Looking at that unit test, the reason BarrierGuard didn't work is because there are 3 SSA definitions here: c = Atoi(...), c = 200 and the point where they merge. Hence it is necessary to use similar() or the like.

The test as expressed at the moment could be simplified like -- this sanitizes all uses of a given variable source when one is subject to a relational test.

private SsaWithFields getAComparedVar() {
  result.getAUse() = any(DataFlow::RelationalComparisonNode n).getAnOperand()
}

class CompSanitizer2 extends Sanitizer {
  CompSanitizer2() {
    this = getAComparedVar().similar().getAUse()
  }
}

If you wanted to be more ambitious, you could look at only uses that fall downstream of the checked use in the control-flow graph:

private DataFlow::Node getAComparedNode() {
  result = any(DataFlow::RelationalComparisonNode n).getAnOperand()
}

class CompSanitizer2 extends Sanitizer {
  CompSanitizer2() {
    exists(DataFlow::Node comparedNode, SsaWithFields var |
      comparedNode = getAComparedNode() and comparedNode = var.getAUse()
    |
      this.asInstruction() = comparedNode.asInstruction().getASuccessor+() and
      this = var.similar().getAUse()
    )
  }
}

Finally you could go as far as distinguishing the less-than from the greater-than arm of the if test and sanitize phi nodes where all inputs are either coming from the less-than side, or draw from a different definition (we hope, have been overwritten with a lower / constant value as in your unit test) -- but this might be too ambitious / difficult to explain to users to be worth the effort.

The tests that I requested were added

[github-actions]

QHelp previews:

go/ql/src/experimental/CWE-20/MemoryAllocationDos.qhelp

User-controlled size of a memory allocation operation

Memory allocation size from user-controlled data can allow an attacker to uses a lot of server memory. This can result the server is unavailable or crash due to failure to allocate huge memory.

References

• OWASP: Denial_of_Service.

[blue-bird1]

What are Cannot find UnexpectedFrontendErrors.expected file.? I run codeql test run . , test is passed.

[smowton]

Under CI the tests run with some consistency queries active, which check for some kinds of mistake against the test database. UnexpectedFrontendErrors reports on errors reported by the Go compiler -- you should be able to see them using go build *.go in the test directory.

[blue-bird1]

For my most recent bug hunter, make some improvements

[blue-bird1]

hi, can you review it?

[smowton]

The sanitizers are strange:

The comparison sanitizer says that if an SsaWithFields instance x.y.z is specifically the greater operand of a comparison (e.g. if x.y.z > 100) then all other uses of x.y.z are sanitized. But, there is no reason why x.y.z > 100 gives us more confidence than x.y.z < 100; it depends on the context where x.y.z is used to allocate memory.

Then FieldReadSanitizer says that if we have if e.f > 100 then everything is sanitized that comes afterwards in the control-flow graph, whether or not is has anything to do with e.f -- for example, it would sanitize if e.f > 100 { ... allocate(x) or allocate(e.g), even though x and e.g are unrelated to the sanitized expression e.f..

Can we make these more regular / easier to explain / understand?

[blue-bird1]

The sanitizers are strange:

The comparison sanitizer says that if an SsaWithFields instance x.y.z is specifically the greater operand of a comparison (e.g. if x.y.z > 100) then all other uses of x.y.z are sanitized. But, there is no reason why x.y.z > 100 gives us more confidence than x.y.z < 100; it depends on the context where x.y.z is used to allocate memory.

Then FieldReadSanitizer says that if we have if e.f > 100 then everything is sanitized that comes afterwards in the control-flow graph, whether or not is has anything to do with e.f -- for example, it would sanitize if e.f > 100 { ... allocate(x) or allocate(e.g), even though x and e.g are unrelated to the sanitized expression e.f..

Can we make these more regular / easier to explain / understand?

in a project , found the following pattern

a = struct{
 pagesize: sinkfunction()
} 

if a.pagesize > x {
   a.pagesize = const
}

sinkfunction(a)

[smowton]

Right, but we could also have had

if a.pagesize <= 16384 {
  sinkfunction(a.pagesize)
}

That's why I don't think the direction of the comparison is relevant to sanitization if we're santizing all other uses of that SsaWithFields (contrast if we're trying to do something smarter, like look at whether a particular use is controlled by such a comparison, in which case the direction is significant)

[blue-bird1]

Right, but we could also have had

if a.pagesize <= 16384 {
  sinkfunction(a.pagesize)
}

That's why I don't think the direction of the comparison is relevant to sanitization if we're santizing all other uses of that SsaWithFields (contrast if we're trying to do something smarter, like look at whether a particular use is controlled by such a comparison, in which case the direction is significant)

If it's not big enough, it won't be a problem. It is very common for a request to use 10-100MB of memory

[blue-bird1]

make(xx, 80000000) only 2G memory, I don't think any programmer would write if pagesize<=800000000

[smowton]

Right, but as a general matter they might use if x < maxsize { ... } as a way to guard allocation.

[blue-bird1]

You are right, I missed the pattern

if (pagesize< 20){
   sinkfunction()
}else{
  panicfunc()
}

[blue-bird1]

I wrongly assumed that development would fix the value instead of skip

[sidshank]

Hi @blue-bird1 👋🏼 We appreciate your contribution efforts in this PR. Due to other maintainer commitments and priorities over the next few weeks, we will not have the capacity to continue the review of this work, or to move it forward. We might return to review and move this work forward in the future if it aligns with our priorities, but I can offer no guarantees on the timeline.

I realize this may not be the outcome you wanted. You are free to leave this PR open if you choose and we'll reengage with it on a best-effort basis when we have the capacity for it.