RB: add a query flagging uses of Kernel.open() that are not with a constant string

[erik-krogh]

CVE-2019-10780: TP/TN
CVE-2019-13574: TP/TN
CVE-2019-5477: TP/TN
CVE-2020-8130: TP/TN
CVE-2021-21289: TP/TN
CVE-2021-31799: TP/TN

The alert-message and severity are different from rb/kernel-open.
But I think the existing qhelp for rb/kernel-open fit nicely, so I reused it completely.

Evaluation looks fine. Performance is good.
There are plenty of results, but the results look OK to me.
Although I don't think many of them are exploitable.

The QHelp preview CI fails hard, and I'm not sure why.
It works fine locally, and the qhelp check passes.
So it's a problem with the preview workflow, and not with the qhelp itself.

[hmac]

There's a compile warning which should be fixed if you rebase against main. And I think the QHelp preview job may be fixed by a rebase too.

[hmac]

Suggested change

	* Provides a utility classes and predicates for queries reasoning about Kernel.open and related methods.
	* Provides utility classes and predicates for reasoning about `Kernel.open` and related methods.

[hmac]

My understanding is that we don't want to be adding any new abstract classes which are exposed to end users (unless we expect them to be extending it, and then we should use the range pattern). E.g. we moved a load of these into an internal directory in this PR. We may want to do the same thing or something similar here.

[erik-krogh]

I've changed it to a simple non-abstract class, and moved the implementations into that class.
We can always change that to the Range:: pattern later without breaking changes.

[hmac]

This looks good, but I wonder if there's a problem if we have two queries that can generate alerts for the same vulnerability? Will that lead to duplicate alerts in code scanning and/or some weird UX?

[github-actions]

QHelp previews:

ruby/ql/src/queries/security/cwe-078/KernelOpen.qhelp

Use of Kernel.open or IO.read with user-controlled input

If Kernel.open is given a file name that starts with a | character, it will execute the remaining string as a shell command. If a malicious user can control the file name, they can execute arbitrary code. The same vulnerability applies to IO.read.

Recommendation

Use File.open instead of Kernel.open, as the former does not have this vulnerability. Similarly, use File.read instead of IO.read.

Example

The following example shows code that calls Kernel.open on a user-supplied file path.

class UsersController < ActionController::Base
  def create
    filename = params[:filename]
    open(filename) # BAD
  end
end  

Instead, File.open should be used, as in the following example.

class UsersController < ActionController::Base
    def create
      filename = params[:filename]
      File.open(filename)
    end
  end  

References

• OWASP: Command Injection.
• Example CVE: Command Injection in RDoc.
• Common Weakness Enumeration: CWE-78.
• Common Weakness Enumeration: CWE-88.
• Common Weakness Enumeration: CWE-73.

ruby/ql/src/queries/security/cwe-078/NonConstantKernelOpen.qhelp

Use of Kernel.open or IO.read with a non-constant value

If Kernel.open is given a file name that starts with a | character, it will execute the remaining string as a shell command. If a malicious user can control the file name, they can execute arbitrary code. The same vulnerability applies to IO.read.

Recommendation

Use File.open instead of Kernel.open, as the former does not have this vulnerability. Similarly, use File.read instead of IO.read.

Example

The following example shows code that calls Kernel.open on a user-supplied file path.

class UsersController < ActionController::Base
  def create
    filename = params[:filename]
    open(filename) # BAD
  end
end  

Instead, File.open should be used, as in the following example.

class UsersController < ActionController::Base
    def create
      filename = params[:filename]
      File.open(filename)
    end
  end  

References

• OWASP: Command Injection.
• Example CVE: Command Injection in RDoc.
• Common Weakness Enumeration: CWE-78.
• Common Weakness Enumeration: CWE-88.
• Common Weakness Enumeration: CWE-73.

[erik-krogh]

This looks good, but I wonder if there's a problem if we have two queries that can generate alerts for the same vulnerability? Will that lead to duplicate alerts in code scanning and/or some weird UX?

I just tried that in a draft PR by adding a few QL-for-QL alerts on the same line.
You just get two code-scanning boxes next to each other, it l looks fine.

[aibaars]

This query has quite a lot of results, but they all seem to make sense. There are quite a lot of cases like if File.exists(path) then open(path) which are sort of safe (unless you have an existing file named |launch_the_rockets) . In those cases it would still be to use File.open instead of Kernel.open.

[erik-krogh]

@hmac do you have anymore comments?
I noticed you started an evaluation.

[hmac]

I think the duplicate alert is a bit unfortunate, but not a huge deal. The large number of results is also a little scary but hopefully since this is a "warning" with lowish severity, users won't mind too much...