Skip to content

Commit 5260d98

Browse files
hvitvedhvitved
authored
Merge pull request github#12582 from hvitved/ruby/element-of-type-content-set
Ruby: Introduce `ContentSet::isElementOfType[OrUnknown]/1`
2 parents caa25f7 + a9ef3f9 commit 5260d98

File tree

7 files changed

+101
-12
lines changed

7 files changed

+101
-12
lines changed

‎ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
private import codeql.util.Boolean
12
private import codeql.ruby.AST
23
private import codeql.ruby.ast.internal.Synthesis
34
private import codeql.ruby.CFG
@@ -456,12 +457,15 @@ private module Cached {
456457
FlowSummaryImplSpecific::ParsePositions::isParsedElementLowerBoundPosition(_, includeUnknown,
457458
lower)
458459
} or
460+
TElementContentOfTypeContent(string type, Boolean includeUnknown) {
461+
type = any(Content::KnownElementContent content).getIndex().getValueType()
462+
} or
459463
TNoContentSet() // Only used by type-tracking
460464

461465
cached
462466
class TContentSet =
463467
TSingletonContent or TAnyElementContent or TKnownOrUnknownElementContent or
464-
TElementLowerBoundContent;
468+
TElementLowerBoundContent or TElementContentOfTypeContent;
465469

466470
private predicate trackKnownValue(ConstantValue cv) {
467471
not cv.isFloat(_) and

‎ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -534,6 +534,21 @@ class ContentSet extends TContentSet {
534534
this = TElementLowerBoundContent(lower, true)
535535
}
536536

537+
/**
538+
* Holds if this content set represents all `KnownElementContent`s where
539+
* the index is of type `type`, as per `ConstantValue::getValueType/0`.
540+
*/
541+
predicate isElementOfType(string type) { this = TElementContentOfTypeContent(type, false) }
542+
543+
/**
544+
* Holds if this content set represents `UnknownElementContent` unioned with
545+
* all `KnownElementContent`s where the index is of type `type`, as per
546+
* `ConstantValue::getValueType/0`.
547+
*/
548+
predicate isElementOfTypeOrUnknown(string type) {
549+
this = TElementContentOfTypeContent(type, true)
550+
}
551+
537552
/** Gets a textual representation of this content set. */
538553
string toString() {
539554
exists(Content c |
@@ -558,6 +573,16 @@ class ContentSet extends TContentSet {
558573
includeUnknown = true and
559574
result = lower + ".."
560575
)
576+
or
577+
exists(string type, boolean includeUnknown |
578+
this = TElementContentOfTypeContent(type, includeUnknown)
579+
|
580+
includeUnknown = false and
581+
result = "any(" + type + ")!"
582+
or
583+
includeUnknown = true and
584+
result = "any(" + type + ")"
585+
)
561586
}
562587

563588
/** Gets a content that may be stored into when storing into this set. */
@@ -576,7 +601,14 @@ class ContentSet extends TContentSet {
576601
// step that store only into `1`
577602
this.isKnownOrUnknownElement(result)
578603
or
579-
this.isElementLowerBound(_) and
604+
// These reverse stores are not as accurate as they could be, but making
605+
// them more accurate would result in a large fan-out
606+
(
607+
this.isElementLowerBound(_) or
608+
this.isElementLowerBoundOrUnknown(_) or
609+
this.isElementOfType(_) or
610+
this.isElementOfTypeOrUnknown(_)
611+
) and
580612
result = TUnknownElementContent()
581613
}
582614

@@ -603,6 +635,15 @@ class ContentSet extends TContentSet {
603635
includeUnknown = true and
604636
result = TUnknownElementContent()
605637
)
638+
or
639+
exists(string type, boolean includeUnknown |
640+
this = TElementContentOfTypeContent(type, includeUnknown)
641+
|
642+
type = result.(Content::KnownElementContent).getIndex().getValueType()
643+
or
644+
includeUnknown = true and
645+
result = TUnknownElementContent()
646+
)
606647
}
607648
}
608649

‎ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,6 @@ class Configuration extends TaintTracking::Configuration {
3434
override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet set) {
3535
// allow implicit reads of array elements
3636
this.isSink(node) and
37-
set.isKnownOrUnknownElement(any(DataFlow::Content::KnownElementContent content |
38-
content.getIndex().getValueType() = "int"
39-
))
37+
set.isElementOfTypeOrUnknown("int")
4038
}
4139
}

‎ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,6 @@ class Configuration extends TaintTracking::Configuration {
3636
override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet set) {
3737
// allow implicit reads of array elements
3838
this.isSink(node) and
39-
set.isKnownOrUnknownElement(any(DataFlow::Content::KnownElementContent content |
40-
content.getIndex().getValueType() = "int"
41-
))
39+
set.isElementOfTypeOrUnknown("int")
4240
}
4341
}

‎ruby/ql/lib/codeql/ruby/typetracking/TypeTrackerSpecific.qll

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,8 @@ private ContentFilter getFilterFromWithoutContentStep(DataFlow::ContentSet conte
449449
or
450450
content.isElementLowerBoundOrUnknown(_)
451451
or
452+
content.isElementOfTypeOrUnknown(_)
453+
or
452454
content.isSingleton(any(DataFlow::Content::UnknownElementContent c))
453455
) and
454456
result = MkElementFilter()
@@ -484,6 +486,10 @@ private ContentFilter getFilterFromWithContentStep(DataFlow::ContentSet content)
484486
or
485487
content.isElementLowerBoundOrUnknown(_)
486488
or
489+
content.isElementOfType(_)
490+
or
491+
content.isElementOfTypeOrUnknown(_)
492+
or
487493
content.isSingleton(any(DataFlow::Content::ElementContent c))
488494
) and
489495
result = MkElementFilter()

‎ruby/ql/test/library-tests/dataflow/flow-summaries/semantics.expected

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,26 @@ edges
248248
| semantics.rb:221:14:221:14 | h [element 2] : | semantics.rb:221:10:221:15 | call to s27 |
249249
| semantics.rb:221:14:221:14 | h [element] : | semantics.rb:221:10:221:15 | call to s27 |
250250
| semantics.rb:221:14:221:14 | h [element] : | semantics.rb:221:10:221:15 | call to s27 |
251+
| semantics.rb:225:9:225:18 | call to source : | semantics.rb:226:13:226:13 | a : |
252+
| semantics.rb:225:9:225:18 | call to source : | semantics.rb:226:13:226:13 | a : |
253+
| semantics.rb:226:9:226:14 | call to s28 [element] : | semantics.rb:227:10:227:10 | x [element] : |
254+
| semantics.rb:226:9:226:14 | call to s28 [element] : | semantics.rb:227:10:227:10 | x [element] : |
255+
| semantics.rb:226:9:226:14 | call to s28 [element] : | semantics.rb:228:10:228:10 | x [element] : |
256+
| semantics.rb:226:9:226:14 | call to s28 [element] : | semantics.rb:228:10:228:10 | x [element] : |
257+
| semantics.rb:226:9:226:14 | call to s28 [element] : | semantics.rb:229:10:229:10 | x [element] : |
258+
| semantics.rb:226:9:226:14 | call to s28 [element] : | semantics.rb:229:10:229:10 | x [element] : |
259+
| semantics.rb:226:9:226:14 | call to s28 [element] : | semantics.rb:230:10:230:10 | x [element] : |
260+
| semantics.rb:226:9:226:14 | call to s28 [element] : | semantics.rb:230:10:230:10 | x [element] : |
261+
| semantics.rb:226:13:226:13 | a : | semantics.rb:226:9:226:14 | call to s28 [element] : |
262+
| semantics.rb:226:13:226:13 | a : | semantics.rb:226:9:226:14 | call to s28 [element] : |
263+
| semantics.rb:227:10:227:10 | x [element] : | semantics.rb:227:10:227:13 | ...[...] |
264+
| semantics.rb:227:10:227:10 | x [element] : | semantics.rb:227:10:227:13 | ...[...] |
265+
| semantics.rb:228:10:228:10 | x [element] : | semantics.rb:228:10:228:13 | ...[...] |
266+
| semantics.rb:228:10:228:10 | x [element] : | semantics.rb:228:10:228:13 | ...[...] |
267+
| semantics.rb:229:10:229:10 | x [element] : | semantics.rb:229:10:229:13 | ...[...] |
268+
| semantics.rb:229:10:229:10 | x [element] : | semantics.rb:229:10:229:13 | ...[...] |
269+
| semantics.rb:230:10:230:10 | x [element] : | semantics.rb:230:10:230:13 | ...[...] |
270+
| semantics.rb:230:10:230:10 | x [element] : | semantics.rb:230:10:230:13 | ...[...] |
251271
| semantics.rb:235:9:235:18 | call to source : | semantics.rb:240:5:240:5 | [post] h [element 1] : |
252272
| semantics.rb:235:9:235:18 | call to source : | semantics.rb:240:5:240:5 | [post] h [element 1] : |
253273
| semantics.rb:236:9:236:18 | call to source : | semantics.rb:241:5:241:5 | [post] h [element 2] : |
@@ -1206,6 +1226,28 @@ nodes
12061226
| semantics.rb:221:14:221:14 | h [element 2] : | semmle.label | h [element 2] : |
12071227
| semantics.rb:221:14:221:14 | h [element] : | semmle.label | h [element] : |
12081228
| semantics.rb:221:14:221:14 | h [element] : | semmle.label | h [element] : |
1229+
| semantics.rb:225:9:225:18 | call to source : | semmle.label | call to source : |
1230+
| semantics.rb:225:9:225:18 | call to source : | semmle.label | call to source : |
1231+
| semantics.rb:226:9:226:14 | call to s28 [element] : | semmle.label | call to s28 [element] : |
1232+
| semantics.rb:226:9:226:14 | call to s28 [element] : | semmle.label | call to s28 [element] : |
1233+
| semantics.rb:226:13:226:13 | a : | semmle.label | a : |
1234+
| semantics.rb:226:13:226:13 | a : | semmle.label | a : |
1235+
| semantics.rb:227:10:227:10 | x [element] : | semmle.label | x [element] : |
1236+
| semantics.rb:227:10:227:10 | x [element] : | semmle.label | x [element] : |
1237+
| semantics.rb:227:10:227:13 | ...[...] | semmle.label | ...[...] |
1238+
| semantics.rb:227:10:227:13 | ...[...] | semmle.label | ...[...] |
1239+
| semantics.rb:228:10:228:10 | x [element] : | semmle.label | x [element] : |
1240+
| semantics.rb:228:10:228:10 | x [element] : | semmle.label | x [element] : |
1241+
| semantics.rb:228:10:228:13 | ...[...] | semmle.label | ...[...] |
1242+
| semantics.rb:228:10:228:13 | ...[...] | semmle.label | ...[...] |
1243+
| semantics.rb:229:10:229:10 | x [element] : | semmle.label | x [element] : |
1244+
| semantics.rb:229:10:229:10 | x [element] : | semmle.label | x [element] : |
1245+
| semantics.rb:229:10:229:13 | ...[...] | semmle.label | ...[...] |
1246+
| semantics.rb:229:10:229:13 | ...[...] | semmle.label | ...[...] |
1247+
| semantics.rb:230:10:230:10 | x [element] : | semmle.label | x [element] : |
1248+
| semantics.rb:230:10:230:10 | x [element] : | semmle.label | x [element] : |
1249+
| semantics.rb:230:10:230:13 | ...[...] | semmle.label | ...[...] |
1250+
| semantics.rb:230:10:230:13 | ...[...] | semmle.label | ...[...] |
12091251
| semantics.rb:235:9:235:18 | call to source : | semmle.label | call to source : |
12101252
| semantics.rb:235:9:235:18 | call to source : | semmle.label | call to source : |
12111253
| semantics.rb:236:9:236:18 | call to source : | semmle.label | call to source : |

‎ruby/ql/test/library-tests/dataflow/flow-summaries/semantics.rb

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -224,10 +224,10 @@ def m27(i)
224224
def m28(i)
225225
a = source "a"
226226
x = s28(a)
227-
sink x[0]
228-
sink x[1] # $ MISSING: hasValueFlow=a
229-
sink x[2] # $ MISSING: hasValueFlow=a
230-
sink x[i] # $ MISSING: hasValueFlow=a
227+
sink x[0] # $ SPURIOUS: hasValueFlow=a
228+
sink x[1] # $ hasValueFlow=a
229+
sink x[2] # $ hasValueFlow=a
230+
sink x[i] # $ hasValueFlow=a
231231
end
232232

233233
def m29(i)

0 commit comments

Comments
 (0)