Browse code

Applied CVE-2016-5423.patch (Bug 1782504)

Change-Id: Ibdb9642d265f5973de040d65ee295362907bbb40
Reviewed-on: http://photon-jenkins.eng.vmware.com:8082/1876
Tested-by: gerrit-photon <photon-checkins@vmware.com>
Reviewed-by: Divya Thaluru <dthaluru@vmware.com>

xiaolin-vmware authored on 2016/12/16 13:47:04
Showing 2 changed files
1 1
new file mode 100644
... ...
@@ -0,0 +1,330 @@
0
+From f0c7b789ab12fbc8248b671c7882dd96ac932ef4 Mon Sep 17 00:00:00 2001
1
+From: Tom Lane <tgl@sss.pgh.pa.us>
2
+Date: Mon, 8 Aug 2016 10:33:46 -0400
3
+Subject: [PATCH] Fix two errors with nested CASE/WHEN constructs.
4
+
5
+ExecEvalCase() tried to save a cycle or two by passing
6
+&econtext->caseValue_isNull as the isNull argument to its sub-evaluation of
7
+the CASE value expression.  If that subexpression itself contained a CASE,
8
+then *isNull was an alias for econtext->caseValue_isNull within the
9
+recursive call of ExecEvalCase(), leading to confusion about whether the
10
+inner call's caseValue was null or not.  In the worst case this could lead
11
+to a core dump due to dereferencing a null pointer.  Fix by not assigning
12
+to the global variable until control comes back from the subexpression.
13
+Also, avoid using the passed-in isNull pointer transiently for evaluation
14
+of WHEN expressions.  (Either one of these changes would have been
15
+sufficient to fix the known misbehavior, but it's clear now that each of
16
+these choices was in itself dangerous coding practice and best avoided.
17
+There do not seem to be any similar hazards elsewhere in execQual.c.)
18
+
19
+Also, it was possible for inlining of a SQL function that implements the
20
+equality operator used for a CASE comparison to result in one CASE
21
+expression's CaseTestExpr node being inserted inside another CASE
22
+expression.  This would certainly result in wrong answers since the
23
+improperly nested CaseTestExpr would be caused to return the inner CASE's
24
+comparison value not the outer's.  If the CASE values were of different
25
+data types, a crash might result; moreover such situations could be abused
26
+to allow disclosure of portions of server memory.  To fix, teach
27
+inline_function to check for "bare" CaseTestExpr nodes in the arguments of
28
+a function to be inlined, and avoid inlining if there are any.
29
+
30
+Heikki Linnakangas, Michael Paquier, Tom Lane
31
+
32
+Report: https://github.com/greenplum-db/gpdb/pull/327
33
+Report: <4DDCEEB8.50602@enterprisedb.com>
34
+Security: CVE-2016-5423
35
+---
36
+ src/backend/executor/execQual.c      | 22 +++++++---
37
+ src/backend/optimizer/util/clauses.c | 81 ++++++++++++++++++++++++++++++++++++
38
+ src/test/regress/expected/case.out   | 44 ++++++++++++++++++++
39
+ src/test/regress/sql/case.sql        | 43 +++++++++++++++++++
40
+ 4 files changed, 185 insertions(+), 5 deletions(-)
41
+
42
+diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
43
+index 69bf65d..cbb76d1 100644
44
+--- a/src/backend/executor/execQual.c
45
+@@ -2943,19 +2943,30 @@ ExecEvalCase(CaseExprState *caseExpr, ExprContext *econtext,
46
+ 
47
+ 	/*
48
+ 	 * If there's a test expression, we have to evaluate it and save the value
49
+-	 * where the CaseTestExpr placeholders can find it. We must save and
50
++	 * where the CaseTestExpr placeholders can find it.  We must save and
51
+ 	 * restore prior setting of econtext's caseValue fields, in case this node
52
+-	 * is itself within a larger CASE.
53
++	 * is itself within a larger CASE.  Furthermore, don't assign to the
54
++	 * econtext fields until after returning from evaluation of the test
55
++	 * expression.  We used to pass &econtext->caseValue_isNull to the
56
++	 * recursive call, but that leads to aliasing that variable within said
57
++	 * call, which can (and did) produce bugs when the test expression itself
58
++	 * contains a CASE.
59
++	 *
60
++	 * If there's no test expression, we don't actually need to save and
61
++	 * restore these fields; but it's less code to just do so unconditionally.
62
+ 	 */
63
+ 	save_datum = econtext->caseValue_datum;
64
+ 	save_isNull = econtext->caseValue_isNull;
65
+ 
66
+ 	if (caseExpr->arg)
67
+ 	{
68
++		bool		arg_isNull;
69
++
70
+ 		econtext->caseValue_datum = ExecEvalExpr(caseExpr->arg,
71
+ 												 econtext,
72
+-												 &econtext->caseValue_isNull,
73
++												 &arg_isNull,
74
+ 												 NULL);
75
++		econtext->caseValue_isNull = arg_isNull;
76
+ 	}
77
+ 
78
+ 	/*
79
+@@ -2994,10 +3005,11 @@ ExecEvalCase(CaseExprState *caseExpr, ExprContext *econtext,
80
+ 	{
81
+ 		CaseWhenState *wclause = lfirst(clause);
82
+ 		Datum		clause_value;
83
++		bool		clause_isNull;
84
+ 
85
+ 		clause_value = ExecEvalExpr(wclause->expr,
86
+ 									econtext,
87
+-									isNull,
88
++									&clause_isNull,
89
+ 									NULL);
90
+ 
91
+ 		/*
92
+@@ -3005,7 +3017,7 @@ ExecEvalCase(CaseExprState *caseExpr, ExprContext *econtext,
93
+ 		 * statement is satisfied.  A NULL result from the test is not
94
+ 		 * considered true.
95
+ 		 */
96
+-		if (DatumGetBool(clause_value) && !*isNull)
97
++		if (DatumGetBool(clause_value) && !clause_isNull)
98
+ 		{
99
+ 			econtext->caseValue_datum = save_datum;
100
+ 			econtext->caseValue_isNull = save_isNull;
101
+diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
102
+index a69af7c..4e23898 100644
103
+--- a/src/backend/optimizer/util/clauses.c
104
+@@ -97,6 +97,8 @@ static bool contain_mutable_functions_walker(Node *node, void *context);
105
+ static bool contain_volatile_functions_walker(Node *node, void *context);
106
+ static bool contain_volatile_functions_not_nextval_walker(Node *node, void *context);
107
+ static bool contain_nonstrict_functions_walker(Node *node, void *context);
108
++static bool contain_context_dependent_node(Node *clause);
109
++static bool contain_context_dependent_node_walker(Node *node, int *flags);
110
+ static bool contain_leaked_vars_walker(Node *node, void *context);
111
+ static Relids find_nonnullable_rels_walker(Node *node, bool top_level);
112
+ static List *find_nonnullable_vars_walker(Node *node, bool top_level);
113
+@@ -1323,6 +1325,76 @@ contain_nonstrict_functions_walker(Node *node, void *context)
114
+ }
115
+ 
116
+ /*****************************************************************************
117
++ *		Check clauses for context-dependent nodes
118
++ *****************************************************************************/
119
++
120
++/*
121
++ * contain_context_dependent_node
122
++ *	  Recursively search for context-dependent nodes within a clause.
123
++ *
124
++ * CaseTestExpr nodes must appear directly within the corresponding CaseExpr,
125
++ * not nested within another one, or they'll see the wrong test value.  If one
126
++ * appears "bare" in the arguments of a SQL function, then we can't inline the
127
++ * SQL function for fear of creating such a situation.
128
++ *
129
++ * CoerceToDomainValue would have the same issue if domain CHECK expressions
130
++ * could get inlined into larger expressions, but presently that's impossible.
131
++ * Still, it might be allowed in future, or other node types with similar
132
++ * issues might get invented.  So give this function a generic name, and set
133
++ * up the recursion state to allow multiple flag bits.
134
++ */
135
++static bool
136
++contain_context_dependent_node(Node *clause)
137
++{
138
++	int			flags = 0;
139
++
140
++	return contain_context_dependent_node_walker(clause, &flags);
141
++}
142
++
143
++#define CCDN_IN_CASEEXPR	0x0001		/* CaseTestExpr okay here? */
144
++
145
++static bool
146
++contain_context_dependent_node_walker(Node *node, int *flags)
147
++{
148
++	if (node == NULL)
149
++		return false;
150
++	if (IsA(node, CaseTestExpr))
151
++		return !(*flags & CCDN_IN_CASEEXPR);
152
++	if (IsA(node, CaseExpr))
153
++	{
154
++		CaseExpr   *caseexpr = (CaseExpr *) node;
155
++
156
++		/*
157
++		 * If this CASE doesn't have a test expression, then it doesn't create
158
++		 * a context in which CaseTestExprs should appear, so just fall
159
++		 * through and treat it as a generic expression node.
160
++		 */
161
++		if (caseexpr->arg)
162
++		{
163
++			int			save_flags = *flags;
164
++			bool		res;
165
++
166
++			/*
167
++			 * Note: in principle, we could distinguish the various sub-parts
168
++			 * of a CASE construct and set the flag bit only for some of them,
169
++			 * since we are only expecting CaseTestExprs to appear in the
170
++			 * "expr" subtree of the CaseWhen nodes.  But it doesn't really
171
++			 * seem worth any extra code.  If there are any bare CaseTestExprs
172
++			 * elsewhere in the CASE, something's wrong already.
173
++			 */
174
++			*flags |= CCDN_IN_CASEEXPR;
175
++			res = expression_tree_walker(node,
176
++									   contain_context_dependent_node_walker,
177
++										 (void *) flags);
178
++			*flags = save_flags;
179
++			return res;
180
++		}
181
++	}
182
++	return expression_tree_walker(node, contain_context_dependent_node_walker,
183
++								  (void *) flags);
184
++}
185
++
186
++/*****************************************************************************
187
+  *		  Check clauses for Vars passed to non-leakproof functions
188
+  *****************************************************************************/
189
+ 
190
+@@ -4230,6 +4302,8 @@ evaluate_function(Oid funcid, Oid result_type, int32 result_typmod,
191
+  * doesn't work in the general case because it discards information such
192
+  * as OUT-parameter declarations.
193
+  *
194
++ * Also, context-dependent expression nodes in the argument list are trouble.
195
++ *
196
+  * Returns a simplified expression if successful, or NULL if cannot
197
+  * simplify the function.
198
+  */
199
+@@ -4424,6 +4498,13 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
200
+ 		contain_nonstrict_functions(newexpr))
201
+ 		goto fail;
202
+ 
203
++ 	/*
204
++	 * If any parameter expression contains a context-dependent node, we can't
205
++	 * inline, for fear of putting such a node into the wrong context.
206
++	 */
207
++	if (contain_context_dependent_node((Node *) args))
208
++		goto fail;
209
++
210
+ 	/*
211
+ 	 * We may be able to do it; there are still checks on parameter usage to
212
+ 	 * make, but those are most easily done in combination with the actual
213
+diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out
214
+index c564eed..35b6476 100644
215
+--- a/src/test/regress/expected/case.out
216
+@@ -297,7 +297,51 @@ SELECT * FROM CASE_TBL;
217
+ (4 rows)
218
+ 
219
+ --
220
++-- Nested CASE expressions
221
++--
222
++-- This test exercises a bug caused by aliasing econtext->caseValue_isNull
223
++-- with the isNull argument of the inner CASE's ExecEvalCase() call.  After
224
++-- evaluating the vol(null) expression in the inner CASE's second WHEN-clause,
225
++-- the isNull flag for the case test value incorrectly became true, causing
226
++-- the third WHEN-clause not to match.  The volatile function calls are needed
227
++-- to prevent constant-folding in the planner, which would hide the bug.
228
++CREATE FUNCTION vol(text) returns text as
229
++  'begin return $1; end' language plpgsql volatile;
230
++SELECT CASE
231
++  (CASE vol('bar')
232
++    WHEN 'foo' THEN 'it was foo!'
233
++    WHEN vol(null) THEN 'null input'
234
++    WHEN 'bar' THEN 'it was bar!' END
235
++  )
236
++  WHEN 'it was foo!' THEN 'foo recognized'
237
++  WHEN 'it was bar!' THEN 'bar recognized'
238
++  ELSE 'unrecognized' END;
239
++      case      
240
++----------------
241
++ bar recognized
242
++(1 row)
243
++
244
++-- In this case, we can't inline the SQL function without confusing things.
245
++CREATE DOMAIN foodomain AS text;
246
++CREATE FUNCTION volfoo(text) returns foodomain as
247
++  'begin return $1::foodomain; end' language plpgsql volatile;
248
++CREATE FUNCTION inline_eq(foodomain, foodomain) returns boolean as
249
++  'SELECT CASE $2::text WHEN $1::text THEN true ELSE false END' language sql;
250
++CREATE OPERATOR = (procedure = inline_eq,
251
++                   leftarg = foodomain, rightarg = foodomain);
252
++SELECT CASE volfoo('bar') WHEN 'foo'::foodomain THEN 'is foo' ELSE 'is not foo' END;
253
++    case    
254
++------------
255
++ is not foo
256
++(1 row)
257
++
258
++--
259
+ -- Clean up
260
+ --
261
+ DROP TABLE CASE_TBL;
262
+ DROP TABLE CASE2_TBL;
263
++DROP OPERATOR = (foodomain, foodomain);
264
++DROP FUNCTION inline_eq(foodomain, foodomain);
265
++DROP FUNCTION volfoo(text);
266
++DROP DOMAIN foodomain;
267
++DROP FUNCTION vol(text);
268
+diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sql
269
+index 5f41753..b2377e4 100644
270
+--- a/src/test/regress/sql/case.sql
271
+@@ -157,8 +157,51 @@ UPDATE CASE_TBL
272
+ SELECT * FROM CASE_TBL;
273
+ 
274
+ --
275
++-- Nested CASE expressions
276
++--
277
++
278
++-- This test exercises a bug caused by aliasing econtext->caseValue_isNull
279
++-- with the isNull argument of the inner CASE's ExecEvalCase() call.  After
280
++-- evaluating the vol(null) expression in the inner CASE's second WHEN-clause,
281
++-- the isNull flag for the case test value incorrectly became true, causing
282
++-- the third WHEN-clause not to match.  The volatile function calls are needed
283
++-- to prevent constant-folding in the planner, which would hide the bug.
284
++
285
++CREATE FUNCTION vol(text) returns text as
286
++  'begin return $1; end' language plpgsql volatile;
287
++
288
++SELECT CASE
289
++  (CASE vol('bar')
290
++    WHEN 'foo' THEN 'it was foo!'
291
++    WHEN vol(null) THEN 'null input'
292
++    WHEN 'bar' THEN 'it was bar!' END
293
++  )
294
++  WHEN 'it was foo!' THEN 'foo recognized'
295
++  WHEN 'it was bar!' THEN 'bar recognized'
296
++  ELSE 'unrecognized' END;
297
++
298
++-- In this case, we can't inline the SQL function without confusing things.
299
++CREATE DOMAIN foodomain AS text;
300
++
301
++CREATE FUNCTION volfoo(text) returns foodomain as
302
++  'begin return $1::foodomain; end' language plpgsql volatile;
303
++
304
++CREATE FUNCTION inline_eq(foodomain, foodomain) returns boolean as
305
++  'SELECT CASE $2::text WHEN $1::text THEN true ELSE false END' language sql;
306
++
307
++CREATE OPERATOR = (procedure = inline_eq,
308
++                   leftarg = foodomain, rightarg = foodomain);
309
++
310
++SELECT CASE volfoo('bar') WHEN 'foo'::foodomain THEN 'is foo' ELSE 'is not foo' END;
311
++
312
++--
313
+ -- Clean up
314
+ --
315
+ 
316
+ DROP TABLE CASE_TBL;
317
+ DROP TABLE CASE2_TBL;
318
++DROP OPERATOR = (foodomain, foodomain);
319
++DROP FUNCTION inline_eq(foodomain, foodomain);
320
++DROP FUNCTION volfoo(text);
321
++DROP DOMAIN foodomain;
322
++DROP FUNCTION vol(text);
323
+-- 
324
+2.1.4
325
+
... ...
@@ -1,32 +1,32 @@
1
-Summary:	PostgreSQL database engine
2
-Name:		postgresql
3
-Version:	9.5.3
4
-Release:	5%{?dist}
5
-License:	PostgreSQL
6
-URL:		www.postgresql.org
7
-Group:		Applications/Databases
8
-Vendor:		VMware, Inc.
9
-Distribution:	Photon
10
-Provides:	%{name}
11
-
12
-Source0:	http://ftp.postgresql.org/pub/source/v%{version}/%{name}-%{version}.tar.bz2
13
-%define sha1 postgresql=bd8dcbc8c4882468675dcc93263182a27d4ff201
1
+Summary:        PostgreSQL database engine
2
+Name:           postgresql
3
+Version:        9.5.3
4
+Release:        6%{?dist}
5
+License:        PostgreSQL
6
+URL:            www.postgresql.org
7
+Group:          Applications/Databases
8
+Vendor:         VMware, Inc.
9
+Distribution:   Photon
10
+Provides:       %{name}
14 11
 
12
+Source0:        http://ftp.postgresql.org/pub/source/v%{version}/%{name}-%{version}.tar.bz2
13
+%define sha1    postgresql=bd8dcbc8c4882468675dcc93263182a27d4ff201
14
+Patch0:         CVE-2016-5423.patch
15 15
 # Common libraries needed
16
-BuildRequires:	krb5-devel
17
-BuildRequires:	libxml2-devel
18
-BuildRequires:	openldap
19
-BuildRequires:	perl
20
-BuildRequires:	readline-devel
21
-BuildRequires:	openssl-devel
22
-BuildRequires:	zlib-devel
23
-BuildRequires:	tzdata
24
-Requires:		krb5
25
-Requires:		libxml2
26
-Requires:		openldap
27
-Requires:		openssl
28
-Requires:		readline
29
-Requires:		zlib
16
+BuildRequires:  krb5-devel
17
+BuildRequires:  libxml2-devel
18
+BuildRequires:  openldap
19
+BuildRequires:  perl
20
+BuildRequires:  readline-devel
21
+BuildRequires:  openssl-devel
22
+BuildRequires:  zlib-devel
23
+BuildRequires:  tzdata
24
+Requires:       krb5
25
+Requires:       libxml2
26
+Requires:       openldap
27
+Requires:       openssl
28
+Requires:       readline
29
+Requires:       zlib
30 30
 Requires:       tzdata
31 31
 
32 32
 Requires:   %{name}-libs = %{version}-%{release}
... ...
@@ -47,18 +47,19 @@ PostgreSQL server.
47 47
 
48 48
 %prep
49 49
 %setup -q
50
+%patch0 -p1
50 51
 %build
51 52
 sed -i '/DEFAULT_PGSOCKET_DIR/s@/tmp@/run/postgresql@' src/include/pg_config_manual.h &&
52 53
 ./configure \
53
-	--enable-thread-safety \
54
-	--prefix=%{_prefix} \
55
-	--with-ldap \
56
-	--with-libxml \
57
-	--with-openssl \
58
-	--with-gssapi \
59
-	--with-readline \
60
-	--with-system-tzdata=%{_datadir}/zoneinfo \
61
-	--docdir=%{_docdir}/postgresql
54
+    --enable-thread-safety \
55
+    --prefix=%{_prefix} \
56
+    --with-ldap \
57
+    --with-libxml \
58
+    --with-openssl \
59
+    --with-gssapi \
60
+    --with-readline \
61
+    --with-system-tzdata=%{_datadir}/zoneinfo \
62
+    --docdir=%{_docdir}/postgresql
62 63
 make %{?_smp_mflags}
63 64
 cd contrib && make %{?_smp_mflags}
64 65
 
... ...
@@ -73,8 +74,8 @@ cd contrib && make install DESTDIR=%{buildroot}
73 73
 chown -Rv nobody .
74 74
 sudo -u nobody -s /bin/bash -c "PATH=$PATH make -k check"
75 75
 
76
-%post	-p /sbin/ldconfig
77
-%postun	-p /sbin/ldconfig
76
+%post   -p /sbin/ldconfig
77
+%postun -p /sbin/ldconfig
78 78
 %clean
79 79
 rm -rf %{buildroot}/*
80 80
 
... ...
@@ -162,6 +163,8 @@ rm -rf %{buildroot}/*
162 162
 %{_datadir}/postgresql/psqlrc.sample
163 163
 
164 164
 %changelog
165
+*   Thu Dec 15 2016 Xiaolin Li <xiaolinl@vmware.com> 9.5.3-6
166
+-   Applied CVE-2016-5423.patch
165 167
 *   Thu Nov 24 2016 Alexey Makhalov <amakhalov@vmware.com> 9.5.3-5
166 168
 -   Required krb5-devel.
167 169
 *   Mon Oct 03 2016 ChangLee <changLee@vmware.com> 9.5.3-4