From 9d107900b2e0b599397b84409580d46e0ed16291 Mon Sep 17 00:00:00 2001
From: Zane Bitter <zbitter@redhat.com>
Date: Tue, 14 May 2019 15:39:19 -0400
Subject: [PATCH] Eliminate SQL injection vulnerability in node_cache

In node_cache.find_node() we were constructing a raw SQL query using
unescaped data that came in on the wire. This presented an SQL injection
vulnerability. To avoid this, use the query builder from SQLAlchemy to
ensure that any input strings are correctly escaped.

Change-Id: I2b0ffa307ec1aa57538733f2e454d2d7e994d656
Story: #2005678
Task: 30992
---
 ironic_inspector/node_cache.py                    | 15 ++++++---------
 ironic_inspector/test/unit/test_node_cache.py     |  5 +++++
 ...ind-node-input-filtering-e8ea529252e80739.yaml |  7 +++++++
 3 files changed, 18 insertions(+), 9 deletions(-)
 create mode 100644 releasenotes/notes/find-node-input-filtering-e8ea529252e80739.yaml

diff --git a/ironic_inspector/node_cache.py b/ironic_inspector/node_cache.py
index 07126d989..fa37447a2 100644
--- a/ironic_inspector/node_cache.py
+++ b/ironic_inspector/node_cache.py
@@ -18,6 +18,7 @@ import contextlib
 import copy
 import datetime
 import json
+import operator
 
 from automaton import exceptions as automaton_errors
 from ironicclient import exceptions
@@ -30,7 +31,6 @@ from oslo_utils import timeutils
 from oslo_utils import uuidutils
 import six
 from sqlalchemy.orm import exc as orm_errors
-from sqlalchemy import text
 
 from ironic_inspector.common.i18n import _
 from ironic_inspector.common import ironic as ir_utils
@@ -840,14 +840,11 @@ def find_node(**attributes):
 
         LOG.debug('Trying to use %s of value %s for node look up',
                   name, value)
-        value_list = []
-        for v in value:
-            value_list.append("name='%s' AND value='%s'" % (name, v))
-        stmt = ('select distinct node_uuid from attributes where ' +
-                ' OR '.join(value_list))
-        rows = (db.model_query(db.Attribute.node_uuid).from_statement(
-            text(stmt)).all())
-        found.update(row.node_uuid for row in rows)
+        query = db.model_query(db.Attribute.node_uuid)
+        pairs = [(db.Attribute.name == name) &
+                 (db.Attribute.value == v) for v in value]
+        query = query.filter(six.moves.reduce(operator.or_, pairs))
+        found.update(row.node_uuid for row in query.distinct().all())
 
     if not found:
         raise utils.NotFoundInCacheError(_(
diff --git a/ironic_inspector/test/unit/test_node_cache.py b/ironic_inspector/test/unit/test_node_cache.py
index 7f45eacca..469fc1745 100644
--- a/ironic_inspector/test/unit/test_node_cache.py
+++ b/ironic_inspector/test/unit/test_node_cache.py
@@ -315,6 +315,11 @@ class TestNodeCacheFind(test_base.NodeTest):
         self.assertRaises(utils.Error, node_cache.find_node,
                           bmc_address='1.2.3.4')
 
+    def test_input_filtering(self):
+        self.assertRaises(utils.NotFoundInCacheError,
+                          node_cache.find_node,
+                          bmc_address="' OR ''='")
+
 
 class TestNodeCacheCleanUp(test_base.NodeTest):
     def setUp(self):
diff --git a/releasenotes/notes/find-node-input-filtering-e8ea529252e80739.yaml b/releasenotes/notes/find-node-input-filtering-e8ea529252e80739.yaml
new file mode 100644
index 000000000..f5e0b3f7d
--- /dev/null
+++ b/releasenotes/notes/find-node-input-filtering-e8ea529252e80739.yaml
@@ -0,0 +1,7 @@
+---
+security:
+  - |
+    Fixes insufficient input filtering when looking up a node by information
+    from the introspection data. It could potentially allow SQL injections
+    via the ``/v1/continue`` API endpoint. See `story 2005678
+    <https://storyboard.openstack.org/#!/story/2005678>`_ for details.