From 1400b4b520e77eeb2d439ceec0a8baf5c257b236 Mon Sep 17 00:00:00 2001
From: Raul Garcia <raul.garcia@microsoft.com>
Date: Mon, 20 Mar 2023 18:52:58 -0700
Subject: [PATCH] Update UnsafeUsageOfClientSideEncryptionVersion.ql

*  predicate `isUnsafeClientSideAzureStorageEncryptionViaObjectCreation` was not useful (it was meant to detect the SDK code, not its usage)
* fixed & simplified `isUnsafeClientSideAzureStorageEncryptionViaAttributes`, the original query was not finding the right code.
NOTE: tested with a real project: https://github.com/wastore/azure-storage-samples-for-python/tree/master/ClientSideEncryptionToServerSideEncryptionMigrationSamples/ClientSideEncryptionV1ToV2
---
 ...nsafeUsageOfClientSideEncryptionVersion.ql | 86 ++++++-------------
 1 file changed, 24 insertions(+), 62 deletions(-)

diff --git a/python/ql/src/experimental/Security/CWE-327/Azure/UnsafeUsageOfClientSideEncryptionVersion.ql b/python/ql/src/experimental/Security/CWE-327/Azure/UnsafeUsageOfClientSideEncryptionVersion.ql
index 5799e0193ce..3eac4385391 100644
--- a/python/ql/src/experimental/Security/CWE-327/Azure/UnsafeUsageOfClientSideEncryptionVersion.ql
+++ b/python/ql/src/experimental/Security/CWE-327/Azure/UnsafeUsageOfClientSideEncryptionVersion.ql
@@ -16,76 +16,38 @@ import semmle.python.ApiGraphs
 
 predicate isUnsafeClientSideAzureStorageEncryptionViaAttributes(Call call, AttrNode node) {
   exists(
-    API::Node n, API::Node n2, Attribute a, AssignStmt astmt, API::Node uploadBlob,
-    ControlFlowNode ctrlFlowNode, string s
+    API::Node n, ControlFlowNode startingNode, Attribute attr, ControlFlowNode ctrlFlowNode,
+    Attribute attrUploadBlob, ControlFlowNode ctrlFlowNodeUploadBlob, string s1, string s2,
+    string s3
   |
-    s in ["key_encryption_key", "key_resolver_function"] and
-    n =
-      API::moduleImport("azure")
-          .getMember("storage")
-          .getMember("blob")
-          .getMember("BlobClient")
-          .getReturn()
-          .getMember(s) and
-    n2 =
-      API::moduleImport("azure")
-          .getMember("storage")
-          .getMember("blob")
-          .getMember("BlobClient")
-          .getReturn()
-          .getMember("upload_blob") and
-    n.getAValueReachableFromSource().asExpr() = a and
-    astmt.getATarget() = a and
-    a.getAFlowNode() = node and
-    uploadBlob =
-      API::moduleImport("azure")
-          .getMember("storage")
-          .getMember("blob")
-          .getMember("BlobClient")
-          .getReturn()
-          .getMember("upload_blob") and
-    uploadBlob.getACall().asExpr() = call and
-    ctrlFlowNode = call.getAFlowNode() and
-    node.strictlyReaches(ctrlFlowNode) and
-    node != ctrlFlowNode and
+    call.getAChildNode() = attrUploadBlob and
+    node = ctrlFlowNode
+  |
+    s1 in ["key_encryption_key", "key_resolver_function"] and
+    s2 in ["ContainerClient", "BlobClient", "BlobServiceClient"] and
+    s3 in ["upload_blob"] and
+    n = API::moduleImport("azure").getMember("storage").getMember("blob").getMember(s2).getAMember() and
+    startingNode = n.getACall().getReturn().getAValueReachableFromSource().asExpr().getAFlowNode() and
+    startingNode.strictlyReaches(ctrlFlowNode) and
+    attr.getAFlowNode() = ctrlFlowNode and
+    attr.getName() = s1 and
+    ctrlFlowNode.strictlyReaches(ctrlFlowNodeUploadBlob) and
+    attrUploadBlob.getAFlowNode() = ctrlFlowNodeUploadBlob and
+    attrUploadBlob.getName() = s3 and
     not exists(
-      AssignStmt astmt2, Attribute a2, AttrNode encryptionVersionSet, StrConst uc,
-      API::Node encryptionVersion
+      Attribute attrBarrier, ControlFlowNode ctrlFlowNodeBarrier, AssignStmt astmt2, StrConst uc
     |
+      startingNode.strictlyReaches(ctrlFlowNodeBarrier) and
+      attrBarrier.getAFlowNode() = ctrlFlowNodeBarrier and
+      attrBarrier.getName() = "encryption_version" and
       uc = astmt2.getValue() and
       uc.getText() in ["'2.0'", "2.0"] and
-      encryptionVersion =
-        API::moduleImport("azure")
-            .getMember("storage")
-            .getMember("blob")
-            .getMember("BlobClient")
-            .getReturn()
-            .getMember("encryption_version") and
-      encryptionVersion.getAValueReachableFromSource().asExpr() = a2 and
-      astmt2.getATarget() = a2 and
-      a2.getAFlowNode() = encryptionVersionSet and
-      encryptionVersionSet.strictlyReaches(ctrlFlowNode)
-    )
-  )
-}
-
-predicate isUnsafeClientSideAzureStorageEncryptionViaObjectCreation(Call call, ControlFlowNode node) {
-  exists(API::Node c, string s, Keyword k | k.getAFlowNode() = node |
-    c.getACall().asExpr() = call and
-    c = API::moduleImport("azure").getMember("storage").getMember("blob").getMember(s) and
-    s in ["ContainerClient", "BlobClient", "BlobServiceClient"] and
-    k.getArg() = "key_encryption_key" and
-    k = call.getANamedArg() and
-    not k.getValue() instanceof None and
-    not exists(Keyword k2 | k2 = call.getANamedArg() |
-      k2.getArg() = "encryption_version" and
-      k2.getValue().(StrConst).getText() in ["'2.0'", "2.0"]
+      astmt2.getATarget().getAChildNode*() = attrBarrier and
+      ctrlFlowNodeBarrier.strictlyReaches(ctrlFlowNodeUploadBlob)
     )
   )
 }
 
 from Call call, ControlFlowNode node
-where
-  isUnsafeClientSideAzureStorageEncryptionViaAttributes(call, node) or
-  isUnsafeClientSideAzureStorageEncryptionViaObjectCreation(call, node)
+where isUnsafeClientSideAzureStorageEncryptionViaAttributes(call, node)
 select node, "Unsafe usage of v1 version of Azure Storage client-side encryption."