Skip to content

Commit e0391b7

Browse files
Jami CogswellJami Cogswell
Jami Cogswell
authored and
Jami Cogswell
committed
Java: clean-up refactoring
1 parent 6b268c4 commit e0391b7

File tree

2 files changed

+84
-217
lines changed

2 files changed

+84
-217
lines changed

‎java/ql/lib/semmle/code/java/JDK.qll‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,36 @@ class StringContainsMethod extends Method {
4545
}
4646
}
4747

48+
/** A call to the `java.lang.String.matches` method. */
49+
class StringMatchesCall extends MethodCall {
50+
StringMatchesCall() {
51+
exists(Method m | m = this.getMethod() |
52+
m.getDeclaringType() instanceof TypeString and
53+
m.hasName("matches")
54+
)
55+
}
56+
}
57+
58+
/** A call to the `java.lang.String.replaceAll` method. */
59+
class StringReplaceAllCall extends MethodCall {
60+
StringReplaceAllCall() {
61+
exists(Method m | m = this.getMethod() |
62+
m.getDeclaringType() instanceof TypeString and
63+
m.hasName("replaceAll")
64+
)
65+
}
66+
}
67+
68+
/** A call to the `java.lang.String.replace` method. */
69+
class StringReplaceCall extends MethodCall {
70+
StringReplaceCall() {
71+
exists(Method m | m = this.getMethod() |
72+
m.getDeclaringType() instanceof TypeString and
73+
m.hasName("replace")
74+
)
75+
}
76+
}
77+
4878
/**
4979
* The methods on the class `java.lang.String` that are used to perform partial matches with a specified substring or char.
5080
*/

‎java/ql/lib/semmle/code/java/security/PathSanitizer.qll‎

Lines changed: 54 additions & 217 deletions
Original file line numberDiff line numberDiff line change
@@ -384,152 +384,80 @@ private class FileConstructorChildArgumentStep extends AdditionalTaintStep {
384384
}
385385
}
386386

387-
private class ReplaceAllCall extends MethodCall {
388-
ReplaceAllCall() {
389-
exists(Method m | m = this.getMethod() |
390-
m.getDeclaringType() instanceof TypeString and
391-
m.hasName("replaceAll")
392-
)
393-
}
394-
}
395-
396-
private class ReplaceCall extends MethodCall {
397-
ReplaceCall() {
398-
exists(Method m | m = this.getMethod() |
399-
m.getDeclaringType() instanceof TypeString and
400-
m.hasName("replace")
401-
)
402-
}
403-
}
404-
405-
private class ReplaceOrReplaceAllCall extends MethodCall {
406-
ReplaceOrReplaceAllCall() {
407-
this instanceof ReplaceCall or
408-
this instanceof ReplaceAllCall
387+
/** A call to `java.lang.String.replace` or `java.lang.String.replaceAll`. */
388+
private class StringReplaceOrReplaceAllCall extends MethodCall {
389+
StringReplaceOrReplaceAllCall() {
390+
this instanceof StringReplaceCall or
391+
this instanceof StringReplaceAllCall
409392
}
410393
}
411394

395+
/** Gets a character used for replacement */
412396
private string getAReplacementChar() { result = ["", "_", "-"] }
413397

398+
/** Gets a directory character represented in regex. */
414399
private string getADirRegexChar() { result = ["\\.", "/", "\\\\"] }
415400

401+
/** Gets a directory character represented as a char. */
416402
private string getADirChar() { result = [".", "/", "\\"] }
417403

418-
// private predicate isSingleReplaceAll(CompileTimeConstantExpr target) {
419-
// not target.getStringValue().matches("%[^%]%") and
420-
// target.getStringValue().matches("[%.%]") and
421-
// target.getStringValue().matches("[%/%]") and
422-
// // ! eight slashes since need to avoid escaping the '%'
423-
// target.getStringValue().matches("[%\\\\\\\\%]")
424-
// or
425-
// target.getStringValue().matches("%|%") and
426-
// //target.getStringValue().matches("%" + ["\\.\\.", "[.][.]", "\\."] + "%") and
427-
// target.getStringValue().regexpMatch(".*(\\\\\\.\\\\\\.|\\[.\\]\\[.\\]|\\\\\\.).*") and
428-
// target.getStringValue().matches("%/%") and
429-
// target.getStringValue().matches("%\\\\\\\\%")
430-
// }
431-
// private predicate isMultipleReplaces(
432-
// MethodCall mc1, CompileTimeConstantExpr target1, string targetValue1, MethodCall mc2,
433-
// CompileTimeConstantExpr target2, string targetValue2
434-
// ) {
435-
// mc2.getQualifier() = mc1 and
436-
// target1 = mc1.getArgument(0) and
437-
// target1.getStringValue() = targetValue1 and
438-
// target2 = mc2.getArgument(0) and
439-
// target2.getStringValue() = targetValue2 and
440-
// mc2.getArgument(1).(CompileTimeConstantExpr).getStringValue() = getAReplacementChar() and
441-
// // make sure the calls replace different characters
442-
// targetValue2 != targetValue1 and
443-
// // make sure one of the calls replaces '.'
444-
// // then the other call must replace one of '/' or '\' if they are not equal
445-
// (targetValue2.matches("%.%") or targetValue1.matches("%.%"))
446-
// }
447-
private predicate replaceAll(ReplaceAllCall mc, CompileTimeConstantExpr target) {
448-
target = mc.getArgument(0)
449-
}
450-
451-
private predicate isSingleReplaceAll(ReplaceAllCall mc) {
404+
/** Holds if `target` is the first argument of `replaceAllCall`. */
405+
private predicate isReplaceAllTarget(
406+
StringReplaceAllCall replaceAllCall, CompileTimeConstantExpr target
407+
) {
408+
target = replaceAllCall.getArgument(0)
409+
}
410+
411+
/** Holds if `target` is the first argument of `replaceCall`. */
412+
private predicate isReplaceTarget(StringReplaceCall replaceCall, CompileTimeConstantExpr target) {
413+
target = replaceCall.getArgument(0)
414+
}
415+
416+
/** Holds if a single `replaceAllCall` replaces all directory characters. */
417+
private predicate isSingleReplaceAll(StringReplaceAllCall replaceAllCall) {
452418
exists(CompileTimeConstantExpr target, string targetValue |
453-
replaceAll(mc, target) and
419+
isReplaceAllTarget(replaceAllCall, target) and
454420
target.getStringValue() = targetValue
455421
|
456422
not targetValue.matches("%[^%]%") and
457423
targetValue.matches("[%.%]") and
458424
targetValue.matches("[%/%]") and
459-
// ! eight slashes since need to avoid escaping the '%'
425+
// Search for "\\\\" (needs extra backslashes to avoid escaping the '%')
460426
targetValue.matches("[%\\\\\\\\%]")
461427
or
462428
targetValue.matches("%|%") and
463-
//target.getStringValue().matches("%" + ["\\.\\.", "[.][.]", "\\."] + "%") and
464-
targetValue.regexpMatch(".*(\\\\\\.\\\\\\.|\\[.\\]\\[.\\]|\\\\\\.).*") and
429+
target.getStringValue().matches("%" + ["\\.\\.", "[.][.]", "\\."] + "%") and
430+
//targetValue.regexpMatch(".*(\\\\\\.\\\\\\.|\\[.\\]\\[.\\]|\\\\\\.).*") and
465431
targetValue.matches("%/%") and
466432
targetValue.matches("%\\\\\\\\%")
467433
)
468434
}
469435

470-
// private predicate isMultipleReplaceAll(ReplaceAllCall mc1) {
471-
// exists(
472-
// CompileTimeConstantExpr target1, string targetValue1, ReplaceAllCall mc2,
473-
// CompileTimeConstantExpr target2, string targetValue2
474-
// |
475-
// replaceAll(mc1, target1) and
476-
// replaceAll(mc2, target2) and
477-
// target1.getStringValue() = targetValue1 and
478-
// target2.getStringValue() = targetValue2 and
479-
// targetValue1 = getADirRegexChar() and
480-
// targetValue2 = getADirRegexChar() and
481-
// mc2.getArgument(1).(CompileTimeConstantExpr).getStringValue() = getAReplacementChar() and
482-
// // make sure the calls replace different characters
483-
// targetValue2 != targetValue1 and
484-
// // make sure one of the calls replaces '.'
485-
// // then the other call must replace one of '/' or '\' if they are not equal
486-
// (targetValue2.matches("%.%") or targetValue1.matches("%.%"))
487-
// )
488-
// }
489-
private predicate replace(ReplaceCall mc, CompileTimeConstantExpr target) {
490-
target = mc.getArgument(0)
491-
}
492-
493-
// private predicate isMultipleReplace(ReplaceCall mc1) {
494-
// exists(
495-
// CompileTimeConstantExpr target1, string targetValue1, ReplaceCall mc2,
496-
// CompileTimeConstantExpr target2, string targetValue2
497-
// |
498-
// replace(mc1, target1) and
499-
// replace(mc2, target2) and
500-
// target1.getStringValue() = targetValue1 and
501-
// target2.getStringValue() = targetValue2 and
502-
// targetValue1 = getADirChar() and
503-
// targetValue2 = getADirChar() and
504-
// mc2.getArgument(1).(CompileTimeConstantExpr).getStringValue() = getAReplacementChar() and
505-
// // make sure the calls replace different characters
506-
// targetValue2 != targetValue1 and
507-
// // make sure one of the calls replaces '.'
508-
// // then the other call must replace one of '/' or '\' if they are not equal
509-
// (targetValue2.matches("%.%") or targetValue1.matches("%.%"))
510-
// )
511-
// }
512-
private predicate isMultipleReplaceOrReplaceAll(ReplaceOrReplaceAllCall mc1) {
436+
/**
437+
* Holds if there are two chained replacement calls, `rc1` and `rc2`, that replace
438+
* '.' and one of '/' or '\'.
439+
*/
440+
private predicate isDoubleReplaceOrReplaceAll(StringReplaceOrReplaceAllCall rc1) {
513441
exists(
514-
CompileTimeConstantExpr target1, string targetValue1, ReplaceOrReplaceAllCall mc2,
442+
CompileTimeConstantExpr target1, string targetValue1, StringReplaceOrReplaceAllCall rc2,
515443
CompileTimeConstantExpr target2, string targetValue2
516444
|
517-
mc1 instanceof ReplaceAllCall and
518-
replaceAll(mc1, target1) and
519-
replaceAll(mc2, target2) and
445+
rc1 instanceof StringReplaceAllCall and
446+
isReplaceAllTarget(rc1, target1) and
447+
isReplaceAllTarget(rc2, target2) and
520448
targetValue1 = getADirRegexChar() and
521449
targetValue2 = getADirRegexChar()
522450
or
523-
mc1 instanceof ReplaceCall and
524-
replace(mc1, target1) and
525-
replace(mc2, target2) and
451+
rc1 instanceof StringReplaceCall and
452+
isReplaceTarget(rc1, target1) and
453+
isReplaceTarget(rc2, target2) and
526454
targetValue1 = getADirChar() and
527455
targetValue2 = getADirChar()
528456
|
529-
mc2.getQualifier() = mc1 and
457+
rc2.getQualifier() = rc1 and
530458
target1.getStringValue() = targetValue1 and
531459
target2.getStringValue() = targetValue2 and
532-
mc2.getArgument(1).(CompileTimeConstantExpr).getStringValue() = getAReplacementChar() and
460+
rc2.getArgument(1).(CompileTimeConstantExpr).getStringValue() = getAReplacementChar() and
533461
// make sure the calls replace different characters
534462
targetValue2 != targetValue1 and
535463
// make sure one of the calls replaces '.'
@@ -542,86 +470,27 @@ private predicate isMultipleReplaceOrReplaceAll(ReplaceOrReplaceAllCall mc1) {
542470
* A complementary sanitizer that protects against path injection vulnerabilities
543471
* by replacing directory characters ('..', '/', and '\') with safe characters.
544472
*/
545-
private class ReplaceDirectoryCharactersSanitizer extends ReplaceOrReplaceAllCall {
473+
private class ReplaceDirectoryCharactersSanitizer extends StringReplaceOrReplaceAllCall {
546474
ReplaceDirectoryCharactersSanitizer() {
547-
isSingleReplaceAll(this)
548-
or
549-
// isMultipleReplaceAll(this) or
550-
// isMultipleReplace(this)
551-
isMultipleReplaceOrReplaceAll(this)
552-
// exists(MethodCall mc, CompileTimeConstantExpr target |
553-
// this = mc and
554-
// target = mc.getArgument(0) and
555-
// mc.getArgument(1).(CompileTimeConstantExpr).getStringValue() = getAReplacementChar() and
556-
// (
557-
// // `replaceAll` with regex
558-
// mc instanceof ReplaceAllCall and
559-
// (
560-
// isSingleReplaceAll(target)
561-
// or
562-
// exists(ReplaceAllCall rac, CompileTimeConstantExpr racTarget |
563-
// isMultipleReplaces(mc, target, getADirRegexChar(), rac, racTarget, getADirRegexChar())
564-
// )
565-
// )
566-
// or
567-
// // `replace` with char/CharSequence
568-
// mc instanceof ReplaceCall and
569-
// exists(ReplaceCall rc, CompileTimeConstantExpr rcTarget |
570-
// isMultipleReplaces(mc, target, getADirChar(), rc, rcTarget, getADirChar())
571-
// )
572-
// )
573-
// )
475+
isSingleReplaceAll(this) or
476+
isDoubleReplaceOrReplaceAll(this)
574477
}
575478
}
576479

577-
// /**
578-
// * A complementary sanitizer that protects against path injection vulnerabilities
579-
// * by replacing directory characters ('..', '/', and '\') with safe characters.
580-
// */
581-
// private class ReplaceDirectoryCharactersSanitizer extends MethodCall {
582-
// ReplaceDirectoryCharactersSanitizer() {
583-
// exists(MethodCall mc, CompileTimeConstantExpr target |
584-
// this = mc and
585-
// target = mc.getArgument(0) and
586-
// mc.getArgument(1).(CompileTimeConstantExpr).getStringValue() = getAReplacementChar() and
587-
// (
588-
// // `replaceAll` with regex
589-
// mc instanceof ReplaceAllCall and
590-
// (
591-
// isSingleReplaceAll(target)
592-
// or
593-
// exists(ReplaceAllCall rac, CompileTimeConstantExpr racTarget |
594-
// isMultipleReplaces(mc, target, getADirRegexChar(), rac, racTarget, getADirRegexChar())
595-
// )
596-
// )
597-
// or
598-
// // `replace` with char/CharSequence
599-
// mc instanceof ReplaceCall and
600-
// exists(ReplaceCall rc, CompileTimeConstantExpr rcTarget |
601-
// isMultipleReplaces(mc, target, getADirChar(), rc, rcTarget, getADirChar())
602-
// )
603-
// )
604-
// )
605-
// }
606-
// }
607-
private class MatchesCall extends MethodCall {
608-
MatchesCall() {
609-
exists(Method m | m = this.getMethod() |
610-
m.getDeclaringType() instanceof TypeString and
611-
m.hasName("matches")
612-
)
613-
}
480+
/** Holds if `target` is the first argument of `matchesCall`. */
481+
private predicate isMatchesTarget(StringMatchesCall matchesCall, CompileTimeConstantExpr target) {
482+
target = matchesCall.getArgument(0)
614483
}
615484

616-
private predicate matches(MatchesCall mc, CompileTimeConstantExpr target) {
617-
target = mc.getArgument(0)
618-
}
619-
620-
private predicate isMatchesTarget(MatchesCall mc, Expr checkedExpr, boolean branch) {
485+
/**
486+
* Holds if `matchesCall` confirms that `checkedExpr` does not contain any directory characters
487+
* on the given `branch`.
488+
*/
489+
private predicate isMatchesCall(StringMatchesCall matchesCall, Expr checkedExpr, boolean branch) {
621490
exists(CompileTimeConstantExpr target, string targetValue |
622-
matches(mc, target) and
491+
isMatchesTarget(matchesCall, target) and
623492
target.getStringValue() = targetValue and
624-
checkedExpr = mc.getQualifier()
493+
checkedExpr = matchesCall.getQualifier()
625494
|
626495
targetValue.regexpMatch("\\[(.*)\\]([*+]|\\{.*\\})") and
627496
(
@@ -656,39 +525,7 @@ private class DirectoryCharactersGuard extends PathGuard {
656525
Expr checkedExpr;
657526
boolean branch;
658527

659-
DirectoryCharactersGuard() {
660-
isMatchesTarget(this, checkedExpr, branch)
661-
// exists(MatchesCall mc, CompileTimeConstantExpr target, string targetValue |
662-
// target = mc.getArgument(0) and
663-
// target.getStringValue() = targetValue and
664-
// checkedExpr = mc.getQualifier() and
665-
// this = mc
666-
// |
667-
// //targetValue.matches(["[%]*", "[%]+", "[%]{%}"]) and
668-
// targetValue.regexpMatch("\\[(.*)\\]([*+]|\\{.*\\})") and
669-
// (
670-
// // Allow anything except `.`, '/', '\'
671-
// (
672-
// // Note: we do not account for when '.', '/', '\' are inside a character range
673-
// // not targetValue.matches("[%" + [".", "/", "\\\\"] + "%]%") and
674-
// not targetValue.regexpMatch("\\[.*(\\.|\\\\|/).*\\].*") and
675-
// not targetValue.matches("%[^%]%")
676-
// or
677-
// targetValue.matches("[^%.%]%") and
678-
// targetValue.matches("[^%/%]%") and
679-
// targetValue.matches("[^%\\\\\\\\%]%")
680-
// ) and
681-
// branch = true
682-
// or
683-
// // Disallow `.`, '/', '\'
684-
// targetValue.matches("[%.%]%") and
685-
// targetValue.matches("[%/%]%") and
686-
// targetValue.matches("[%\\\\\\\\%]%") and
687-
// not targetValue.matches("%[^%]%") and
688-
// branch = false
689-
// )
690-
// )
691-
}
528+
DirectoryCharactersGuard() { isMatchesCall(this, checkedExpr, branch) }
692529

693530
override Expr getCheckedExpr() { result = checkedExpr }
694531

0 commit comments

Comments
 (0)