Skip to content

Commit c681e69

Browse files
geoffw0geoffw0
committed
C++: Refine the strcat and strcpy models, have BufferWrite depend on them so that information isn't duplicated.
1 parent 38067b5 commit c681e69

File tree

3 files changed

+113
-112
lines changed

3 files changed

+113
-112
lines changed

‎cpp/ql/src/semmle/code/cpp/models/implementations/Strcat.qll

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,21 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid
2424
)
2525
}
2626

27+
/**
28+
* Gets the index of the parameter that is the size of the copy (in characters).
29+
*/
30+
int getParamSize() { if exists(getParameter(2)) then result = 2 else none() }
31+
32+
/**
33+
* Gets the index of the parameter that is the source of the copy.
34+
*/
35+
int getParamSrc() { result = 1 }
36+
37+
/**
38+
* Gets the index of the parameter that is the destination to be appended to.
39+
*/
40+
int getParamDest() { result = 0 }
41+
2742
override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
2843
input.isParameter(0) and
2944
output.isReturnValue()

‎cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll

Lines changed: 85 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -8,70 +8,107 @@ import semmle.code.cpp.models.interfaces.SideEffect
88
*/
99
class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, SideEffectFunction {
1010
StrcpyFunction() {
11-
this.hasName("strcpy") or
12-
this.hasName("_mbscpy") or
13-
this.hasName("wcscpy") or
14-
this.hasName("strncpy") or
15-
this.hasName("_strncpy_l") or
16-
this.hasName("_mbsncpy") or
17-
this.hasName("_mbsncpy_l") or
18-
this.hasName("wcsncpy") or
19-
this.hasName("_wcsncpy_l")
11+
exists(string name | name = getName() |
12+
// strcpy(dst, src)
13+
name = "strcpy"
14+
or
15+
// wcscpy(dst, src)
16+
name = "wcscpy"
17+
or
18+
// _mbscpy(dst, src)
19+
name = "_mbscpy"
20+
or
21+
(
22+
name = "strcpy_s" or // strcpy_s(dst, max_amount, src)
23+
name = "wcscpy_s" or // wcscpy_s(dst, max_amount, src)
24+
name = "_mbscpy_s" // _mbscpy_s(dst, max_amount, src)
25+
) and
26+
// exclude the 2-parameter template versions
27+
// that find the size of a fixed size destination buffer.
28+
getNumberOfParameters() = 3
29+
or
30+
// strncpy(dst, src, max_amount)
31+
name = "strncpy"
32+
or
33+
// _strncpy_l(dst, src, max_amount, locale)
34+
name = "_strncpy_l"
35+
or
36+
// wcsncpy(dst, src, max_amount)
37+
name = "wcsncpy"
38+
or
39+
// _wcsncpy_l(dst, src, max_amount, locale)
40+
name = "_wcsncpy_l"
41+
or
42+
// _mbsncpy(dst, src, max_amount)
43+
name = "_mbsncpy"
44+
or
45+
// _mbsncpy_l(dst, src, max_amount, locale)
46+
name = "_mbsncpy_l"
47+
)
48+
}
49+
50+
/**
51+
* Holds if this is one of the `strcpy_s` variants.
52+
*/
53+
private predicate isSVariant() {
54+
exists(string name | name = getName() | name.suffix(name.length() - 2) = "_s")
55+
}
56+
57+
/**
58+
* Gets the index of the parameter that is the maximum size of the copy (in characters).
59+
*/
60+
int getParamSize() {
61+
if isSVariant()
62+
then result = 1
63+
else
64+
if exists(getName().indexOf("ncpy"))
65+
then result = 2
66+
else none()
2067
}
2168

22-
override predicate hasArrayInput(int bufParam) { bufParam = 1 }
69+
/**
70+
* Gets the index of the parameter that is the source of the copy.
71+
*/
72+
int getParamSrc() { if isSVariant() then result = 2 else result = 1 }
2373

24-
override predicate hasArrayOutput(int bufParam) { bufParam = 0 }
74+
/**
75+
* Gets the index of the parameter that is the destination of the copy.
76+
*/
77+
int getParamDest() { result = 0 }
2578

26-
override predicate hasArrayWithNullTerminator(int bufParam) { bufParam = 1 }
79+
override predicate hasArrayInput(int bufParam) { bufParam = getParamSrc() }
80+
81+
override predicate hasArrayOutput(int bufParam) { bufParam = getParamDest() }
82+
83+
override predicate hasArrayWithNullTerminator(int bufParam) { bufParam = getParamSrc() }
2784

2885
override predicate hasArrayWithVariableSize(int bufParam, int countParam) {
29-
(
30-
this.hasName("strncpy") or
31-
this.hasName("_strncpy_l") or
32-
this.hasName("_mbsncpy") or
33-
this.hasName("_mbsncpy_l") or
34-
this.hasName("wcsncpy") or
35-
this.hasName("_wcsncpy_l")
36-
) and
37-
bufParam = 0 and
38-
countParam = 2
86+
bufParam = getParamDest() and
87+
countParam = getParamSize()
3988
}
4089

4190
override predicate hasArrayWithUnknownSize(int bufParam) {
42-
(
43-
this.hasName("strcpy") or
44-
this.hasName("_mbscpy") or
45-
this.hasName("wcscpy")
46-
) and
47-
bufParam = 0
91+
not exists(getParamSize()) and
92+
bufParam = getParamDest()
4893
}
4994

5095
override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
51-
input.isParameterDeref(1) and
52-
output.isParameterDeref(0)
96+
input.isParameterDeref(getParamSrc()) and
97+
output.isParameterDeref(getParamDest())
5398
or
54-
input.isParameterDeref(1) and
99+
input.isParameterDeref(getParamSrc()) and
55100
output.isReturnValueDeref()
56101
or
57-
input.isParameter(0) and
102+
input.isParameter(getParamDest()) and
58103
output.isReturnValue()
59104
}
60105

61106
override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
107+
// these may do only a partial copy of the input buffer to the output
108+
// buffer
109+
input.isParameter(getParamSize()) and
62110
(
63-
// these may do only a partial copy of the input buffer to the output
64-
// buffer
65-
this.hasName("strncpy") or
66-
this.hasName("_strncpy_l") or
67-
this.hasName("_mbsncpy") or
68-
this.hasName("_mbsncpy_l") or
69-
this.hasName("wcsncpy") or
70-
this.hasName("_wcsncpy_l")
71-
) and
72-
input.isParameter(2) and
73-
(
74-
output.isParameterDeref(0) or
111+
output.isParameterDeref(getParamDest()) or
75112
output.isReturnValueDeref()
76113
)
77114
}
@@ -81,17 +118,18 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid
81118
override predicate hasOnlySpecificWriteSideEffects() { any() }
82119

83120
override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) {
84-
i = 0 and
121+
i = getParamDest() and
85122
buffer = true and
86123
mustWrite = false
87124
}
88125

89126
override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
90-
i = 1 and
127+
i = getParamSrc() and
91128
buffer = true
92129
}
93130

94131
override ParameterIndex getParameterSizeIndex(ParameterIndex i) {
95-
hasArrayWithVariableSize(i, result)
132+
i = getParamDest() and
133+
result = getParamSize()
96134
}
97135
}

‎cpp/ql/src/semmle/code/cpp/security/BufferWrite.qll

Lines changed: 13 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import semmle.code.cpp.commons.Alloc
1010
import semmle.code.cpp.commons.Buffer
1111
import semmle.code.cpp.commons.Scanf
1212
import semmle.code.cpp.models.implementations.Strcat
13+
import semmle.code.cpp.models.implementations.Strcpy
1314

1415
/*
1516
* --- BufferWrite framework ---
@@ -106,82 +107,27 @@ abstract class BufferWriteCall extends BufferWrite, FunctionCall { }
106107
* A call to a variant of `strcpy`.
107108
*/
108109
class StrCopyBW extends BufferWriteCall {
109-
StrCopyBW() {
110-
exists(TopLevelFunction fn, string name | fn = getTarget() and name = fn.getName() |
111-
// strcpy(dst, src)
112-
name = "strcpy"
113-
or
114-
// wcscpy(dst, src)
115-
name = "wcscpy"
116-
or
117-
// _mbscpy(dst, src)
118-
name = "_mbscpy"
119-
or
120-
(
121-
name = "strcpy_s" or // strcpy_s(dst, max_amount, src)
122-
name = "wcscpy_s" or // wcscpy_s(dst, max_amount, src)
123-
name = "_mbscpy_s" // _mbscpy_s(dst, max_amount, src)
124-
) and
125-
// exclude the 2-parameter template versions
126-
// that find the size of a fixed size destination buffer.
127-
fn.getNumberOfParameters() = 3
128-
or
129-
// strncpy(dst, src, max_amount)
130-
name = "strncpy"
131-
or
132-
// strncpy_l(dst, src, max_amount, locale)
133-
name = "strncpy_l"
134-
or
135-
// wcsncpy(dst, src, max_amount)
136-
name = "wcsncpy"
137-
or
138-
// _wcsncpy_l(dst, src, max_amount, locale)
139-
name = "_wcsncpy_l"
140-
or
141-
// _mbsncpy(dst, src, max_amount)
142-
name = "_mbsncpy"
143-
or
144-
// _mbsncpy_l(dst, src, max_amount, locale)
145-
name = "_mbsncpy_l"
146-
)
147-
}
110+
StrcpyFunction f;
111+
112+
StrCopyBW() { getTarget() = f.(TopLevelFunction) }
148113

149114
/**
150115
* Gets the index of the parameter that is the maximum size of the copy (in characters).
151116
*/
152-
int getParamSize() {
153-
exists(TopLevelFunction fn, string name |
154-
fn = getTarget() and
155-
name = fn.getName() and
156-
(
157-
if name.suffix(name.length() - 2) = "_s"
158-
then result = 1
159-
else
160-
if exists(name.indexOf("ncpy"))
161-
then result = 2
162-
else none()
163-
)
164-
)
165-
}
117+
int getParamSize() { result = f.getParamSize() }
166118

167119
/**
168120
* Gets the index of the parameter that is the source of the copy.
169121
*/
170-
int getParamSrc() {
171-
exists(TopLevelFunction fn, string name |
172-
fn = getTarget() and
173-
name = fn.getName() and
174-
(if name.suffix(name.length() - 2) = "_s" then result = 2 else result = 1)
175-
)
176-
}
122+
int getParamSrc() { result = f.getParamSrc() }
177123

178124
override Type getBufferType() {
179125
result = this.getTarget().getParameter(getParamSrc()).getUnspecifiedType()
180126
}
181127

182128
override Expr getASource() { result = getArgument(getParamSrc()) }
183129

184-
override Expr getDest() { result = getArgument(0) }
130+
override Expr getDest() { result = getArgument(f.getParamDest()) }
185131

186132
override predicate hasExplicitLimit() { exists(getParamSize()) }
187133

@@ -198,25 +144,27 @@ class StrCopyBW extends BufferWriteCall {
198144
* A call to a variant of `strcat`.
199145
*/
200146
class StrCatBW extends BufferWriteCall {
201-
StrCatBW() { exists(TopLevelFunction fn | fn = getTarget() and fn instanceof StrcatFunction) }
147+
StrcatFunction f;
148+
149+
StrCatBW() { getTarget() = f.(TopLevelFunction) }
202150

203151
/**
204152
* Gets the index of the parameter that is the maximum size of the copy (in characters).
205153
*/
206-
int getParamSize() { if exists(getArgument(2)) then result = 2 else none() }
154+
int getParamSize() { result = f.getParamSize() }
207155

208156
/**
209157
* Gets the index of the parameter that is the source of the copy.
210158
*/
211-
int getParamSrc() { result = 1 }
159+
int getParamSrc() { result = f.getParamSrc() }
212160

213161
override Type getBufferType() {
214162
result = this.getTarget().getParameter(getParamSrc()).getUnspecifiedType()
215163
}
216164

217165
override Expr getASource() { result = getArgument(getParamSrc()) }
218166

219-
override Expr getDest() { result = getArgument(0) }
167+
override Expr getDest() { result = getArgument(f.getParamDest()) }
220168

221169
override predicate hasExplicitLimit() { exists(getParamSize()) }
222170

0 commit comments

Comments
 (0)