Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brodes/seh flow phase1 throwing models #18014

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
de05aee
Adding model transition to using Throwing.qll.
bdrodes Nov 18, 2024
4b83a45
Change log
bdrodes Nov 18, 2024
1c7b5ae
Merge branch 'main' into brodes/seh_flow_phase1_throwing_models
bdrodes Nov 18, 2024
792231c
Removing SEH default case for function calls as the logic to handle S…
bdrodes Nov 18, 2024
1c874d3
Fixed usage raisesException
bdrodes Nov 19, 2024
5bb765d
Merge branch 'main' into brodes/seh_flow_phase1_throwing_models
bdrodes Nov 19, 2024
26d590a
Putting back deleted file, and deprecating instead. Deprecating mayTh…
bdrodes Nov 19, 2024
63ddd81
Merge branch 'brodes/seh_flow_phase1_throwing_models' of https://gith…
bdrodes Nov 19, 2024
0784776
bringing back mayThrowException to make it cleaner/easier for backwar…
bdrodes Nov 19, 2024
ae1ed38
Merge branch 'main' into brodes/seh_flow_phase1_throwing_models
bdrodes Nov 19, 2024
a69daa0
Missing change to 'mayThrowException' in StructuredExceptionHandling.qll
bdrodes Nov 19, 2024
23485f1
Merge branch 'brodes/seh_flow_phase1_throwing_models' of https://gith…
bdrodes Nov 19, 2024
4e77756
Changing terminology back to "throws" vs "rasis" for alwaysThrowsExce…
bdrodes Nov 19, 2024
69df07e
Update cpp/ql/lib/change-notes/2024-11-18-throwing-functions.md
bdrodes Nov 20, 2024
6aa7412
Merge branch 'main' into brodes/seh_flow_phase1_throwing_models
bdrodes Nov 20, 2024
9b2590e
Updating PR per review comments. Moving more towards a simplified model.
bdrodes Nov 21, 2024
4412691
Delaying deprecation of ThrowingFunction.
bdrodes Nov 21, 2024
7059fc3
Adding intermediate solution towards deprecating ThrowingFunction
bdrodes Nov 21, 2024
248f1c4
Updating change log
bdrodes Nov 21, 2024
583651b
Missing NonCppThrowingFunction changes in Printf.qll
bdrodes Nov 21, 2024
66cf736
printf formatting.
bdrodes Nov 21, 2024
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
4 changes: 4 additions & 0 deletions cpp/ql/lib/change-notes/2024-11-18-throwing-functions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Removed NonThrowing.qll. Throwing meta-data now part of Throwing.qll. Updated models and IR to use the new Throwing library and predicates.
bdrodes marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -363,11 +363,11 @@ class TranslatedFunctionCall extends TranslatedCallExpr, TranslatedDirectCall {
}

final override predicate mayThrowException() {
expr.getTarget().(ThrowingFunction).mayThrowException(_)
expr.getTarget().(ThrowingFunction).raisesException(_)
}

final override predicate mustThrowException() {
expr.getTarget().(ThrowingFunction).mayThrowException(true)
expr.getTarget().(ThrowingFunction).raisesException(true)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@
* functions. See `semmle.code.cpp.models.Models` for usage information.
*/

import semmle.code.cpp.Function

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.ArrayFunction
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.DataFlow
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.Alias
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.SideEffect
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.Taint
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.NonThrowing
.
import semmle.code.cpp.models.interfaces.ArrayFunction
import semmle.code.cpp.models.interfaces.DataFlow
import semmle.code.cpp.models.interfaces.Alias
import semmle.code.cpp.models.interfaces.SideEffect
import semmle.code.cpp.models.interfaces.Taint
import semmle.code.cpp.models.interfaces.NonThrowing
import semmle.code.cpp.models.interfaces.Throwing

/**
* The standard functions `memcpy`, `memmove` and `bcopy`; and the gcc variant
Expand Down Expand Up @@ -106,6 +106,8 @@
not this.hasGlobalName(["bcopy", mempcpy(), "memccpy"]) and
index = this.getParamDest()
}

override TCxxException getExceptionType() { any() }
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this confusing. Together with the NonThrowing this seems to say that:

This function is non-throwing, but only in the case of C++ exceptions. So it may still throw a SEH exception.

Is that the correct reading?

Copy link
Contributor Author

@bdrodes bdrodes Nov 21, 2024

Choose a reason for hiding this comment

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

That's right. That's what it is saying. It is no longer sufficient to say a function doesn't throw, you have to say how it doesn't throw (which kind of exception doesn't it throw). If they want to say it doesn't throw any you can just return the parent exception type.

The issue we got into with making memcpy nonthrowing is that it is true that it doesn't throw a C++ exception, but it absolutely throws a SEH exception. The mechanics in this PR force users to think about what it is they really want when they say a function throws or doesn't throw.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.

I wonder if this is all somewhat overly complicated. In my understanding the following cases are interesting:

  • C++ exceptions:
    • Functions that may throw (this is the default, but are modelling of this in the IR is limited)
    • Functions that do never throw a C++ exception, i.e., functions marked either as `noexcept or that are C functions (that we model)
  • SEH exceptions:
    • Functions that may throw (this is the default, and we want to improve the modelling in the IR here)
    • Functions that always throw

Is this correct?

Copy link
Contributor

@jketema jketema Nov 21, 2024

Choose a reason for hiding this comment

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

We have a brief meeting. The conclusion was that my assessment above is correct. The proposal is do something simpler:

  • Rename, NonThrowing to something like NonCppThrowingFunction and introduce a deprecated NonThrowing alias.
  • Introduce a new class AlwaysSehThrowingFunction which is used to model functions that always throw an SEH exception.
  • Deprecate the Throwing class.

We should also remove the use of the Throwing class in the models, but we can only do that in the next phase when we update the IR (otherwise the IR breaks).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overhauled the PR, let me know if that works.

}

private string mempcpy() { result = ["mempcpy", "wmempcpy"] }
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
* functions. See `semmle.code.cpp.models.Models` for usage information.
*/

import semmle.code.cpp.Function

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.ArrayFunction
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.DataFlow
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.Alias
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.SideEffect
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.NonThrowing
.
import semmle.code.cpp.models.interfaces.ArrayFunction
import semmle.code.cpp.models.interfaces.DataFlow
import semmle.code.cpp.models.interfaces.Alias
import semmle.code.cpp.models.interfaces.SideEffect
import semmle.code.cpp.models.interfaces.NonThrowing
import semmle.code.cpp.models.interfaces.Throwing

private class MemsetFunctionModel extends ArrayFunction, DataFlowFunction, AliasFunction,
SideEffectFunction, NonThrowingFunction
Expand Down Expand Up @@ -74,6 +74,8 @@
i = 0 and
if this.hasGlobalName(bzero()) then result = 1 else result = 2
}

override TCxxException getExceptionType() { any() }
}

private string bzero() { result = ["bzero", "explicit_bzero"] }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import semmle.code.cpp.models.interfaces.NonThrowing
import semmle.code.cpp.models.interfaces.Throwing

/**
* A function that is annotated with a `noexcept` specifier (or the equivalent
Expand All @@ -8,4 +8,6 @@ import semmle.code.cpp.models.interfaces.NonThrowing
*/
class NoexceptFunction extends NonThrowingFunction {
NoexceptFunction() { this.isNoExcept() or this.isNoThrow() }

override TCxxException getExceptionType() { any() }
}
12 changes: 11 additions & 1 deletion cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import semmle.code.cpp.models.interfaces.FormattingFunction
import semmle.code.cpp.models.interfaces.Alias
import semmle.code.cpp.models.interfaces.SideEffect
import semmle.code.cpp.models.interfaces.NonThrowing
import semmle.code.cpp.models.interfaces.Throwing

/**
* The standard functions `printf`, `wprintf` and their glib variants.
Expand All @@ -32,6 +32,8 @@ private class Printf extends FormattingFunction, AliasFunction, NonThrowingFunct
override predicate parameterEscapesOnlyViaReturn(int n) { none() }

override predicate parameterIsAlwaysReturned(int n) { none() }

override TCxxException getExceptionType() { any() }
}

/**
Expand All @@ -50,6 +52,8 @@ private class Fprintf extends FormattingFunction, NonThrowingFunction {
override int getFormatParameterIndex() { result = 1 }

override int getOutputParameterIndex(boolean isStream) { result = 0 and isStream = true }

override TCxxException getExceptionType() { any() }
}

/**
Expand Down Expand Up @@ -93,6 +97,8 @@ private class Sprintf extends FormattingFunction, NonThrowingFunction {
then result = 4
else result = super.getFirstFormatArgumentIndex()
}

override TCxxException getExceptionType() { any() }
}

/**
Expand Down Expand Up @@ -165,6 +171,8 @@ private class SnprintfImpl extends Snprintf, AliasFunction, SideEffectFunction,
// We don't know how many parameters are passed to the function since it's varargs, but they also have read side effects.
i = this.getFormatParameterIndex() and buffer = true
}

override TCxxException getExceptionType() { any() }
}

/**
Expand Down Expand Up @@ -215,4 +223,6 @@ private class Syslog extends FormattingFunction, NonThrowingFunction {
override int getFormatParameterIndex() { result = 1 }

override predicate isOutputGlobal() { any() }

override TCxxException getExceptionType() { any() }
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import semmle.code.cpp.models.interfaces.ArrayFunction
import semmle.code.cpp.models.interfaces.DataFlow
import semmle.code.cpp.models.interfaces.Taint
import semmle.code.cpp.models.interfaces.SideEffect
import semmle.code.cpp.models.interfaces.NonThrowing
import semmle.code.cpp.models.interfaces.Throwing

/**
* The standard function `strcat` and its wide, sized, and Microsoft variants.
Expand Down Expand Up @@ -94,6 +94,8 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid
(i = 0 or i = 1) and
buffer = true
}

override TCxxException getExceptionType() { any() }
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import semmle.code.cpp.models.interfaces.ArrayFunction
import semmle.code.cpp.models.interfaces.DataFlow
import semmle.code.cpp.models.interfaces.Taint
import semmle.code.cpp.models.interfaces.SideEffect
import semmle.code.cpp.models.interfaces.NonThrowing
import semmle.code.cpp.models.interfaces.Throwing

/**
* The standard function `strcpy` and its wide, sized, and Microsoft variants.
Expand Down Expand Up @@ -145,4 +145,6 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid
i = this.getParamDest() and
result = this.getParamSize()
}

override TCxxException getExceptionType() { any() }
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import semmle.code.cpp.models.interfaces.Throwing

class WindowsDriverFunction extends ThrowingFunction {
WindowsDriverFunction() {
class WindowsDriverExceptionAnnotation extends ThrowingFunction {
WindowsDriverExceptionAnnotation() {
this.hasGlobalName(["RaiseException", "ExRaiseAccessViolation", "ExRaiseDatatypeMisalignment"])
}

final override predicate mayThrowException(boolean unconditional) { unconditional = true }
override predicate raisesException(boolean unconditional) { unconditional = true }

override TSehException getExceptionType() { any() }
}
11 changes: 0 additions & 11 deletions cpp/ql/lib/semmle/code/cpp/models/interfaces/NonThrowing.qll

This file was deleted.

69 changes: 64 additions & 5 deletions cpp/ql/lib/semmle/code/cpp/models/interfaces/Throwing.qll
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,71 @@ import semmle.code.cpp.models.Models
import semmle.code.cpp.models.interfaces.FunctionInputsAndOutputs

/**
* A class that models the exceptional behavior of a function.
* Represents a type of exception,
* either Structured Exception Handling (SEH) or C++ exceptions.
*/
abstract class ThrowingFunction extends Function {
newtype TException =
/** Structured Exception Handling (SEH) exception */
TSehException() or
/** C++ exception */
TCxxException()

/**
* Functions with information about how an exception is thrown or if one is thrown at all.
* If throwing details conflict for the same function, IR is assumed
* to use the most restricted interpretation, meaning taking options
* that stipulate no exception is raised, before the exception is always raised,
* before conditional exceptions.
*
* Annotations must specify if the exception is from SEH (structured exception handling)
* or ordinary c++ exceptions.
*/
abstract private class ExceptionAnnotation extends Function {
/**
* Returns the type of exception this annotation is for,
* either a CPP exception or a STructured Exception Handling (SEH) exception.
*/
abstract TException getExceptionType();

/**
* Holds if the exception type of this annotation is for a Structured Exception Handling (SEH) exception.
*/
final predicate isSeh() { this.getExceptionType() = TSehException() }

/**
* Holds if the exception type of this annotation is for a CPP exception.
*/
final predicate isCxx() { this.getExceptionType() = TCxxException() }
}

/**
* A Function that is known to not throw an exception.
*/
abstract class NonThrowingFunction extends ExceptionAnnotation { }

/**
* A function this is known to raise an exception.
*/
abstract class ThrowingFunction extends ExceptionAnnotation {
ThrowingFunction() { any() }

/**
* Holds if this function may raise an exception during evaluation.
* If `unconditional` is `false` the function may raise, and if `true` the function
* will always raise an exception.
* Do not specify `none()` if no exception is raised, instead use the
* `NonThrowingFunction` class instead.
*/
abstract predicate raisesException(boolean unconditional);

/**
* Holds if this function will always raise an exception if called
*/
final predicate alwaysRaisesException() { this.raisesException(true) }

/**
* Holds if this function may throw an exception during evaluation.
* If `unconditional` is `true` the function always throws an exception.
* Holds if this function may raise an exception if called but
* it is not guaranteed to do so. I.e., the function does not always raise an exception.
*/
abstract predicate mayThrowException(boolean unconditional);
final predicate mayRaiseException() { this.raisesException(false) }
Copy link
Contributor

@jketema jketema Nov 19, 2024

Choose a reason for hiding this comment

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

Similarly, the mayThrowException predicate will need to be kept for now and marked as deprecated.

Copy link
Contributor Author

@bdrodes bdrodes Nov 19, 2024

Choose a reason for hiding this comment

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

Need to rethink this generally for backwards compatibility given the original thing being deprecated is abstract. Stand by...

Copy link
Contributor Author

@bdrodes bdrodes Nov 19, 2024

Choose a reason for hiding this comment

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

Ok I decided to move back to the old mechanic. I switched because SEH doesn't really throw an exception per se. But it's fine, and easier to just keep it the way it was. The changes have been made, just waiting for the checks to pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm seeing failures with some security/CWE tests, but I cannot recreate that locally. @MathiasVP is that a common discrepancy? can you maybe run the test locally as well? Thye all passed for me.

}