Skip to content

Commit 5003ef1

Browse files
authored
Scripts: Fix security for deprecation warning (#28485)
If you call `getDates()` on a long or date type field add a deprecation warning to the response and log something to the deprecation logger. This *mostly* worked just fine but if the deprecation logger happens to roll then the roll will be performed with the script's permissions rather than the permissions of the server. And scripts don't have permissions to, say, open files. So the rolling failed. This fixes that by wrapping the call the deprecation logger in `doPriviledged`. This is a strange `doPrivileged` call because it doens't check Elasticsearch's `SpecialPermission`. `SpecialPermission` is a permission that no-script code has and that scripts never have. Usually all `doPrivileged` calls check `SpecialPermission` to make sure that they are not accidentally acting on behalf of a script. But in this case we are *intentionally* acting on behalf of a script. Closes #28408
1 parent de6d31e commit 5003ef1

File tree

4 files changed

+216
-15
lines changed

4 files changed

+216
-15
lines changed

modules/lang-painless/src/test/resources/rest-api-spec/test/painless/50_script_doc_values.yml

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,9 @@ setup:
8383

8484
---
8585
"date":
86+
- skip:
87+
features: "warnings"
88+
8689
- do:
8790
search:
8891
body:
@@ -101,6 +104,28 @@ setup:
101104
source: "doc.date.value"
102105
- match: { hits.hits.0.fields.field.0: '2017-01-01T12:11:12.000Z' }
103106

107+
- do:
108+
warnings:
109+
- getDate is no longer necessary on date fields as the value is now a date.
110+
search:
111+
body:
112+
script_fields:
113+
field:
114+
script:
115+
source: "doc['date'].date"
116+
- match: { hits.hits.0.fields.field.0: '2017-01-01T12:11:12.000Z' }
117+
118+
- do:
119+
warnings:
120+
- getDates is no longer necessary on date fields as the values are now dates.
121+
search:
122+
body:
123+
script_fields:
124+
field:
125+
script:
126+
source: "doc['date'].dates.get(0)"
127+
- match: { hits.hits.0.fields.field.0: '2017-01-01T12:11:12.000Z' }
128+
104129
---
105130
"geo_point":
106131
- do:
@@ -165,6 +190,9 @@ setup:
165190

166191
---
167192
"long":
193+
- skip:
194+
features: "warnings"
195+
168196
- do:
169197
search:
170198
body:
@@ -183,6 +211,28 @@ setup:
183211
source: "doc['long'].value"
184212
- match: { hits.hits.0.fields.field.0: 12348732141234 }
185213

214+
- do:
215+
warnings:
216+
- getDate on numeric fields is deprecated. Use a date field to get dates.
217+
search:
218+
body:
219+
script_fields:
220+
field:
221+
script:
222+
source: "doc['long'].date"
223+
- match: { hits.hits.0.fields.field.0: '2361-04-26T03:22:21.234Z' }
224+
225+
- do:
226+
warnings:
227+
- getDates on numeric fields is deprecated. Use a date field to get dates.
228+
search:
229+
body:
230+
script_fields:
231+
field:
232+
script:
233+
source: "doc['long'].dates.get(0)"
234+
- match: { hits.hits.0.fields.field.0: '2361-04-26T03:22:21.234Z' }
235+
186236
---
187237
"integer":
188238
- do:

server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java

Lines changed: 74 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,18 +35,21 @@
3535
import org.joda.time.ReadableDateTime;
3636

3737
import java.io.IOException;
38+
import java.security.AccessController;
39+
import java.security.PrivilegedAction;
3840
import java.util.AbstractList;
3941
import java.util.Arrays;
4042
import java.util.Comparator;
4143
import java.util.List;
44+
import java.util.function.Consumer;
4245
import java.util.function.UnaryOperator;
4346

4447

4548
/**
4649
* Script level doc values, the assumption is that any implementation will
4750
* implement a <code>getValue</code> and a <code>getValues</code> that return
4851
* the relevant type that then can be used in scripts.
49-
*
52+
*
5053
* Implementations should not internally re-use objects for the values that they
5154
* return as a single {@link ScriptDocValues} instance can be reused to return
5255
* values form multiple documents.
@@ -94,14 +97,30 @@ public static final class Longs extends ScriptDocValues<Long> {
9497
protected static final DeprecationLogger deprecationLogger = new DeprecationLogger(ESLoggerFactory.getLogger(Longs.class));
9598

9699
private final SortedNumericDocValues in;
100+
/**
101+
* Callback for deprecated fields. In production this should always point to
102+
* {@link #deprecationLogger} but tests will override it so they can test that
103+
* we use the required permissions when calling it.
104+
*/
105+
private final Consumer<String> deprecationCallback;
97106
private long[] values = new long[0];
98107
private int count;
99108
private Dates dates;
100109
private int docId = -1;
101110

111+
/**
112+
* Standard constructor.
113+
*/
102114
public Longs(SortedNumericDocValues in) {
103-
this.in = in;
115+
this(in, deprecationLogger::deprecated);
116+
}
104117

118+
/**
119+
* Constructor for testing the deprecation callback.
120+
*/
121+
Longs(SortedNumericDocValues in, Consumer<String> deprecationCallback) {
122+
this.in = in;
123+
this.deprecationCallback = deprecationCallback;
105124
}
106125

107126
@Override
@@ -142,7 +161,7 @@ public long getValue() {
142161

143162
@Deprecated
144163
public ReadableDateTime getDate() throws IOException {
145-
deprecationLogger.deprecated("getDate on numeric fields is deprecated. Use a date field to get dates.");
164+
deprecated("getDate on numeric fields is deprecated. Use a date field to get dates.");
146165
if (dates == null) {
147166
dates = new Dates(in);
148167
dates.setNextDocId(docId);
@@ -152,7 +171,7 @@ public ReadableDateTime getDate() throws IOException {
152171

153172
@Deprecated
154173
public List<ReadableDateTime> getDates() throws IOException {
155-
deprecationLogger.deprecated("getDates on numeric fields is deprecated. Use a date field to get dates.");
174+
deprecated("getDates on numeric fields is deprecated. Use a date field to get dates.");
156175
if (dates == null) {
157176
dates = new Dates(in);
158177
dates.setNextDocId(docId);
@@ -169,6 +188,22 @@ public Long get(int index) {
169188
public int size() {
170189
return count;
171190
}
191+
192+
/**
193+
* Log a deprecation log, with the server's permissions, not the permissions of the
194+
* script calling this method. We need to do this to prevent errors when rolling
195+
* the log file.
196+
*/
197+
private void deprecated(String message) {
198+
// Intentionally not calling SpecialPermission.check because this is supposed to be called by scripts
199+
AccessController.doPrivileged(new PrivilegedAction<Void>() {
200+
@Override
201+
public Void run() {
202+
deprecationCallback.accept(message);
203+
return null;
204+
}
205+
});
206+
}
172207
}
173208

174209
public static final class Dates extends ScriptDocValues<ReadableDateTime> {
@@ -177,15 +212,32 @@ public static final class Dates extends ScriptDocValues<ReadableDateTime> {
177212
private static final ReadableDateTime EPOCH = new DateTime(0, DateTimeZone.UTC);
178213

179214
private final SortedNumericDocValues in;
215+
/**
216+
* Callback for deprecated fields. In production this should always point to
217+
* {@link #deprecationLogger} but tests will override it so they can test that
218+
* we use the required permissions when calling it.
219+
*/
220+
private final Consumer<String> deprecationCallback;
180221
/**
181222
* Values wrapped in {@link MutableDateTime}. Null by default an allocated on first usage so we allocate a reasonably size. We keep
182223
* this array so we don't have allocate new {@link MutableDateTime}s on every usage. Instead we reuse them for every document.
183224
*/
184225
private MutableDateTime[] dates;
185226
private int count;
186227

228+
/**
229+
* Standard constructor.
230+
*/
187231
public Dates(SortedNumericDocValues in) {
232+
this(in, deprecationLogger::deprecated);
233+
}
234+
235+
/**
236+
* Constructor for testing deprecation logging.
237+
*/
238+
Dates(SortedNumericDocValues in, Consumer<String> deprecationCallback) {
188239
this.in = in;
240+
this.deprecationCallback = deprecationCallback;
189241
}
190242

191243
/**
@@ -204,7 +256,7 @@ public ReadableDateTime getValue() {
204256
*/
205257
@Deprecated
206258
public ReadableDateTime getDate() {
207-
deprecationLogger.deprecated("getDate is no longer necessary on date fields as the value is now a date.");
259+
deprecated("getDate is no longer necessary on date fields as the value is now a date.");
208260
return getValue();
209261
}
210262

@@ -213,7 +265,7 @@ public ReadableDateTime getDate() {
213265
*/
214266
@Deprecated
215267
public List<ReadableDateTime> getDates() {
216-
deprecationLogger.deprecated("getDates is no longer necessary on date fields as the values are now dates.");
268+
deprecated("getDates is no longer necessary on date fields as the values are now dates.");
217269
return this;
218270
}
219271

@@ -274,6 +326,22 @@ void refreshArray() throws IOException {
274326
dates[i] = new MutableDateTime(in.nextValue(), DateTimeZone.UTC);
275327
}
276328
}
329+
330+
/**
331+
* Log a deprecation log, with the server's permissions, not the permissions of the
332+
* script calling this method. We need to do this to prevent errors when rolling
333+
* the log file.
334+
*/
335+
private void deprecated(String message) {
336+
// Intentionally not calling SpecialPermission.check because this is supposed to be called by scripts
337+
AccessController.doPrivileged(new PrivilegedAction<Void>() {
338+
@Override
339+
public Void run() {
340+
deprecationCallback.accept(message);
341+
return null;
342+
}
343+
});
344+
}
277345
}
278346

279347
public static final class Doubles extends ScriptDocValues<Double> {

server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,17 @@
2626
import org.joda.time.ReadableDateTime;
2727

2828
import java.io.IOException;
29+
import java.security.AccessControlContext;
30+
import java.security.AccessController;
31+
import java.security.PermissionCollection;
32+
import java.security.Permissions;
33+
import java.security.PrivilegedAction;
34+
import java.security.ProtectionDomain;
35+
import java.util.HashSet;
36+
import java.util.Set;
37+
import java.util.function.Consumer;
38+
39+
import static org.hamcrest.Matchers.containsInAnyOrder;
2940

3041
public class ScriptDocValuesDatesTests extends ESTestCase {
3142
public void test() throws IOException {
@@ -39,12 +50,19 @@ public void test() throws IOException {
3950
values[d][i] = expectedDates[d][i].getMillis();
4051
}
4152
}
42-
Dates dates = wrap(values);
53+
Set<String> warnings = new HashSet<>();
54+
Dates dates = wrap(values, deprecationMessage -> {
55+
warnings.add(deprecationMessage);
56+
/* Create a temporary directory to prove we are running with the
57+
* server's permissions. */
58+
createTempDir();
59+
});
4360

4461
for (int round = 0; round < 10; round++) {
4562
int d = between(0, values.length - 1);
4663
dates.setNextDocId(d);
4764
assertEquals(expectedDates[d].length > 0 ? expectedDates[d][0] : new DateTime(0, DateTimeZone.UTC), dates.getValue());
65+
assertEquals(expectedDates[d].length > 0 ? expectedDates[d][0] : new DateTime(0, DateTimeZone.UTC), dates.getDate());
4866

4967
assertEquals(values[d].length, dates.size());
5068
for (int i = 0; i < values[d].length; i++) {
@@ -54,9 +72,33 @@ public void test() throws IOException {
5472
Exception e = expectThrows(UnsupportedOperationException.class, () -> dates.add(new DateTime()));
5573
assertEquals("doc values are unmodifiable", e.getMessage());
5674
}
75+
76+
/*
77+
* Invoke getDates without any privileges to verify that
78+
* it still works without any. In particularly, this
79+
* verifies that the callback that we've configured
80+
* above works. That callback creates a temporary
81+
* directory which is not possible with "noPermissions".
82+
*/
83+
PermissionCollection noPermissions = new Permissions();
84+
AccessControlContext noPermissionsAcc = new AccessControlContext(
85+
new ProtectionDomain[] {
86+
new ProtectionDomain(null, noPermissions)
87+
}
88+
);
89+
AccessController.doPrivileged(new PrivilegedAction<Void>() {
90+
public Void run() {
91+
dates.getDates();
92+
return null;
93+
}
94+
}, noPermissionsAcc);
95+
96+
assertThat(warnings, containsInAnyOrder(
97+
"getDate is no longer necessary on date fields as the value is now a date.",
98+
"getDates is no longer necessary on date fields as the values are now dates."));
5799
}
58100

59-
private Dates wrap(long[][] values) {
101+
private Dates wrap(long[][] values, Consumer<String> deprecationHandler) {
60102
return new Dates(new AbstractSortedNumericDocValues() {
61103
long[] current;
62104
int i;
@@ -75,6 +117,6 @@ public int docValueCount() {
75117
public long nextValue() {
76118
return current[i++];
77119
}
78-
});
120+
}, deprecationHandler);
79121
}
80122
}

0 commit comments

Comments
 (0)