Added missing "GOOD" and "BAD" to some examples

Author: fabienpe

java/ql/src/Security/CWE/CWE-020/ExternalAPITaintStepExample.java

@@ -4,6 +4,7 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response)
 
 		StringBuilder sqlQueryBuilder = new StringBuilder();
 		sqlQueryBuilder.append("SELECT * FROM user WHERE user_id='");
+		// BAD: a request parameter is concatenated directly into a SQL query
 		sqlQueryBuilder.append(request.getParameter("user_id"));
 		sqlQueryBuilder.append("'");
 

java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java

@@ -1,6 +1,6 @@
 public class PartialPathTraversalBad {
     public void example(File dir, File parent) throws IOException {
-        if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath())) {
+        if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath())) { // BAD: dir.getCanonicalPath() not slash-terminated
             throw new IOException("Path traversal attempt: " + dir.getCanonicalPath());
         }
     }

java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java

@@ -2,7 +2,7 @@
 
 public class PartialPathTraversalGood {
     public void example(File dir, File parent) throws IOException {
-        if (!dir.toPath().normalize().startsWith(parent.toPath())) {
+        if (!dir.toPath().normalize().startsWith(parent.toPath())) {  // GOOD: Check if dir.Path() is normalised
             throw new IOException("Path traversal attempt: " + dir.getCanonicalPath());
         }
     }

java/ql/src/Security/CWE/CWE-079/AndroidWebViewAddJavascriptInterfaceExample.java

@@ -20,4 +20,4 @@ public String studentEmail(String studentName) {
 webview.loadData("", "text/html", null);
 
 String name = "Robert'; DROP TABLE students; --";
-webview.loadUrl("javascript:alert(exposedObject.studentEmail(\""+ name +"\"))");
+webview.loadUrl("javascript:alert(exposedObject.studentEmail(\""+ name +"\"))"); // BAD: Untrusted input loaded into WebView

java/ql/src/Security/CWE/CWE-079/WebSettingsDisableJavascript.java

@@ -1,2 +1,2 @@
 WebSettings settings = webview.getSettings();
-settings.setJavaScriptEnabled(false);
+settings.setJavaScriptEnabled(false); // GOOD: webview has JavaScript disabled

java/ql/src/Security/CWE/CWE-079/WebSettingsEnableJavascript.java

@@ -1,2 +1,2 @@
 WebSettings settings = webview.getSettings();
-settings.setJavaScriptEnabled(true);
+settings.setJavaScriptEnabled(true); // BAD: webview has JavaScript enabled

java/ql/src/Security/CWE/CWE-094/GroovyInjectionBad.java

@@ -2,26 +2,26 @@ public class GroovyInjection {
     void injectionViaClassLoader(HttpServletRequest request) {    
         String script = request.getParameter("script");
         final GroovyClassLoader classLoader = new GroovyClassLoader();
-        Class groovy = classLoader.parseClass(script);
+        Class groovy = classLoader.parseClass(script); // BAD: Groovy code injection
         GroovyObject groovyObj = (GroovyObject) groovy.newInstance();
     }
 
     void injectionViaEval(HttpServletRequest request) {
         String script = request.getParameter("script");
-        Eval.me(script);
+        Eval.me(script); // BAD: Groovy code injection
     }
 
     void injectionViaGroovyShell(HttpServletRequest request) {
         GroovyShell shell = new GroovyShell();
         String script = request.getParameter("script");
-        shell.evaluate(script);
+        shell.evaluate(script); // BAD: Groovy code injection
     }
 
     void injectionViaGroovyShellGroovyCodeSource(HttpServletRequest request) {
         GroovyShell shell = new GroovyShell();
         String script = request.getParameter("script");
         GroovyCodeSource gcs = new GroovyCodeSource(script, "test", "Test");
-        shell.evaluate(gcs);
+        shell.evaluate(gcs); // BAD: Groovy code injection
     }
 }
 

java/ql/src/Security/CWE/CWE-094/InstallApkWithFile.java

@@ -9,6 +9,6 @@
 File file = new File(Environment.getExternalStorageDirectory(), "myapp.apk");
 Intent intent = new Intent(Intent.ACTION_VIEW);
 /* Set the mimetype to APK */
-intent.setDataAndType(Uri.fromFile(file), "application/vnd.android.package-archive");
+intent.setDataAndType(Uri.fromFile(file), "application/vnd.android.package-archive"); // BAD: The file may be altered by another app
 
 startActivity(intent);

java/ql/src/Security/CWE/CWE-094/InstallApkWithFileProvider.java

@@ -21,7 +21,7 @@
 
 /* Expose temporary file with FileProvider */
 File toInstall = new File(this.getFilesDir(), tempFilename);
-Uri applicationUri = FileProvider.getUriForFile(this, "com.example.apkprovider", toInstall);
+Uri applicationUri = FileProvider.getUriForFile(this, "com.example.apkprovider", toInstall); // GOOD: The file is protected by FileProvider
 
 /* Create Intent and set data to APK file. */
 Intent intent = new Intent(Intent.ACTION_INSTALL_PACKAGE);

java/ql/src/Security/CWE/CWE-094/InstallApkWithPackageInstaller.java

@@ -1,3 +1,4 @@
+// GOOD: Package installed using PackageInstaller
 import android.content.Context;
 import android.content.Intent;
 import android.content.pm.PackageInstaller;

java/ql/src/Security/CWE/CWE-094/SSTIBad.java

@@ -14,6 +14,6 @@ public void bad(HttpServletRequest request) {
 
 		StringWriter w = new StringWriter();
 		// evaluate( Context context, Writer out, String logTag, String instring )
-		Velocity.evaluate(context, w, "mystring", code);
+		Velocity.evaluate(context, w, "mystring", code); // BAD: code is controlled by the user
 	}
 }

java/ql/src/Security/CWE/CWE-094/SSTIGood.java

@@ -11,7 +11,7 @@ public void good(HttpServletRequest request) {
 
 		String s = "We are using $project $name to render this.";
 		StringWriter w = new StringWriter();
-		Velocity.evaluate(context, w, "mystring", s);
+		Velocity.evaluate(context, w, "mystring", s); // GOOD: s is a constant string
 		System.out.println(" string : " + w);
 	}
 }

java/ql/src/Security/CWE/CWE-094/SaferJexlExpressionEvaluationWithSandbox.java

@@ -4,7 +4,7 @@ public void evaluate(Socket socket) throws IOException {
 
     JexlSandbox onlyMath = new JexlSandbox(false);
     onlyMath.white("java.lang.Math");
-    JexlEngine jexl = new JexlBuilder().sandbox(onlyMath).create();
+    JexlEngine jexl = new JexlBuilder().sandbox(onlyMath).create(); // GOOD: using a sandbox
 
     String input = reader.readLine();
     JexlExpression expression = jexl.createExpression(input);

java/ql/src/Security/CWE/CWE-094/SaferJexlExpressionEvaluationWithUberspectSandbox.java

@@ -6,7 +6,7 @@ public void evaluate(Socket socket) throws IOException {
     JexlEngine jexl = new JexlBuilder().uberspect(sandbox).create();
 
     String input = reader.readLine();
-    JexlExpression expression = jexl.createExpression(input);
+    JexlExpression expression = jexl.createExpression(input); // GOOD: jexl uses a sandbox
     JexlContext context = new MapContext();
     expression.evaluate(context);
   }

java/ql/src/Security/CWE/CWE-094/SaferSpelExpressionEvaluation.java

@@ -4,9 +4,9 @@ public Object evaluate(Socket socket) throws IOException {
 
     String string = reader.readLine();
     ExpressionParser parser = new SpelExpressionParser();
-    Expression expression = parser.parseExpression(string);
+    Expression expression = parser.parseExpression(string); // AVOID: string is controlled by the user
     SimpleEvaluationContext context 
         = SimpleEvaluationContext.forReadWriteDataBinding().build();
-    return expression.getValue(context);
+    return expression.getValue(context); // OK: Untrusted expressions are evaluated in a restricted context
   }
 }

java/ql/src/Security/CWE/CWE-094/UnsafeJexlExpressionEvaluation.java

@@ -4,7 +4,7 @@ public void evaluate(Socket socket) throws IOException {
 
     String input = reader.readLine();
     JexlEngine jexl = new JexlBuilder().create();
-    JexlExpression expression = jexl.createExpression(input);
+    JexlExpression expression = jexl.createExpression(input); // BAD: input is controlled by the user
     JexlContext context = new MapContext();
     expression.evaluate(context);
   }

java/ql/src/Security/CWE/CWE-094/UnsafeSpelExpressionEvaluation.java

@@ -4,7 +4,7 @@ public Object evaluate(Socket socket) throws IOException {
 
     String string = reader.readLine();
     ExpressionParser parser = new SpelExpressionParser();
-    Expression expression = parser.parseExpression(string);
+    Expression expression = parser.parseExpression(string); // BAD: string is controlled by the user
     return expression.getValue();
   }
 }

java/ql/src/Security/CWE/CWE-1204/BadStaticInitializationVector.java

@@ -1,4 +1,4 @@
-byte[] iv = new byte[16]; // all zeroes
+byte[] iv = new byte[16]; // BAD: all zeroes
 GCMParameterSpec params = new GCMParameterSpec(128, iv);
 Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING");
 cipher.init(Cipher.ENCRYPT_MODE, key, params);

java/ql/src/Security/CWE/CWE-1204/GoodRandomInitializationVector.java

@@ -1,6 +1,6 @@
 byte[] iv = new byte[16];
 SecureRandom random = SecureRandom.getInstanceStrong();
-random.nextBytes(iv);
+random.nextBytes(iv); // GOOD: random initialization vector
 GCMParameterSpec params = new GCMParameterSpec(128, iv);
 Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING");
 cipher.init(Cipher.ENCRYPT_MODE, key, params);

java/ql/src/Security/CWE/CWE-200/AndroidSensitiveTextBad.java

@@ -1,2 +1,2 @@
 TextView pwView = getViewById(R.id.pw_text);
-pwView.setText("Your password is: " + password);
+pwView.setText("Your password is: " + password); // BAD: password is shown immediately

java/ql/src/Security/CWE/CWE-200/AndroidSensitiveTextGood.java

@@ -5,6 +5,6 @@
 Button showButton = findViewById(R.id.show_pw_button);
 showButton.setOnClickListener(new View.OnClickListener() {
     public void onClick(View v) {
-      pwView.setVisibility(View.VISIBLE);
+      pwView.setVisibility(View.VISIBLE); // GOOD: password is only shown when the user clicks the button
     }
 });

java/ql/src/Security/CWE/CWE-200/ContentAccessDisabled.java

@@ -1,3 +1,4 @@
 WebSettings settings = webview.getSettings();
 
+// GOOD: WebView is configured to disallow content access
 settings.setAllowContentAccess(false);

java/ql/src/Security/CWE/CWE-200/ContentAccessEnabled.java

@@ -1,3 +1,4 @@
 WebSettings settings = webview.getSettings();
 
+// BAD: WebView is configured to allow content access
 settings.setAllowContentAccess(true);

java/ql/src/Security/CWE/CWE-200/WebViewFileAccessSafe.java

@@ -1,5 +1,6 @@
 WebSettings settings = view.getSettings();
 
+// GOOD: WebView is configured to disallow file access
 settings.setAllowFileAccess(false);
 settings.setAllowFileAccessFromURLs(false);
 settings.setAllowUniversalAccessFromURLs(false);

java/ql/src/Security/CWE/CWE-200/WebViewFileAccessUnsafe.java

@@ -1,5 +1,6 @@
 WebSettings settings = view.getSettings();
 
+// BAD: WebView is configured to allow file access
 settings.setAllowFileAccess(true);
 settings.setAllowFileAccessFromURLs(true);
 settings.setAllowUniversalAccessFromURLs(true);

java/ql/src/Security/CWE/CWE-330/examples/InsecureRandomnessCookie.java

@@ -1,4 +1,4 @@
-Random r = new Random();
+Random r = new Random(); // BAD: Random is not cryptographically secure
 
 byte[] bytes = new byte[16];
 r.nextBytes(bytes);

java/ql/src/Security/CWE/CWE-330/examples/SecureRandomnessCookie.java

@@ -1,4 +1,4 @@
-SecureRandom r = new SecureRandom();
+SecureRandom r = new SecureRandom(); // GOOD: SecureRandom is cryptographically secure
 
 byte[] bytes = new byte[16];
 r.nextBytes(bytes);

java/ql/src/Security/CWE/CWE-367/TOCTOURace.java

@@ -12,14 +12,14 @@ public synchronized void act() {
 
 public synchronized void bad(Resource r) {
 	if (r.isReady()) {
-		// r might no longer be ready, another thread might
+		// BAD: r might no longer be ready, another thread might
 		// have called setReady(false)
 		r.act();
 	}
 }
 
 public synchronized void good(Resource r) {
-	synchronized(r) {
+	synchronized(r) { // GOOD: r is locked
 		if (r.isReady()) {
 			r.act();
 		}

java/ql/src/Security/CWE/CWE-502/UnsafeDeserializationBad.java

@@ -7,6 +7,6 @@
 
 public MyObject deserialize(Socket sock) {
   try(ObjectInputStream in = new ObjectInputStream(sock.getInputStream())) {
-    return (MyObject)in.readObject(); // unsafe
+    return (MyObject)in.readObject(); // BAD: in is from untrusted source
   }
 }

java/ql/src/Security/CWE/CWE-502/UnsafeDeserializationGood.java

@@ -1,5 +1,5 @@
 public MyObject deserialize(Socket sock) {
   try(DataInputStream in = new DataInputStream(sock.getInputStream())) {
-    return new MyObject(in.readInt());
+    return new MyObject(in.readInt()); // GOOD: read only an int
   }
 }

java/ql/src/Security/CWE/CWE-522/LdapAuthUseLdap.java

@@ -1,3 +1,4 @@
+// BAD: LDAP authentication is used
 String ldapUrl = "ldap://ad.your-server.com:389";
 Hashtable<String, String> environment = new Hashtable<String, String>();
 environment.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");

java/ql/src/Security/CWE/CWE-522/LdapAuthUseLdaps.java

@@ -1,3 +1,4 @@
+// GOOD: LDAP connection using LDAPS
 String ldapUrl = "ldaps://ad.your-server.com:636";
 Hashtable<String, String> environment = new Hashtable<String, String>();
 environment.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");

java/ql/src/Security/CWE/CWE-522/LdapEnableSasl.java

@@ -1,3 +1,4 @@
+// GOOD: LDAP is used but SASL authentication is enabled
 String ldapUrl = "ldap://ad.your-server.com:389";
 Hashtable<String, String> environment = new Hashtable<String, String>();
 environment.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");

java/ql/src/Security/CWE/CWE-611/XXEBad.java

@@ -1,5 +1,5 @@
 public void parse(Socket sock) throws Exception {
   DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
   DocumentBuilder builder = factory.newDocumentBuilder();
-  builder.parse(sock.getInputStream()); //unsafe
+  builder.parse(sock.getInputStream()); // BAD: DTD parsing is enabled
 }

java/ql/src/Security/CWE/CWE-611/XXEGood.java

@@ -2,5 +2,5 @@ public void disableDTDParse(Socket sock) throws Exception {
   DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
   factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
   DocumentBuilder builder = factory.newDocumentBuilder();
-  builder.parse(sock.getInputStream()); //safe
+  builder.parse(sock.getInputStream()); // GOOD: DTD parsing is disabled
 }

java/ql/src/Security/CWE/CWE-798/HardcodedAWSCredentials.java

@@ -3,7 +3,7 @@
 
 public class HardcodedAWSCredentials {
 	public static void main(String[] args) {
-		//Hardcoded credentials for connecting to AWS services
+		// BAD: Hardcoded credentials for connecting to AWS services
 		//To fix the problem, use other approaches including AWS credentials file, environment variables, or instance/container credentials instead 
 		AWSCredentials creds = new BasicAWSCredentials("ACCESS_KEY", "SECRET_KEY"); //sensitive call
 	}

java/ql/src/Security/CWE/CWE-798/HardcodedCredentialsApiCall.java

@@ -1,8 +1,8 @@
-private static final String p = "123456"; // hard-coded credential
+private static final String p = "123456"; // BAD: hard-coded credential
 
 public static void main(String[] args) throws SQLException {
     String url = "jdbc:mysql://localhost/test";
-    String u = "admin"; // hard-coded credential
+    String u = "admin"; // BAD: hard-coded credential
 
     getConn(url, u, p);
 }

java/ql/src/Security/CWE/CWE-835/InfiniteLoopBad.java

@@ -1,5 +1,5 @@
 for (int i=0; i<10; i++) {
-    for (int j=0; i<10; j++) {
+    for (int j=0; i<10; j++) { // BAD: Potential infinite loop: i should be j
         // do stuff
         if (shouldBreak()) break;
     }

java/ql/src/Security/CWE/CWE-835/InfiniteLoopGood.java

@@ -1,5 +1,5 @@
 for (int i=0; i<10; i++) {
-    for (int j=0; j<10; j++) {
+    for (int j=0; j<10; j++) { // GOOD: correct variable j
         // do stuff
         if (shouldBreak()) break;
     }

java/ql/src/Security/CWE/CWE-925/Bad.java

@@ -1,6 +1,7 @@
 public class ShutdownReceiver extends BroadcastReceiver {
     @Override
     public void onReceive(final Context context, final Intent intent) {
+        // BAD: The code does not check if the intent is an ACTION_SHUTDOWN intent
         mainActivity.saveLocalData();
         mainActivity.stopActivity();
     }

java/ql/src/Security/CWE/CWE-925/Good.java

@@ -1,6 +1,7 @@
 public class ShutdownReceiver extends BroadcastReceiver {
     @Override
     public void onReceive(final Context context, final Intent intent) {
+        // GOOD: The code checks if the intent is an ACTION_SHUTDOWN intent
         if (!intent.getAction().equals(Intent.ACTION_SHUTDOWN)) {
             return;
         }