Skip to content

Commit b90f4fb

Browse files
vikhy-awsvikhy-aws
committed
Fix authorization bypass in stashContext() (backport opensearch-project#1760)
Signed-off-by: vikhy-aws <191836418+vikhy-aws@users.noreply.github.com>
1 parent b40e29c commit b90f4fb

9 files changed

Lines changed: 650 additions & 58 deletions

File tree

‎src/main/java/org/opensearch/securityanalytics/transport/QueryUtils.java‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
package org.opensearch.securityanalytics.transport;
6+
7+
import org.opensearch.index.query.BoolQueryBuilder;
8+
import org.opensearch.index.query.BoostingQueryBuilder;
9+
import org.opensearch.index.query.ConstantScoreQueryBuilder;
10+
import org.opensearch.index.query.DisMaxQueryBuilder;
11+
import org.opensearch.index.query.NestedQueryBuilder;
12+
import org.opensearch.index.query.QueryBuilder;
13+
import org.opensearch.index.query.TermsQueryBuilder;
14+
15+
/**
16+
* Utility methods for inspecting query trees for security-sensitive patterns.
17+
* Uses manual instanceof traversal to walk all sub-queries, ensuring complete
18+
* coverage of known compound query types. Unknown query types are denied by default
19+
* (treated as potentially containing terms lookups) to prevent bypass via novel wrappers.
20+
*/
21+
public class QueryUtils {
22+
23+
private QueryUtils() {}
24+
25+
/**
26+
* Checks if a query tree contains any TermsQueryBuilder with a termsLookup
27+
* (i.e., a cross-index terms lookup that could be used to probe data in unauthorized indices).
28+
* Traverses known compound query types recursively; denies unknown compound types by default.
29+
*/
30+
public static boolean containsTermsLookup(QueryBuilder query) {
31+
if (query == null) {
32+
return false;
33+
}
34+
35+
// Check if this is a TermsQueryBuilder with a termsLookup
36+
if (query instanceof TermsQueryBuilder) {
37+
return ((TermsQueryBuilder) query).termsLookup() != null;
38+
}
39+
40+
// Recurse into known compound query types
41+
if (query instanceof BoolQueryBuilder) {
42+
BoolQueryBuilder bool = (BoolQueryBuilder) query;
43+
for (QueryBuilder clause : bool.must()) {
44+
if (containsTermsLookup(clause)) return true;
45+
}
46+
for (QueryBuilder clause : bool.should()) {
47+
if (containsTermsLookup(clause)) return true;
48+
}
49+
for (QueryBuilder clause : bool.mustNot()) {
50+
if (containsTermsLookup(clause)) return true;
51+
}
52+
for (QueryBuilder clause : bool.filter()) {
53+
if (containsTermsLookup(clause)) return true;
54+
}
55+
return false;
56+
}
57+
58+
if (query instanceof ConstantScoreQueryBuilder) {
59+
return containsTermsLookup(((ConstantScoreQueryBuilder) query).innerQuery());
60+
}
61+
62+
if (query instanceof BoostingQueryBuilder) {
63+
BoostingQueryBuilder boosting = (BoostingQueryBuilder) query;
64+
return containsTermsLookup(boosting.positiveQuery()) || containsTermsLookup(boosting.negativeQuery());
65+
}
66+
67+
if (query instanceof DisMaxQueryBuilder) {
68+
for (QueryBuilder clause : ((DisMaxQueryBuilder) query).innerQueries()) {
69+
if (containsTermsLookup(clause)) return true;
70+
}
71+
return false;
72+
}
73+
74+
if (query instanceof NestedQueryBuilder) {
75+
return containsTermsLookup(((NestedQueryBuilder) query).query());
76+
}
77+
78+
// For all other (leaf) query types, they cannot contain a terms lookup
79+
return false;
80+
}
81+
}

‎src/main/java/org/opensearch/securityanalytics/transport/TransportCreateIndexMappingsAction.java‎

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,11 @@
55
package org.opensearch.securityanalytics.transport;
66

77
import org.opensearch.action.ActionListener;
8+
import org.opensearch.action.admin.indices.mapping.put.PutMappingRequest;
89
import org.opensearch.action.support.ActionFilters;
910
import org.opensearch.action.support.HandledTransportAction;
1011
import org.opensearch.action.support.master.AcknowledgedResponse;
11-
import org.opensearch.cluster.metadata.IndexMetadata;
12+
import org.opensearch.client.Client;
1213
import org.opensearch.cluster.service.ClusterService;
1314
import org.opensearch.common.inject.Inject;
1415
import org.opensearch.securityanalytics.action.CreateIndexMappingsAction;
@@ -18,11 +19,14 @@
1819
import org.opensearch.threadpool.ThreadPool;
1920
import org.opensearch.transport.TransportService;
2021

22+
import java.util.Map;
23+
2124
public class TransportCreateIndexMappingsAction extends HandledTransportAction<CreateIndexMappingsRequest, AcknowledgedResponse> {
2225
private MapperService mapperService;
2326
private ClusterService clusterService;
2427

2528
private final ThreadPool threadPool;
29+
private final Client client;
2630

2731

2832
@Inject
@@ -31,24 +35,35 @@ public TransportCreateIndexMappingsAction(
3135
ActionFilters actionFilters,
3236
ThreadPool threadPool,
3337
MapperService mapperService,
34-
ClusterService clusterService
38+
ClusterService clusterService,
39+
Client client
3540
) {
3641
super(CreateIndexMappingsAction.NAME, transportService, actionFilters, CreateIndexMappingsRequest::new);
3742
this.clusterService = clusterService;
3843
this.mapperService = mapperService;
3944
this.threadPool = threadPool;
45+
this.client = client;
4046
}
4147

4248
@Override
4349
protected void doExecute(Task task, CreateIndexMappingsRequest request, ActionListener<AcknowledgedResponse> actionListener) {
44-
this.threadPool.getThreadContext().stashContext();
45-
46-
mapperService.createMappingAction(
47-
request.getIndexName(),
48-
request.getRuleTopic(),
49-
request.getAliasMappings(),
50-
request.getPartial(),
51-
actionListener
52-
);
50+
// Verify caller has indices:admin/mapping/put on the target index before elevating privileges.
51+
// Issues a no-op PutMappingRequest (empty properties) as the caller — the security plugin
52+
// checks the permission naturally without stashContext, so unauthorized users get 403.
53+
PutMappingRequest putMappingRequest = new PutMappingRequest(request.getIndexName())
54+
.source(Map.of("properties", Map.of()));
55+
client.admin().indices().putMapping(putMappingRequest, ActionListener.wrap(
56+
putMappingResponse -> {
57+
this.threadPool.getThreadContext().stashContext();
58+
mapperService.createMappingAction(
59+
request.getIndexName(),
60+
request.getRuleTopic(),
61+
request.getAliasMappings(),
62+
request.getPartial(),
63+
actionListener
64+
);
65+
},
66+
actionListener::onFailure
67+
));
5368
}
54-
}
69+
}

‎src/main/java/org/opensearch/securityanalytics/transport/TransportGetIndexMappingsAction.java‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,19 +4,17 @@
44
*/
55
package org.opensearch.securityanalytics.transport;
66

7-
import org.opensearch.OpenSearchStatusException;
87
import org.opensearch.action.ActionListener;
8+
import org.opensearch.action.admin.indices.mapping.get.GetMappingsRequest;
99
import org.opensearch.action.support.ActionFilters;
1010
import org.opensearch.action.support.HandledTransportAction;
11-
import org.opensearch.cluster.metadata.IndexMetadata;
11+
import org.opensearch.client.Client;
1212
import org.opensearch.cluster.service.ClusterService;
1313
import org.opensearch.common.inject.Inject;
14-
import org.opensearch.rest.RestStatus;
1514
import org.opensearch.securityanalytics.action.GetIndexMappingsAction;
1615
import org.opensearch.securityanalytics.mapper.MapperService;
1716
import org.opensearch.securityanalytics.action.GetIndexMappingsRequest;
1817
import org.opensearch.securityanalytics.action.GetIndexMappingsResponse;
19-
import org.opensearch.securityanalytics.util.SecurityAnalyticsException;
2018
import org.opensearch.tasks.Task;
2119
import org.opensearch.threadpool.ThreadPool;
2220
import org.opensearch.transport.TransportService;
@@ -26,6 +24,7 @@ public class TransportGetIndexMappingsAction extends HandledTransportAction<GetI
2624
private ClusterService clusterService;
2725

2826
private final ThreadPool threadPool;
27+
private final Client client;
2928

3029
@Inject
3130
public TransportGetIndexMappingsAction(
@@ -34,18 +33,26 @@ public TransportGetIndexMappingsAction(
3433
GetIndexMappingsAction getIndexMappingsAction,
3534
MapperService mapperService,
3635
ClusterService clusterService,
37-
ThreadPool threadPool
36+
ThreadPool threadPool,
37+
Client client
3838
) {
3939
super(getIndexMappingsAction.NAME, transportService, actionFilters, GetIndexMappingsRequest::new);
4040
this.clusterService = clusterService;
4141
this.mapperService = mapperService;
4242
this.threadPool = threadPool;
43+
this.client = client;
4344
}
4445

4546
@Override
4647
protected void doExecute(Task task, GetIndexMappingsRequest request, ActionListener<GetIndexMappingsResponse> actionListener) {
47-
this.threadPool.getThreadContext().stashContext();
48-
49-
mapperService.getMappingAction(request.getIndexName(), actionListener);
48+
// Verify caller has permission on the target index before elevating privileges
49+
GetMappingsRequest getMappingsRequest = new GetMappingsRequest().indices(request.getIndexName());
50+
client.admin().indices().getMappings(getMappingsRequest, ActionListener.wrap(
51+
getMappingsResponse -> {
52+
this.threadPool.getThreadContext().stashContext();
53+
mapperService.getMappingAction(request.getIndexName(), actionListener);
54+
},
55+
actionListener::onFailure
56+
));
5057
}
51-
}
58+
}

‎src/main/java/org/opensearch/securityanalytics/transport/TransportGetMappingsViewAction.java‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,22 +4,17 @@
44
*/
55
package org.opensearch.securityanalytics.transport;
66

7-
import org.opensearch.OpenSearchStatusException;
87
import org.opensearch.action.ActionListener;
8+
import org.opensearch.action.admin.indices.mapping.get.GetMappingsRequest;
99
import org.opensearch.action.support.ActionFilters;
1010
import org.opensearch.action.support.HandledTransportAction;
11-
import org.opensearch.cluster.metadata.IndexMetadata;
11+
import org.opensearch.client.Client;
1212
import org.opensearch.cluster.service.ClusterService;
1313
import org.opensearch.common.inject.Inject;
14-
import org.opensearch.rest.RestStatus;
15-
import org.opensearch.securityanalytics.action.GetIndexMappingsAction;
16-
import org.opensearch.securityanalytics.action.GetIndexMappingsRequest;
17-
import org.opensearch.securityanalytics.action.GetIndexMappingsResponse;
1814
import org.opensearch.securityanalytics.action.GetMappingsViewAction;
1915
import org.opensearch.securityanalytics.action.GetMappingsViewRequest;
2016
import org.opensearch.securityanalytics.action.GetMappingsViewResponse;
2117
import org.opensearch.securityanalytics.mapper.MapperService;
22-
import org.opensearch.securityanalytics.util.SecurityAnalyticsException;
2318
import org.opensearch.tasks.Task;
2419
import org.opensearch.threadpool.ThreadPool;
2520
import org.opensearch.transport.TransportService;
@@ -28,6 +23,7 @@ public class TransportGetMappingsViewAction extends HandledTransportAction<GetMa
2823
private MapperService mapperService;
2924
private ClusterService clusterService;
3025
private final ThreadPool threadPool;
26+
private final Client client;
3127

3228
@Inject
3329
public TransportGetMappingsViewAction(
@@ -36,17 +32,26 @@ public TransportGetMappingsViewAction(
3632
GetMappingsViewAction getMappingsViewAction,
3733
MapperService mapperService,
3834
ClusterService clusterService,
39-
ThreadPool threadPool
35+
ThreadPool threadPool,
36+
Client client
4037
) {
4138
super(getMappingsViewAction.NAME, transportService, actionFilters, GetMappingsViewRequest::new);
4239
this.clusterService = clusterService;
4340
this.mapperService = mapperService;
4441
this.threadPool = threadPool;
42+
this.client = client;
4543
}
4644

4745
@Override
4846
protected void doExecute(Task task, GetMappingsViewRequest request, ActionListener<GetMappingsViewResponse> actionListener) {
49-
this.threadPool.getThreadContext().stashContext();
50-
this.mapperService.getMappingsViewAction(request.getIndexName(), request.getRuleTopic(), actionListener);
47+
// Verify caller has permission on the target index before elevating privileges
48+
GetMappingsRequest getMappingsRequest = new GetMappingsRequest().indices(request.getIndexName());
49+
client.admin().indices().getMappings(getMappingsRequest, ActionListener.wrap(
50+
getMappingsResponse -> {
51+
this.threadPool.getThreadContext().stashContext();
52+
this.mapperService.getMappingsViewAction(request.getIndexName(), request.getRuleTopic(), actionListener);
53+
},
54+
actionListener::onFailure
55+
));
5156
}
52-
}
57+
}

‎src/main/java/org/opensearch/securityanalytics/transport/TransportSearchDetectorAction.java‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,20 +6,19 @@
66

77
import org.apache.logging.log4j.LogManager;
88
import org.apache.logging.log4j.Logger;
9-
9+
import org.opensearch.OpenSearchStatusException;
1010
import org.opensearch.action.ActionListener;
1111
import org.opensearch.action.search.SearchResponse;
12-
1312
import org.opensearch.action.support.ActionFilters;
1413
import org.opensearch.action.support.HandledTransportAction;
1514
import org.opensearch.commons.authuser.User;
1615
import org.opensearch.client.Client;
1716
import org.opensearch.common.inject.Inject;
18-
import org.opensearch.common.io.stream.StreamInput;
1917
import org.opensearch.common.settings.Settings;
2018
import org.opensearch.cluster.service.ClusterService;
2119
import org.opensearch.common.xcontent.NamedXContentRegistry;
2220
import org.opensearch.rest.RestStatus;
21+
import org.opensearch.search.builder.SearchSourceBuilder;
2322
import org.opensearch.securityanalytics.action.SearchDetectorAction;
2423
import org.opensearch.securityanalytics.action.SearchDetectorRequest;
2524
import org.opensearch.securityanalytics.settings.SecurityAnalyticsSettings;
@@ -77,6 +76,14 @@ protected void doExecute(Task task, SearchDetectorRequest searchDetectorRequest,
7776
addFilter(user, searchDetectorRequest.searchRequest().source(), "detector.user.backend_roles.keyword");
7877
}
7978

79+
// Reject queries containing terms lookups that reference external indices
80+
SearchSourceBuilder source = searchDetectorRequest.searchRequest().source();
81+
if (source != null && source.query() != null && QueryUtils.containsTermsLookup(source.query())) {
82+
actionListener.onFailure(new OpenSearchStatusException(
83+
"Terms lookup queries referencing external indices are not permitted in detector search", RestStatus.FORBIDDEN));
84+
return;
85+
}
86+
8087
this.threadPool.getThreadContext().stashContext();
8188

8289
client.search(searchDetectorRequest.searchRequest(), new ActionListener<>() {
@@ -96,4 +103,4 @@ private void setFilterByEnabled(boolean filterByEnabled) {
96103
this.filterByEnabled = filterByEnabled;
97104
}
98105

99-
}
106+
}

‎src/main/java/org/opensearch/securityanalytics/transport/TransportSearchRuleAction.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import org.opensearch.common.unit.TimeValue;
2626
import org.opensearch.index.reindex.BulkByScrollResponse;
2727
import org.opensearch.rest.RestStatus;
28+
import org.opensearch.search.builder.SearchSourceBuilder;
2829
import org.opensearch.search.internal.InternalSearchResponse;
2930
import org.opensearch.securityanalytics.action.SearchRuleAction;
3031
import org.opensearch.securityanalytics.action.SearchRuleRequest;
@@ -88,6 +89,13 @@ class AsyncSearchRulesAction {
8889
}
8990

9091
void start() {
92+
SearchSourceBuilder source = request.getSearchRequest().source();
93+
if (source != null && source.query() != null && QueryUtils.containsTermsLookup(source.query())) {
94+
listener.onFailure(new OpenSearchStatusException(
95+
"Terms lookup queries referencing external indices are not permitted in rule search", RestStatus.FORBIDDEN));
96+
return;
97+
}
98+
9199
TransportSearchRuleAction.this.threadPool.getThreadContext().stashContext();
92100
if (request.isPrepackaged()) {
93101
ruleIndices.initPrepackagedRulesIndex(
@@ -243,4 +251,4 @@ private void finishHim(SearchResponse response, Exception t) {
243251
}));
244252
}
245253
}
246-
}
254+
}

0 commit comments

Comments
 (0)