From 0b9131035702f6ec885f443bb48b53f89f58123f Mon Sep 17 00:00:00 2001 From: Jeroen Ketema Date: Thu, 9 Nov 2023 17:21:37 +0100 Subject: [PATCH 1/7] C++: Add models for `strlcpy` and `strlcat` --- cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll | 5 +++-- cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll index 7e55868b847f..97a25fb9d372 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll @@ -25,7 +25,8 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid "_mbsncat", // _mbsncat(dst, src, max_amount) "_mbsncat_l", // _mbsncat_l(dst, src, max_amount, locale) "_mbsnbcat", // _mbsnbcat(dest, src, count) - "_mbsnbcat_l" // _mbsnbcat_l(dest, src, count, locale) + "_mbsnbcat_l", // _mbsnbcat_l(dest, src, count, locale) + "strlcat" // strncat(dst, src, dst_size) ]) } @@ -51,7 +52,7 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { ( - this.getName() = ["strncat", "wcsncat", "_mbsncat", "_mbsncat_l"] and + this.getName() = ["strncat", "strlcat", "wcsncat", "_mbsncat", "_mbsncat_l"] and input.isParameter(2) or this.getName() = ["_mbsncat_l", "_mbsnbcat_l"] and diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll index ea371de958ac..3ca4234af4f4 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll @@ -32,7 +32,8 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid "wcsxfrm_l", // _strxfrm_l(dest, src, max_amount, locale) "_mbsnbcpy", // _mbsnbcpy(dest, src, max_amount) "stpcpy", // stpcpy(dest, src) - "stpncpy" // stpcpy(dest, src, max_amount) + "stpncpy", // stpcpy(dest, src, max_amount) + "strlcpy" // strlcpy(dst, src, dst_size) ]) or ( @@ -60,7 +61,7 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid if this.isSVariant() then result = 1 else ( - this.getName().matches(["%ncpy%", "%nbcpy%", "%xfrm%"]) and + this.getName().matches(["%ncpy%", "%nbcpy%", "%xfrm%", "%lcpy%"]) and result = 2 ) } From a051a57e00a2a97791027e80ca68f6e0483473ac Mon Sep 17 00:00:00 2001 From: Jeroen Ketema <93738568+jketema@users.noreply.github.com> Date: Thu, 9 Nov 2023 17:48:45 +0100 Subject: [PATCH 2/7] Update cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll Co-authored-by: Mathias Vorreiter Pedersen --- cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll index 97a25fb9d372..3e9312da7715 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll @@ -26,7 +26,7 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid "_mbsncat_l", // _mbsncat_l(dst, src, max_amount, locale) "_mbsnbcat", // _mbsnbcat(dest, src, count) "_mbsnbcat_l", // _mbsnbcat_l(dest, src, count, locale) - "strlcat" // strncat(dst, src, dst_size) + "strlcat" // strlcat(dst, src, dst_size) ]) } From e4c84063656ded0e0a820fb85c344c711239154a Mon Sep 17 00:00:00 2001 From: Jeroen Ketema Date: Fri, 10 Nov 2023 09:55:58 +0100 Subject: [PATCH 3/7] C++: Split `strlcat` off in a separate model --- .../cpp/models/implementations/Strcat.qll | 73 ++++++++++++++++++- 1 file changed, 70 insertions(+), 3 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll index 3e9312da7715..fe97d2510f53 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll @@ -10,6 +10,8 @@ import semmle.code.cpp.models.interfaces.SideEffect /** * The standard function `strcat` and its wide, sized, and Microsoft variants. + * + * Does not include `strlcat`, which is covered by `StrlcatFunction` */ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, SideEffectFunction { StrcatFunction() { @@ -25,8 +27,7 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid "_mbsncat", // _mbsncat(dst, src, max_amount) "_mbsncat_l", // _mbsncat_l(dst, src, max_amount, locale) "_mbsnbcat", // _mbsnbcat(dest, src, count) - "_mbsnbcat_l", // _mbsnbcat_l(dest, src, count, locale) - "strlcat" // strlcat(dst, src, dst_size) + "_mbsnbcat_l" // _mbsnbcat_l(dest, src, count, locale) ]) } @@ -52,7 +53,7 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { ( - this.getName() = ["strncat", "strlcat", "wcsncat", "_mbsncat", "_mbsncat_l"] and + this.getName() = ["strncat", "wcsncat", "_mbsncat", "_mbsncat_l"] and input.isParameter(2) or this.getName() = ["_mbsncat_l", "_mbsnbcat_l"] and @@ -91,3 +92,69 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid buffer = true } } + +/** + * The `strlcat` function. + */ +class StrlcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, SideEffectFunction { + StrlcatFunction() { + this.hasGlobalName("strlcat") // strlcat(dst, src, dst_size) + } + + /** + * Gets the index of the parameter that is the size of the copy (in characters). + */ + int getParamSize() { exists(this.getParameter(2)) and result = 2 } + + /** + * Gets the index of the parameter that is the source of the copy. + */ + int getParamSrc() { result = 1 } + + /** + * Gets the index of the parameter that is the destination to be appended to. + */ + int getParamDest() { result = 0 } + + override predicate hasDataFlow(FunctionInput input, FunctionOutput output) { + input.isParameter(0) and + output.isReturnValue() + } + + override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { + ( + input.isParameter(2) + or + input.isParameterDeref(0) + or + input.isParameterDeref(1) + ) and + (output.isParameterDeref(0) or output.isReturnValueDeref()) + } + + override predicate hasArrayInput(int param) { + param = 0 or + param = 1 + } + + override predicate hasArrayOutput(int param) { param = 0 } + + override predicate hasArrayWithNullTerminator(int param) { param = 1 } + + override predicate hasArrayWithUnknownSize(int param) { param = 0 } + + override predicate hasOnlySpecificReadSideEffects() { any() } + + override predicate hasOnlySpecificWriteSideEffects() { any() } + + override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) { + i = 0 and + buffer = true and + mustWrite = false + } + + override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) { + (i = 0 or i = 1) and + buffer = true + } +} From 5e21a5d28441db320a88cb27bf44589f44a6fb97 Mon Sep 17 00:00:00 2001 From: Jeroen Ketema Date: Fri, 10 Nov 2023 12:28:48 +0100 Subject: [PATCH 4/7] C++: Fix flow for return values of `strlcat` and `strlcpy` --- .../code/cpp/models/implementations/Strcat.qll | 9 ++------- .../code/cpp/models/implementations/Strcpy.qll | 13 ++++++++++--- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll index fe97d2510f53..eaf275bf0618 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll @@ -96,7 +96,7 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid /** * The `strlcat` function. */ -class StrlcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, SideEffectFunction { +class StrlcatFunction extends TaintFunction, ArrayFunction, SideEffectFunction { StrlcatFunction() { this.hasGlobalName("strlcat") // strlcat(dst, src, dst_size) } @@ -116,11 +116,6 @@ class StrlcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Si */ int getParamDest() { result = 0 } - override predicate hasDataFlow(FunctionInput input, FunctionOutput output) { - input.isParameter(0) and - output.isReturnValue() - } - override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { ( input.isParameter(2) @@ -129,7 +124,7 @@ class StrlcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Si or input.isParameterDeref(1) ) and - (output.isParameterDeref(0) or output.isReturnValueDeref()) + (output.isParameterDeref(0) or output.isReturnValue()) } override predicate hasArrayInput(int param) { diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll index 3ca4234af4f4..22b14a05dad9 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll @@ -54,6 +54,11 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid */ private predicate isSVariant() { this.getName().matches("%\\_s") } + /** + * Holds if the function returns the total length the string would have had if the size was unlimited. + */ + private predicate returnsTotalLength() { this.getName() = "strlcpy" } + /** * Gets the index of the parameter that is the maximum size of the copy (in characters). */ @@ -61,7 +66,7 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid if this.isSVariant() then result = 1 else ( - this.getName().matches(["%ncpy%", "%nbcpy%", "%xfrm%", "%lcpy%"]) and + this.getName().matches(["%ncpy%", "%nbcpy%", "%xfrm%", "strlcpy"]) and result = 2 ) } @@ -101,6 +106,7 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid input.isParameterDeref(this.getParamSrc()) and output.isReturnValueDeref() or + not this.returnsTotalLength() and input.isParameter(this.getParamDest()) and output.isReturnValue() } @@ -111,8 +117,9 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid exists(this.getParamSize()) and input.isParameterDeref(this.getParamSrc()) and ( - output.isParameterDeref(this.getParamDest()) or - output.isReturnValueDeref() + output.isParameterDeref(this.getParamDest()) + or + not this.returnsTotalLength() and output.isReturnValueDeref() ) } From 1c878750494e87603a8519b5463297cbfb3b6712 Mon Sep 17 00:00:00 2001 From: Jeroen Ketema Date: Fri, 10 Nov 2023 13:15:57 +0100 Subject: [PATCH 5/7] C++: Drop the size return value of `strlcat` from `hasTaintFlow` --- cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll index eaf275bf0618..0291aee5fd53 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll @@ -124,7 +124,7 @@ class StrlcatFunction extends TaintFunction, ArrayFunction, SideEffectFunction { or input.isParameterDeref(1) ) and - (output.isParameterDeref(0) or output.isReturnValue()) + output.isParameterDeref(0) } override predicate hasArrayInput(int param) { From 617d950a256cbbad2ef7840bb6e27d9da5ed45b8 Mon Sep 17 00:00:00 2001 From: Jeroen Ketema <93738568+jketema@users.noreply.github.com> Date: Fri, 10 Nov 2023 13:55:39 +0100 Subject: [PATCH 6/7] Apply suggestions from code review Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com> --- cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll | 2 +- cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll index 0291aee5fd53..f081a36fac62 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll @@ -104,7 +104,7 @@ class StrlcatFunction extends TaintFunction, ArrayFunction, SideEffectFunction { /** * Gets the index of the parameter that is the size of the copy (in characters). */ - int getParamSize() { exists(this.getParameter(2)) and result = 2 } + int getParamSize() { result = 2 } /** * Gets the index of the parameter that is the source of the copy. diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll index 22b14a05dad9..1858da65234d 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll @@ -32,7 +32,7 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid "wcsxfrm_l", // _strxfrm_l(dest, src, max_amount, locale) "_mbsnbcpy", // _mbsnbcpy(dest, src, max_amount) "stpcpy", // stpcpy(dest, src) - "stpncpy", // stpcpy(dest, src, max_amount) + "stpncpy", // stpncpy(dest, src, max_amount) "strlcpy" // strlcpy(dst, src, dst_size) ]) or From b48d483eba1ca88fcbd94fb76be7604fc52d7173 Mon Sep 17 00:00:00 2001 From: Jeroen Ketema Date: Fri, 10 Nov 2023 17:25:19 +0100 Subject: [PATCH 7/7] C++: Add change note --- cpp/ql/lib/change-notes/2023-11-10-strlcpy-strlcat-models.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 cpp/ql/lib/change-notes/2023-11-10-strlcpy-strlcat-models.md diff --git a/cpp/ql/lib/change-notes/2023-11-10-strlcpy-strlcat-models.md b/cpp/ql/lib/change-notes/2023-11-10-strlcpy-strlcat-models.md new file mode 100644 index 000000000000..6e4e1fe8a53a --- /dev/null +++ b/cpp/ql/lib/change-notes/2023-11-10-strlcpy-strlcat-models.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added models for `strlcpy` and `strlcat`.