CPP: Add query for CWE-14 compiler removal of code to clear buffers.

[ihsinme]

Good day.

The error in question in this PR is quite common in projects, I tried to minimize the false detection. PR also takes into account the specifics of various compilers.

note that I had to add c.getArgument (2) .toString () = "--force-recompute" to keep the tests running. It was a surprise to me, but the "codeql test run" uses a different line when compiling than the "codeql query run".

I am also worried about your rule.

"Compilation

Compilation of the query and any associated libraries and tests must be resilient to future development of the supported libraries. This means that the functionality cannot use internal libraries, cannot depend on the output of getAQlClass, and cannot make use of regexp matching on toString.
The query and any associated libraries and tests must not cause any compiler warnings to be emitted (such as use of deprecated functionality or missing override annotations). "

if it is a problem then I am ready to weaken this detection.

I want to draw your attention to the following features in the presented PRs.

1. accepted the offer in an optional format.
2. agreed but did not accept, referring to the borrowed code.
   fix cleaning important data. lsh123/xmlsec#297
   fix cleaning important data. google/jsonnet#856

[jbj]

Thanks for the submission. We'll find a reviewer.

[MathiasVP]

Thanks for your contribution! This is indeed a problem that is worthy of a CodeQL query :)

I've attached the first round of review comments. If anything is unclear please let me know.

[ihsinme]

thanks for your corrections.
I'll try to fix them quickly.

[intrigus-lgtm]

@ihsinme a small tip that also helped me:
You can go to the "Files changed" tab and add multiple suggestions.
This way you can apply multiple suggestions in only one commit :)

[intrigus-lgtm]

All these** commits have only one change.
Instead of having x commits with only 1 change, you can have only 1 commit with x changes, which looks cleaner.

To do that, you would go to https://github.com/github/codeql/pull/4953/files and click on "Add suggestion to batch" for every suggestion as seen in step 3.2 ħere.
Then you can click on "Commit suggestions" to commit them all in only 1 commit :)

Hope this is better to understand :)

**:
4631658
76b768f
4ba4de3
0d0ea0c

[ihsinme]

I cannot see what is the cause of the error.
Can you help me?
QHelp Preview Checks (GitHub Actions) - Internal CI run started by Qlucie

[criemen]

The error message is

   [2021-01-14 15:15:31] This is codeql generate query-help -vvv --log-to-stderr --output out --format markdown --search-path . -- cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp
  [2021-01-14 15:15:31] [WARN] generate query-help> cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql does not have a corresponding qhelp file.
  [2021-01-14 15:15:31] Exception caught at top level: Failed to read path cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql
                        (eventual cause: NoSuchFileException "cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql")
                        com.semmle.util.io.WholeIO.strictread(WholeIO.java:318)
                        com.semmle.prettyprint.doc.QhelpToMD.<init>(QhelpToMD.java:601)
                        com.semmle.prettyprint.doc.QhelpToMD.<init>(QhelpToMD.java:590)
                        com.semmle.cli2.generate.QueryHelpCommand.executeSubcommand(QueryHelpCommand.java:133)
                        com.semmle.cli2.picocli.SubcommandCommon.call(SubcommandCommon.java:430)
                        com.semmle.cli2.picocli.SubcommandMaker.runMain(SubcommandMaker.java:201)
                        com.semmle.cli2.picocli.SubcommandMaker.runMain(SubcommandMaker.java:209)
                        com.semmle.cli2.CodeQL.main(CodeQL.java:91)
                        
                         ... caused by:
                        
                        java.nio.file.NoSuchFileException: cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql
                        sun.nio.fs.UnixException.translateToIOException(UnixException.java:86)
                        sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)
                        sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:107)
                        sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:214)
                        java.nio.file.Files.newByteChannel(Files.java:361)
                        java.nio.file.Files.newByteChannel(Files.java:407)
                        java.nio.file.spi.FileSystemProvider.newInputStream(FileSystemProvider.java:384)
                        java.nio.file.Files.newInputStream(Files.java:152)
                        com.semmle.util.io.WholeIO.read(WholeIO.java:265)
                        com.semmle.util.io.WholeIO.strictread(WholeIO.java:316)
                        com.semmle.prettyprint.doc.QhelpToMD.<init>(QhelpToMD.java:601)
                        com.semmle.prettyprint.doc.QhelpToMD.<init>(QhelpToMD.java:590)
                        com.semmle.cli2.generate.QueryHelpCommand.executeSubcommand(QueryHelpCommand.java:133)
                        com.semmle.cli2.picocli.SubcommandCommon.call(SubcommandCommon.java:430)
                        com.semmle.cli2.picocli.SubcommandMaker.runMain(SubcommandMaker.java:201)
                        com.semmle.cli2.picocli.SubcommandMaker.runMain(SubcommandMaker.java:209)
                        com.semmle.cli2.CodeQL.main(CodeQL.java:91)
  A fatal error occurred: Failed to read path cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql
  (eventual cause: NoSuchFileException "cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql")
  Error: Process completed with exit code 2.

[ihsinme]

Thank you.
the error looks strange to me, I still don't understand what to do in this situation.
it is possible to restart this check.

[MathiasVP]

it is possible to restart this check.

I've restarted it now.

[ihsinme]

but unfortunately the error has not gone away.
if the error message remains the same, then I can assume there is a path problem in the test itself.
for my part I revisited the ql qhelp file paths again and found no problem.
I would be grateful if you could suggest possible actions on my part, or provide additional information to reproduce the problem on my side.

[MathiasVP]

for my part I revisited the ql qhelp file paths again and found no problem.

Thanks for looking at the problem. I'll take a look at it later today.

[ihsinme]

sorry Sorry sorry ....

[MathiasVP]

sorry Sorry sorry ....

No worries! We've identified a problem with the PR check, so it's totally not your fault :) We'll have a fix on Monday and then we can merge your query!

[MathiasVP]

The failing PR check is still being fixed. I see you've opened github/securitylab#237 to get a bounty for getting the query merged, and the ambition level there is up to you.
You can create follow-up PRs to make the query more production-ready, following the steps in https://github.com/github/codeql/blob/main/docs/supported-queries.md.

Until then, we'll continue to review any PRs you make to improve the query. It would be very helpful if you could explicitly state in github/securitylab#237 once you're done improving the query.

[ihsinme]

good day.
I'm not experienced enough yet so sorry for the simple questions.
unless I currently plan on improving this request. Did I understand correctly that I need to write in the request for reward that I have finished improving the request?

[MathiasVP]

good day.

I'm not experienced enough yet so sorry for the simple questions.

unless I currently plan on improving this request. Did I understand correctly that I need to write in the request for reward that I have finished improving the request?

Yes. If you don't plan on doing further improvements to the query please say so in github/securitylab#237

[kevinbackhouse]

This query has a lot of false positives. Click here for a typical example. That memset won't get removed by the compiler because it isn't erasing a stack buffer that's about to go out of scope. I would suggest trying the StackAddress library.

I have seen one good result. But I am puzzled why the query doesn't also tag the memset(nonce32, 0, 32) a few line down, which looks like an identical pattern to me.

[ihsinme]

This query has a lot of false positives. Click here for a typical example. That memset won't get removed by the compiler because it isn't erasing a stack buffer that's about to go out of scope. I would suggest trying the StackAddress library.

indeed this is a false detection. it probably comes from a rather long chain of heap bindings. when two or more variables recursively assign each other a heap value. I'll try to fix it using StackAddress. Thank you.

I have seen one good result. But I am puzzled why the query doesn't also tag the memset(nonce32, 0, 32) a few line down, which looks like an identical pattern to me.

this place memset (nonce32, 0, 32) is skipped, probably because the initialization takes place inside another function, I don't know yet whether it is worth adding this detection. in my opinion it will please other false positives. but you definitely know better.

[ihsinme]

I corrected, I expect your reaction.
I think that StackAddress may in the future eliminate the need to use some of my predicates in this query, but I'm not ready for that yet.

I consider myself a newbie in your environment, so thanks for the fixes, they add experience.

[ihsinme]

good day.
maybe I have something else to do so that the check is launched and the PR is accepted?

[MathiasVP]

We are still evaluating the query. github/securitylab#237 shows the current state of the evaluation process.

[MathiasVP]

this place memset (nonce32, 0, 32) is skipped, probably because the initialization takes place inside another function, I don't know yet whether it is worth adding this detection. in my opinion it will please other false positives. but you definitely know better.

If you want to handle this case, you can do so by changing your characteristic predicate from this:

CompilerRemovaMemset() {
  this.getTarget().hasGlobalOrStdName("memset") and
  exists(DataFlow::Node source, DataFlow::Node sink, LocalVariable isv, Expr exp |
    DataFlow::localFlow(source, sink) and
    this.getArgument(0) = isv.getAnAccess() and
    source.asExpr() = exp and
    exp.getLocation().getEndLine() < this.getArgument(0).getLocation().getStartLine() and
    sink.asExpr() = this.getArgument(0)
  )
}

to this:

CompilerRemovaMemset() {
  this.getTarget().hasGlobalOrStdName("memset") and
  exists(DataFlow::Node source, DataFlow::Node sink, LocalVariable isv, Expr exp |
    DataFlow::localFlow(source, sink) and
    this.getArgument(0) = isv.getAnAccess() and
    (
      source.asExpr() = exp
      or
      // handle the case where exp is defined by an address being passed into some function.
      source.asDefiningArgument() = exp
    ) and
    exp.getLocation().getEndLine() < this.getArgument(0).getLocation().getStartLine() and
    sink.asExpr() = this.getArgument(0)
  )
}

I'll leave it up to you whether this change is worth it or not.

[ihsinme]

thanks for the help.
I'll take a look.

[ihsinme]

it really gives additional error detection.
I do not yet see the possibility of an additional false detection.
so once again I thank you for your help, I hope in future PR I will need it less.
and I accept this change.

I definitely need to read more about Expr. (

[kevinbackhouse]

@ihsinme: just by coincidence I am currently writing some code that handles passwords, so I tried using memset_s to clear the buffer. But, as far as I can tell, memset_s isn't defined on Linux. Do you know what the recommended way to do this is on Linux? (This question isn't related to the bounty process.)

[ihsinme]

@ihsinme: just by coincidence I am currently writing some code that handles passwords, so I tried using memset_s to clear the buffer. But, as far as I can tell, memset_s isn't defined on Linux. Do you know what the recommended way to do this is on Linux? (This question isn't related to the bounty process.)

it is optimal to use compile flag -fno-builtin-memset. which will exclude deletion of memset.

a more hardcore method is organizing access to array elements after zeroing.

[jbj]

There's a good USENIX paper on how to use memset securely. It's definitely worth linking to from the qhelp if/when we make a non-experimental version of this query.