Skip to content

Commit

Permalink
SONARPHP-1057 Adapt existing vulnerabilities to named arguments (#691)
Browse files Browse the repository at this point in the history 
* SONARPHP-1057 Named arguments: adapt EmptyDatabasePasswordCheck

* SONARPHP-1057 Named arguments: adapt HashFunctionCheck

* SONARPHP-1057 Named arguments: adapt LDAPAuthenticatedConnectionCheck

* SONARPHP-1057 Named arguments: adapt NoPaddingRsaCheck

* SONARPHP-1057 Named arguments: adapt SessionCookiePersistenceCheck

* SONARPHP-1057 Named arguments: adapt WeakSSLProtocolCheck

* SONARPHP-1057 FunctionArgumentCheck: further improvement for named arguments

* SONARPHP-1057 Named arguments: adapt EncryptionModeAndPaddingCheck

* SONARPHP-1057 Named arguments: adapt RobustCipherAlgorithmCheck

* SONARPHP-1057 Named arguments: adapt SSLCertificatesVerificationDisabledCheck

* SONARPHP-1057 Named arguments: adapt SSLHostVerificationDisabledCheck
  • Loading branch information
@pynicolas
pynicolas authored Oct 19, 2020
1 parent 9f30d0b commit e7d82ea
Show file tree
Hide file tree
Showing 21 changed files with 149 additions and 67 deletions.
28 changes: 14 additions & 14 deletions php-checks/src/main/java/org/sonar/php/checks/EmptyDatabasePasswordCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,15 @@

import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.regex.Pattern;
import org.sonar.check.Rule;
import org.sonar.php.checks.utils.CheckUtils;
import org.sonar.php.tree.visitors.AssignmentExpressionVisitor;
import org.sonar.plugins.php.api.symbols.Symbol;
import org.sonar.plugins.php.api.tree.CompilationUnitTree;
import org.sonar.plugins.php.api.tree.SeparatedList;
import org.sonar.plugins.php.api.tree.Tree.Kind;
import org.sonar.plugins.php.api.tree.declaration.CallArgumentTree;
import org.sonar.plugins.php.api.tree.expression.ArrayInitializerTree;
import org.sonar.plugins.php.api.tree.expression.ArrayPairTree;
import org.sonar.plugins.php.api.tree.expression.BinaryExpressionTree;
Expand Down Expand Up @@ -58,9 +59,9 @@ public void visitCompilationUnit(CompilationUnitTree tree) {
public void visitFunctionCall(FunctionCallTree functionCall) {
String functionName = CheckUtils.getLowerCaseFunctionName(functionCall);
if ("mysqli".equals(functionName) || "mysqli_connect".equals(functionName) || "PDO".equalsIgnoreCase(functionName)) {
checkPasswordArgument(functionCall, 2);
checkPasswordArgument(functionCall, "passwd", 2);
} else if ("oci_connect".equals(functionName)) {
checkPasswordArgument(functionCall, 1);
checkPasswordArgument(functionCall, "password", 1);
} else if ("sqlsrv_connect".equals(functionName)) {
checkSqlServer(functionCall);
} else if ("pg_connect".equals(functionName)) {
Expand All @@ -69,10 +70,10 @@ public void visitFunctionCall(FunctionCallTree functionCall) {
super.visitFunctionCall(functionCall);
}

private void checkPasswordArgument(FunctionCallTree functionCall, int argumentIndex) {
SeparatedList<ExpressionTree> arguments = functionCall.arguments();
if (arguments.size() > argumentIndex) {
ExpressionTree passwordArgument = arguments.get(argumentIndex);
private void checkPasswordArgument(FunctionCallTree functionCall, String argumentName, int argumentIndex) {
Optional<CallArgumentTree> argument = CheckUtils.argument(functionCall, argumentName, argumentIndex);
if (argument.isPresent()) {
ExpressionTree passwordArgument = argument.get().value();
if (hasEmptyValue(passwordArgument)) {
context().newIssue(this, passwordArgument, MESSAGE);
}
Expand Down Expand Up @@ -101,10 +102,9 @@ private boolean hasEmptyValue(ExpressionTree expression) {
}

private void checkSqlServer(FunctionCallTree functionCall) {
SeparatedList<ExpressionTree> arguments = functionCall.arguments();
int argumentIndex = 1;
if (arguments.size() > argumentIndex) {
ExpressionTree connectionInfo = arguments.get(argumentIndex);
Optional<CallArgumentTree> argument = CheckUtils.argument(functionCall, "connectionInfo", 1);
if (argument.isPresent()) {
ExpressionTree connectionInfo = argument.get().value();
ExpressionTree password = sqlServerPassword(connectionInfo);
if (password != null && hasEmptyValue(password)) {
context().newIssue(this, password, MESSAGE);
Expand All @@ -130,11 +130,11 @@ private ExpressionTree sqlServerPassword(ExpressionTree connectionInfo) {
}

private void checkPostgresql(FunctionCallTree functionCall) {
SeparatedList<ExpressionTree> arguments = functionCall.arguments();
if (arguments.isEmpty()) {
Optional<CallArgumentTree> connectionStringArgument = CheckUtils.argument(functionCall, "connection_string", 0);
if (!connectionStringArgument.isPresent()) {
return;
}
ExpressionTree connectionString = arguments.get(0);
ExpressionTree connectionString = connectionStringArgument.get().value();
Symbol connectionStringSymbol = context().symbolTable().getSymbol(connectionString);
connectionString = assignmentExpressionVisitor
.getUniqueAssignedValue(connectionStringSymbol)
Expand Down
6 changes: 3 additions & 3 deletions php-checks/src/main/java/org/sonar/php/checks/EncryptionModeAndPaddingCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@ private static Set<String> opensslPublicEncryptNoncompliantValues() {
public void visitFunctionCall(FunctionCallTree tree) {
// by default OPENSSL_PKCS1_PADDING is used as padding mode argument
if (!checkArgumentAbsence(tree, OPENSSL_PUBLIC_ENCRYPT, 3)) {
checkArgument(tree, OPENSSL_PUBLIC_ENCRYPT, new ArgumentVerifier(3, OPENSSL_PUBLIC_ENCRYPT_COMPLIANT_VALUE, false));
checkArgument(tree, OPENSSL_PUBLIC_ENCRYPT, new ArgumentVerifier(3, "padding", OPENSSL_PUBLIC_ENCRYPT_COMPLIANT_VALUE, false));
}
checkArgument(tree, OPENSSL_ENCRYPT, new ArgumentVerifier(1, OPENSSL_ENCRYPT_NONCOMPLIANT_VALUES, true));
checkArgument(tree, MCRYPT_ENCRYPT, new ArgumentVerifier(3, MCRYPT_ENCRYPT_NONCOMPLIANT_VALUE, true));
checkArgument(tree, OPENSSL_ENCRYPT, new ArgumentVerifier(1, "method", OPENSSL_ENCRYPT_NONCOMPLIANT_VALUES, true));
checkArgument(tree, MCRYPT_ENCRYPT, new ArgumentVerifier(3, "mode", MCRYPT_ENCRYPT_NONCOMPLIANT_VALUE, true));

super.visitFunctionCall(tree);
}
Expand Down
21 changes: 13 additions & 8 deletions php-checks/src/main/java/org/sonar/php/checks/HashFunctionCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.sonar.plugins.php.api.symbols.Symbol;
import org.sonar.plugins.php.api.tree.CompilationUnitTree;
import org.sonar.plugins.php.api.tree.Tree;
import org.sonar.plugins.php.api.tree.declaration.CallArgumentTree;
import org.sonar.plugins.php.api.tree.expression.ArrayAccessTree;
import org.sonar.plugins.php.api.tree.expression.ArrayInitializerBracketTree;
import org.sonar.plugins.php.api.tree.expression.ArrayPairTree;
Expand All @@ -54,18 +55,22 @@ public class HashFunctionCheck extends PHPVisitorCheck {
@Override
public void visitFunctionCall(FunctionCallTree tree) {
String functionName = CheckUtils.getLowerCaseFunctionName(tree);
if ("hash_pbkdf2".equals(functionName) && tree.arguments().size() >= 3) {
ExpressionTree saltArgument = tree.arguments().get(2);
if (isPredictable(saltArgument)) {
context().newIssue(this, saltArgument, MESSAGE);
if ("hash_pbkdf2".equals(functionName)) {
Optional<CallArgumentTree> saltArgument = CheckUtils.argument(tree, "salt", 2);
if (saltArgument.isPresent()) {
ExpressionTree saltArgumentValue = saltArgument.get().value();
if (isPredictable(saltArgumentValue)) {
context().newIssue(this, saltArgumentValue, MESSAGE);
}
}
} else if ("crypt".equals(functionName)) {
if (tree.arguments().size() < 2) {
Optional<CallArgumentTree> saltArgument = CheckUtils.argument(tree, "salt", 1);
if (!saltArgument.isPresent()) {
context().newIssue(this, tree, MESSAGE_MISSING_SALT);
} else {
ExpressionTree saltArgument = tree.arguments().get(1);
if (isPredictable(saltArgument)) {
context().newIssue(this, saltArgument, MESSAGE);
ExpressionTree saltArgumentValue = saltArgument.get().value();
if (isPredictable(saltArgumentValue)) {
context().newIssue(this, saltArgumentValue, MESSAGE);
}
}
} else if ("password_hash".equals(functionName)) {
Expand Down
8 changes: 6 additions & 2 deletions php-checks/src/main/java/org/sonar/php/checks/NoPaddingRsaCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,12 @@
package org.sonar.php.checks;

import com.google.common.collect.ImmutableSet;
import java.util.Optional;
import org.sonar.check.Rule;
import org.sonar.php.checks.utils.CheckUtils;
import org.sonar.php.checks.utils.FunctionUsageCheck;
import org.sonar.plugins.php.api.tree.Tree.Kind;
import org.sonar.plugins.php.api.tree.declaration.CallArgumentTree;
import org.sonar.plugins.php.api.tree.declaration.NamespaceNameTree;
import org.sonar.plugins.php.api.tree.expression.ExpressionTree;
import org.sonar.plugins.php.api.tree.expression.FunctionCallTree;
Expand All @@ -43,8 +46,9 @@ protected ImmutableSet<String> functionNames() {

@Override
protected void createIssue(FunctionCallTree tree) {
if (tree.arguments().size() > PADDING_ARGUMENT_INDEX) {
ExpressionTree padding = tree.arguments().get(PADDING_ARGUMENT_INDEX);
Optional<CallArgumentTree> paddingArgument = CheckUtils.argument(tree, "padding", PADDING_ARGUMENT_INDEX);
if (paddingArgument.isPresent()) {
ExpressionTree padding = paddingArgument.get().value();
if (padding.is(Kind.NAMESPACE_NAME) && !((NamespaceNameTree) padding).fullName().equals(SECURE_PADDING)) {
context().newIssue(this, padding, MESSAGE);
}
Expand Down
2 changes: 1 addition & 1 deletion php-checks/src/main/java/org/sonar/php/checks/SSLCertificatesVerificationDisabledCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public class SSLCertificatesVerificationDisabledCheck extends FunctionArgumentCh

@Override
public void visitFunctionCall(FunctionCallTree tree) {
checkArgument(tree, CURL_SETOPT, new ArgumentMatcher(1, CURLOPT_SSL_VERIFYPEER), new ArgumentVerifier(2, VERIFY_PEER_COMPLIANT_VALUES));
checkArgument(tree, CURL_SETOPT, new ArgumentMatcher(1, "option", CURLOPT_SSL_VERIFYPEER), new ArgumentVerifier(2, "value", VERIFY_PEER_COMPLIANT_VALUES));

super.visitFunctionCall(tree);
}
Expand Down
2 changes: 1 addition & 1 deletion php-checks/src/main/java/org/sonar/php/checks/SSLHostVerificationDisabledCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public class SSLHostVerificationDisabledCheck extends FunctionArgumentCheck {

@Override
public void visitFunctionCall(FunctionCallTree tree) {
checkArgument(tree, CURL_SETOPT, new ArgumentMatcher(1, CURLOPT_SSL_VERIFYHOST), new ArgumentVerifier(2, VERIFY_HOST_COMPLIANT_VALUES, false));
checkArgument(tree, CURL_SETOPT, new ArgumentMatcher(1, "option", CURLOPT_SSL_VERIFYHOST), new ArgumentVerifier(2, "value", VERIFY_HOST_COMPLIANT_VALUES, false));

super.visitFunctionCall(tree);
}
Expand Down
18 changes: 10 additions & 8 deletions php-checks/src/main/java/org/sonar/php/checks/SessionCookiePersistenceCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,17 @@
import com.google.common.collect.ImmutableSet;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import org.sonar.check.Rule;
import org.sonar.php.checks.utils.CheckUtils;
import org.sonar.php.checks.utils.FunctionUsageCheck;
import org.sonar.php.ini.BasePhpIniIssue;
import org.sonar.php.ini.PhpIniCheck;
import org.sonar.php.ini.PhpIniIssue;
import org.sonar.php.ini.tree.Directive;
import org.sonar.php.ini.tree.PhpIniFile;
import org.sonar.plugins.php.api.tree.SeparatedList;
import org.sonar.plugins.php.api.tree.Tree.Kind;
import org.sonar.plugins.php.api.tree.declaration.CallArgumentTree;
import org.sonar.plugins.php.api.tree.expression.ExpressionTree;
import org.sonar.plugins.php.api.tree.expression.FunctionCallTree;
import org.sonar.plugins.php.api.tree.expression.LiteralTree;
Expand All @@ -39,7 +41,7 @@
public class SessionCookiePersistenceCheck extends FunctionUsageCheck implements PhpIniCheck {

private static final String PHP_INI_MESSAGE = "Configure \"session.cookie_lifetime\" to 0.";
private static final String PHP_CODE_MESSAGE = "Pass \"0\" as first argument.";
private static final String PHP_CODE_MESSAGE = "Set \"lifetime\" parameter to \"0\".";

@Override
public List<PhpIniIssue> analyze(PhpIniFile phpIniFile) {
Expand All @@ -60,13 +62,13 @@ protected ImmutableSet<String> functionNames() {

@Override
protected void createIssue(FunctionCallTree functionCall) {
SeparatedList<ExpressionTree> arguments = functionCall.arguments();
if (!arguments.isEmpty()) {
ExpressionTree firstArgument = arguments.get(0);
if (firstArgument.is(Kind.NUMERIC_LITERAL)) {
LiteralTree literal = (LiteralTree) firstArgument;
Optional<CallArgumentTree> lifetimeArgument = CheckUtils.argument(functionCall, "lifetime", 0);
if (lifetimeArgument.isPresent()) {
ExpressionTree lifetimeArgumentValue = lifetimeArgument.get().value();
if (lifetimeArgumentValue.is(Kind.NUMERIC_LITERAL)) {
LiteralTree literal = (LiteralTree) lifetimeArgumentValue;
if (!"0".equals(literal.value())) {
context().newIssue(this, firstArgument, PHP_CODE_MESSAGE);
context().newIssue(this, lifetimeArgumentValue, PHP_CODE_MESSAGE);
}
}
}
Expand Down
23 changes: 15 additions & 8 deletions php-checks/src/main/java/org/sonar/php/checks/WeakSSLProtocolCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.sonar.plugins.php.api.symbols.Symbol;
import org.sonar.plugins.php.api.tree.CompilationUnitTree;
import org.sonar.plugins.php.api.tree.Tree;
import org.sonar.plugins.php.api.tree.declaration.CallArgumentTree;
import org.sonar.plugins.php.api.tree.declaration.NamespaceNameTree;
import org.sonar.plugins.php.api.tree.expression.ArrayInitializerTree;
import org.sonar.plugins.php.api.tree.expression.BinaryExpressionTree;
Expand Down Expand Up @@ -94,16 +95,22 @@ public void visitCompilationUnit(CompilationUnitTree tree) {
public void visitFunctionCall(FunctionCallTree tree) {
String functionName = CheckUtils.getLowerCaseFunctionName(tree);
List<ExpressionTree> arguments = tree.arguments();
if (STREAM_CONTEXT_CREATE.equals(functionName) && !arguments.isEmpty()) {
checkStreamSSLConfig(arguments.get(0));
if (STREAM_CONTEXT_CREATE.equals(functionName)) {
CheckUtils.argument(tree, "options", 0).ifPresent(
options -> checkStreamSSLConfig(options.value()));
}
if (STREAM_SOCKET_ENABLE_CRYPTO.equals(functionName) && arguments.size() > 2) {
checkStreamWeakProtocol(getAssignedValue(arguments.get(2)), STREAM_SOCKET_ENABLE_CRYPTO);
if (STREAM_SOCKET_ENABLE_CRYPTO.equals(functionName)) {
CheckUtils.argument(tree, "crypto_type", 2).ifPresent(
cryptoType -> checkStreamWeakProtocol(getAssignedValue(cryptoType.value()), STREAM_SOCKET_ENABLE_CRYPTO));
}
if (CURL_SETOPT.equals(functionName) && arguments.size() > 2) {
ExpressionTree optionArgument = arguments.get(1);
if (optionArgument.is(Tree.Kind.NAMESPACE_NAME) && "CURLOPT_SSLVERSION".equals(((NamespaceNameTree) optionArgument).name().text())) {
checkCURLWeakProtocol(getAssignedValue(arguments.get(2)));
if (CURL_SETOPT.equals(functionName)) {
Optional<CallArgumentTree> optionArgument = CheckUtils.argument(tree, "option", 1);
Optional<CallArgumentTree> valueArgument = CheckUtils.argument(tree, "value", 2);
if (optionArgument.isPresent() && valueArgument.isPresent()) {
ExpressionTree optionArgumentValue = optionArgument.get().value();
if (optionArgumentValue.is(Tree.Kind.NAMESPACE_NAME) && "CURLOPT_SSLVERSION".equals(((NamespaceNameTree) optionArgumentValue).name().text())) {
checkCURLWeakProtocol(getAssignedValue(valueArgument.get().value()));
}
}
}
super.visitFunctionCall(tree);
Expand Down
15 changes: 8 additions & 7 deletions php-checks/src/main/java/org/sonar/php/checks/security/LDAPAuthenticatedConnectionCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,22 @@
package org.sonar.php.checks.security;

import com.google.common.collect.ImmutableSet;
import java.util.Optional;
import org.sonar.check.Rule;
import org.sonar.php.checks.utils.CheckUtils;
import org.sonar.php.checks.utils.FunctionUsageCheck;
import org.sonar.php.tree.visitors.AssignmentExpressionVisitor;
import org.sonar.plugins.php.api.symbols.Symbol;
import org.sonar.plugins.php.api.tree.CompilationUnitTree;
import org.sonar.plugins.php.api.tree.Tree;
import org.sonar.plugins.php.api.tree.declaration.CallArgumentTree;
import org.sonar.plugins.php.api.tree.expression.ExpressionTree;
import org.sonar.plugins.php.api.tree.expression.FunctionCallTree;

@Rule(key = "S4433")
public class LDAPAuthenticatedConnectionCheck extends FunctionUsageCheck {

private static final String MESSAGE = "Provide username and password to authenticate the connection.";
private static final int USERNAME_ARG_INDEX = 1;
private static final int PASSWORD_ARG_INDEX = 2;
private AssignmentExpressionVisitor assignmentExpressionVisitor;

@Override
Expand All @@ -52,15 +52,16 @@ public void visitCompilationUnit(CompilationUnitTree tree) {

@Override
protected void createIssue(FunctionCallTree tree) {
if (argumentIsNullOrEmptyString(tree, USERNAME_ARG_INDEX) || argumentIsNullOrEmptyString(tree, PASSWORD_ARG_INDEX)) {
if (argumentIsNullOrEmptyString(tree, "bind_rdn", 1) || argumentIsNullOrEmptyString(tree, "bind_password", 2)) {
context().newIssue(this, tree, MESSAGE);
}
}

private boolean argumentIsNullOrEmptyString(FunctionCallTree tree, int argumentIndex) {
if (tree.arguments().size() > argumentIndex) {
ExpressionTree valueArgument = getAssignedValue(tree.arguments().get(argumentIndex));
return CheckUtils.isNullOrEmptyString(valueArgument);
private boolean argumentIsNullOrEmptyString(FunctionCallTree tree, String argumentName, int argumentIndex) {
Optional<CallArgumentTree> argument = CheckUtils.argument(tree, argumentName, argumentIndex);
if (argument.isPresent()) {
ExpressionTree argumentValue = getAssignedValue(argument.get().value());
return CheckUtils.isNullOrEmptyString(argumentValue);
}
return true;
}
Expand Down
4 changes: 2 additions & 2 deletions php-checks/src/main/java/org/sonar/php/checks/security/RobustCipherAlgorithmCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public class RobustCipherAlgorithmCheck extends FunctionArgumentCheck {

@Override
public void visitFunctionCall(FunctionCallTree tree) {
checkArgument(tree, "mcrypt_encrypt", new ArgumentVerifier(0, ImmutableSet.of(
checkArgument(tree, "mcrypt_encrypt", new ArgumentVerifier(0, "cipher", ImmutableSet.of(
"mcrypt_des",
"mcrypt_des_compat",
"mcrypt_tripledes",
Expand All @@ -40,7 +40,7 @@ public void visitFunctionCall(FunctionCallTree tree) {
"mcrypt_rc2",
"mcrypt_rc4")));

checkArgument(tree, "openssl_encrypt", new ArgumentVerifier(1, ImmutableSet.of(
checkArgument(tree, "openssl_encrypt", new ArgumentVerifier(1, "method", ImmutableSet.of(
"bf-ecb",
"des-ede3",
"des-ofb",
Expand Down
10 changes: 10 additions & 0 deletions php-checks/src/main/java/org/sonar/php/checks/utils/FunctionArgumentCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,11 +210,21 @@ public ArgumentVerifier(int position, String value, boolean raiseIssueOnMatch) {
this.raiseIssueOnMatch = raiseIssueOnMatch;
}

public ArgumentVerifier(int position, String name, String value, boolean raiseIssueOnMatch) {
this(position, name, ImmutableSet.of(value));
this.raiseIssueOnMatch = raiseIssueOnMatch;
}

public ArgumentVerifier(int position, Set<String> values, boolean raiseIssueOnMatch) {
super(position, values);
this.raiseIssueOnMatch = raiseIssueOnMatch;
}

public ArgumentVerifier(int position, String name, Set<String> values, boolean raiseIssueOnMatch) {
super(position, name, values);
this.raiseIssueOnMatch = raiseIssueOnMatch;
}

@VisibleForTesting
boolean isRaiseIssueOnMatch() {
return raiseIssueOnMatch;
Expand Down
Loading

0 comments on commit e7d82ea

Please sign in to comment.