From 3159d958a0f3c07e1f7c19a45f284177687a2fcd Mon Sep 17 00:00:00 2001
From: dilanbhalla <dbhalla21@berkeley.edu>
Date: Thu, 2 Jul 2020 19:09:37 -0700
Subject: [PATCH 1/3] bufferedrwpair undefined behavior

---
 .../BufferedRWPairUndefinedBehavior.qhelp     | 24 +++++++++++++++++++
 .../BufferedRWPairUndefinedBehavior.ql        | 23 ++++++++++++++++++
 .../BufferedRWPairUndefinedBehavior.expected  |  1 +
 .../BufferedRWPairUndefinedBehavior.py        | 11 +++++++++
 .../BufferedRWPairUndefinedBehavior.qlref     |  1 +
 5 files changed, 60 insertions(+)
 create mode 100644 python/ql/src/experimental/Security/BufferedRWPairUndefinedBehavior.qhelp
 create mode 100644 python/ql/src/experimental/Security/BufferedRWPairUndefinedBehavior.ql
 create mode 100644 python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.expected
 create mode 100644 python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.py
 create mode 100644 python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.qlref

diff --git a/python/ql/src/experimental/Security/BufferedRWPairUndefinedBehavior.qhelp b/python/ql/src/experimental/Security/BufferedRWPairUndefinedBehavior.qhelp
new file mode 100644
index 000000000000..8921ff5d2766
--- /dev/null
+++ b/python/ql/src/experimental/Security/BufferedRWPairUndefinedBehavior.qhelp
@@ -0,0 +1,24 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+
+    <overview>
+        <p>
+            <code>BufferedRWPair</code> does not attempt to synchronize accesses to its underlying raw streams. 
+            You should not pass it the same object as reader and writer.
+        </p>
+    </overview>
+
+    <recommendation>
+        <p>
+
+            Use <code>BufferedRandom</code> instead.
+
+        </p>
+    </recommendation>
+
+    <references>
+        <li>Python Documentation: <a href="https://docs.python.org/3/library/io.html#io.BufferedRWPair">BufferedRWPair</a></li>
+    </references>
+</qhelp>
diff --git a/python/ql/src/experimental/Security/BufferedRWPairUndefinedBehavior.ql b/python/ql/src/experimental/Security/BufferedRWPairUndefinedBehavior.ql
new file mode 100644
index 000000000000..0fdfde41136f
--- /dev/null
+++ b/python/ql/src/experimental/Security/BufferedRWPairUndefinedBehavior.ql
@@ -0,0 +1,23 @@
+/**
+ * @name Same parameter used as reader and writer in BufferedRWPair
+ * @description Use of same parameter as reader and writer in BufferedRWPair.
+ * @kind problem
+ * @problem.severity warning
+ * @precision medium
+ * @id python/bufferedrwpair-undefined-behavior
+ * @tags reliability
+ *       security
+ */
+
+import python
+
+ClassValue requestFunction() { result = Module::named("io").attr("BufferedRWPair") }
+
+from CallNode call, ClassValue func, ControlFlowNode read, ControlFlowNode write
+where
+  func = requestFunction() and
+  func.getACall() = call and
+  read = call.getArg(0) and
+  write = call.getArg(1) and
+  read.getNode().toString() = write.getNode().toString()
+select call, "BufferedRWPair does not attempt to synchronize accesses to its underlying raw streams"
diff --git a/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.expected b/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.expected
new file mode 100644
index 000000000000..b749a574f432
--- /dev/null
+++ b/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.expected
@@ -0,0 +1 @@
+| BufferedRWPairUndefinedBehavior.py:7:5:7:37 | ControlFlowNode for Attribute() | BufferedRWPair does not attempt to synchronize accesses to its underlying raw streams |
diff --git a/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.py b/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.py
new file mode 100644
index 000000000000..173ca074c2a8
--- /dev/null
+++ b/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.py
@@ -0,0 +1,11 @@
+import io
+
+def buffer_rw_pair_test():
+
+    reader = io.BufferedReader(io.RawIOBase)
+    # BAD, uses reader for both parameters
+    io.BufferedRWPair(reader, reader)
+
+    writer = io.BufferedWriter(io.RawIOBase)
+    # GOOD, uses different reader/writer
+    io.BufferedRWPair(reader, writer)
\ No newline at end of file
diff --git a/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.qlref b/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.qlref
new file mode 100644
index 000000000000..42a2c930029f
--- /dev/null
+++ b/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.qlref
@@ -0,0 +1 @@
+experimental/Security/BufferedRWPairUndefinedBehavior.ql
\ No newline at end of file

From 78c10b95bcf7145b6bbf975b7c3cc755c5fe52f0 Mon Sep 17 00:00:00 2001
From: dilanbhalla <dbhalla21@berkeley.edu>
Date: Thu, 2 Jul 2020 19:24:25 -0700
Subject: [PATCH 2/3] added to test file

---
 .../query-tests/BufferedRWPairUndefinedBehavior.expected | 2 +-
 .../query-tests/BufferedRWPairUndefinedBehavior.py       | 9 +++++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.expected b/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.expected
index b749a574f432..71ca8e9f51ae 100644
--- a/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.expected
+++ b/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.expected
@@ -1 +1 @@
-| BufferedRWPairUndefinedBehavior.py:7:5:7:37 | ControlFlowNode for Attribute() | BufferedRWPair does not attempt to synchronize accesses to its underlying raw streams |
+| BufferedRWPairUndefinedBehavior.py:7:13:7:45 | ControlFlowNode for Attribute() | BufferedRWPair does not attempt to synchronize accesses to its underlying raw streams |
diff --git a/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.py b/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.py
index 173ca074c2a8..ca4e44825caa 100644
--- a/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.py
+++ b/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.py
@@ -4,8 +4,13 @@ def buffer_rw_pair_test():
 
     reader = io.BufferedReader(io.RawIOBase)
     # BAD, uses reader for both parameters
-    io.BufferedRWPair(reader, reader)
+    pair1 = io.BufferedRWPair(reader, reader)
 
     writer = io.BufferedWriter(io.RawIOBase)
     # GOOD, uses different reader/writer
-    io.BufferedRWPair(reader, writer)
\ No newline at end of file
+    pair2 = io.BufferedRWPair(reader, writer)
+
+    # GOOD, uses reader variable as both reader and writer for random access
+    random = io.BufferedRandom(reader)
+
+    return (pair1, pair2, random)
\ No newline at end of file

From 1ee3e17517c9bfa3d53d72b8aee0dc5e2bde68d6 Mon Sep 17 00:00:00 2001
From: dilanbhalla <dbhalla21@berkeley.edu>
Date: Tue, 7 Jul 2020 10:05:27 -0700
Subject: [PATCH 3/3] implemented suggestions from pr

---
 .../Security/BufferedRWPairUndefinedBehavior.ql          | 9 +++++----
 .../query-tests/BufferedRWPairUndefinedBehavior.expected | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/python/ql/src/experimental/Security/BufferedRWPairUndefinedBehavior.ql b/python/ql/src/experimental/Security/BufferedRWPairUndefinedBehavior.ql
index 0fdfde41136f..e3029fb589cf 100644
--- a/python/ql/src/experimental/Security/BufferedRWPairUndefinedBehavior.ql
+++ b/python/ql/src/experimental/Security/BufferedRWPairUndefinedBehavior.ql
@@ -3,10 +3,11 @@
  * @description Use of same parameter as reader and writer in BufferedRWPair.
  * @kind problem
  * @problem.severity warning
- * @precision medium
- * @id python/bufferedrwpair-undefined-behavior
+ * @id py/bufferedrwpair-undefined-behavior
  * @tags reliability
  *       security
+ *       external/cwe/cwe-475
+ *       external/cwe/cwe-758
  */
 
 import python
@@ -19,5 +20,5 @@ where
   func.getACall() = call and
   read = call.getArg(0) and
   write = call.getArg(1) and
-  read.getNode().toString() = write.getNode().toString()
-select call, "BufferedRWPair does not attempt to synchronize accesses to its underlying raw streams"
+  read.pointsTo() = write.pointsTo()
+select call, "Using the same reader and writer for BufferedRWPair may cause unexpected behavior"
diff --git a/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.expected b/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.expected
index 71ca8e9f51ae..48c0e2710835 100644
--- a/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.expected
+++ b/python/ql/test/experimental/query-tests/BufferedRWPairUndefinedBehavior.expected
@@ -1 +1 @@
-| BufferedRWPairUndefinedBehavior.py:7:13:7:45 | ControlFlowNode for Attribute() | BufferedRWPair does not attempt to synchronize accesses to its underlying raw streams |
+| BufferedRWPairUndefinedBehavior.py:7:13:7:45 | ControlFlowNode for Attribute() | Using the same reader and writer for BufferedRWPair may cause unexpected behavior |