Skip to content

Commit 81ddeef

Browse files
shwstpprshwstppr
committed
webhook: fix delivery hardening regression
Fixes #14482 Deliveries rewrote the payload URL to the resolved IP, breaking TLS hostname verification, SNI and the Host header (e.g. CDNs). Keep the hostname and enforce the destination blocklist in a custom HttpClient DnsResolver, validating the exact addresses connected to. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
1 parent e742aad commit 81ddeef

4 files changed

Lines changed: 369 additions & 38 deletions

File tree

‎plugins/event-bus/webhook/src/main/java/org/apache/cloudstack/mom/webhook/WebhookDeliveryThread.java‎

Lines changed: 59 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -47,12 +47,19 @@
4747
import org.apache.http.client.config.RequestConfig;
4848
import org.apache.http.client.methods.CloseableHttpResponse;
4949
import org.apache.http.client.methods.HttpPost;
50+
import org.apache.http.config.Registry;
51+
import org.apache.http.config.RegistryBuilder;
52+
import org.apache.http.conn.DnsResolver;
53+
import org.apache.http.conn.socket.ConnectionSocketFactory;
54+
import org.apache.http.conn.socket.PlainConnectionSocketFactory;
5055
import org.apache.http.conn.ssl.NoopHostnameVerifier;
56+
import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
5157
import org.apache.http.conn.ssl.TrustAllStrategy;
5258
import org.apache.http.entity.ContentType;
5359
import org.apache.http.entity.StringEntity;
5460
import org.apache.http.impl.client.CloseableHttpClient;
5561
import org.apache.http.impl.client.HttpClients;
62+
import org.apache.http.impl.conn.BasicHttpClientConnectionManager;
5663
import org.apache.http.message.BasicHeader;
5764
import org.apache.http.ssl.SSLContextBuilder;
5865
import org.apache.http.util.EntityUtils;
@@ -102,17 +109,30 @@ protected boolean isValidJson(String json) {
102109
return true;
103110
}
104111

105-
protected void setHttpClient() throws NoSuchAlgorithmException, KeyStoreException, KeyManagementException {
112+
protected SSLConnectionSocketFactory getSslConnectionSocketFactory() throws NoSuchAlgorithmException,
113+
KeyStoreException, KeyManagementException {
106114
if (webhook.isSslVerification()) {
107-
httpClient = HttpClients.custom().disableRedirectHandling().build();
108-
return;
115+
return SSLConnectionSocketFactory.getSocketFactory();
109116
}
117+
return new SSLConnectionSocketFactory(new SSLContextBuilder().loadTrustMaterial(null,
118+
TrustAllStrategy.INSTANCE).build(), NoopHostnameVerifier.INSTANCE);
119+
}
120+
121+
protected DnsResolver getDnsResolver() {
122+
return new WebhookUrlValidator.DestinationPolicyDnsResolver(destinationBlocklist, blockLocalAddresses);
123+
}
124+
125+
protected void setHttpClient() throws NoSuchAlgorithmException, KeyStoreException, KeyManagementException {
126+
Registry<ConnectionSocketFactory> socketFactoryRegistry = RegistryBuilder.<ConnectionSocketFactory>create()
127+
.register("http", PlainConnectionSocketFactory.getSocketFactory())
128+
.register("https", getSslConnectionSocketFactory())
129+
.build();
130+
// Destination addresses are validated by the DNS resolver
110131
httpClient = HttpClients
111132
.custom()
112133
.disableRedirectHandling()
113-
.setSSLContext(new SSLContextBuilder().loadTrustMaterial(null,
114-
TrustAllStrategy.INSTANCE).build())
115-
.setSSLHostnameVerifier(NoopHostnameVerifier.INSTANCE)
134+
.setConnectionManager(new BasicHttpClientConnectionManager(socketFactoryRegistry, null, null,
135+
getDnsResolver()))
116136
.build();
117137
}
118138

@@ -205,18 +225,34 @@ public void run() {
205225
callback.complete(new WebhookDeliveryResult(headers, payload, success, response, new Date()));
206226
return;
207227
}
208-
if (payloadUri == null) {
209-
payloadUri = WebhookUrlValidator.validateWebhookDestinationUrl(webhook.getPayloadUrl(), allowHttp,
210-
destinationBlocklist, blockLocalAddresses);
211-
}
212-
while (attempt < deliveryTries) {
213-
attempt++;
214-
if (delivery(attempt)) {
215-
success = true;
216-
break;
228+
try {
229+
if (payloadUri == null) {
230+
payloadUri = URI.create(webhook.getPayloadUrl());
231+
WebhookUrlValidator.validateScheme(payloadUri, allowHttp);
217232
}
233+
while (attempt < deliveryTries) {
234+
attempt++;
235+
if (delivery(attempt)) {
236+
success = true;
237+
break;
238+
}
239+
}
240+
} catch (InvalidParameterValueException | IllegalArgumentException e) {
241+
LOGGER.warn("Failed to deliver {} due to invalid payload URL: {}", webhook, e.getMessage());
242+
response = String.format("Failed due to : %s", e.getMessage());
243+
} finally {
244+
closeHttpClient();
245+
}
246+
callback.complete(new WebhookDeliveryResult(headers, payload, success, response,
247+
startTime != null ? startTime : new Date()));
248+
}
249+
250+
protected void closeHttpClient() {
251+
try {
252+
httpClient.close();
253+
} catch (IOException e) {
254+
LOGGER.debug("Failed to close HTTP client for {}: {}", webhook, e.getMessage());
218255
}
219-
callback.complete(new WebhookDeliveryResult(headers, payload, success, response, startTime));
220256
}
221257

222258
protected void updateResponseFromRequest(HttpEntity entity) {
@@ -271,16 +307,19 @@ protected boolean delivery(int attempt) {
271307
MAX_REDIRECT_HOPS);
272308
return false;
273309
}
274-
currentUri = WebhookUrlValidator.validateWebhookDestinationURI(
275-
currentUri.resolve(locationHeader.getValue()),
276-
allowHttp,destinationBlocklist, blockLocalAddresses);
310+
currentUri = currentUri.resolve(locationHeader.getValue());
311+
WebhookUrlValidator.validateScheme(currentUri, allowHttp);
277312
}
278313
}
314+
} catch (WebhookUrlValidator.BlockedDestinationException e) {
315+
LOGGER.warn("Failed to deliver {}, in attempt #{} due to security policy: {}",
316+
webhook, attempt, e.getMessage());
317+
response = String.format("Failed due to : %s", e.getMessage());
279318
} catch (IOException | DecoderException | NoSuchAlgorithmException | InvalidKeyException e) {
280319
LOGGER.warn("Failed to deliver {}, in attempt #{} due to: {}",
281320
webhook, attempt, e.getMessage());
282321
response = String.format("Failed due to : %s", e.getMessage());
283-
} catch (InvalidParameterValueException e) {
322+
} catch (InvalidParameterValueException | IllegalArgumentException e) {
284323
LOGGER.warn("Failed to deliver {}, in attempt #{} due to security policy: {}",
285324
webhook, attempt, e.getMessage());
286325
response = String.format("Failed due to : %s", e.getMessage());

‎plugins/event-bus/webhook/src/main/java/org/apache/cloudstack/mom/webhook/WebhookUrlValidator.java‎

Lines changed: 55 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,12 @@
1919

2020
import java.net.InetAddress;
2121
import java.net.URI;
22-
import java.net.URISyntaxException;
2322
import java.net.UnknownHostException;
2423
import java.util.Arrays;
2524

2625
import org.apache.commons.lang3.StringUtils;
26+
import org.apache.http.conn.DnsResolver;
27+
import org.apache.http.impl.conn.SystemDefaultDnsResolver;
2728

2829
import com.cloud.exception.InvalidParameterValueException;
2930
import com.cloud.utils.net.NetUtils;
@@ -42,18 +43,9 @@ public static URI validateWebhookDestinationUrl(final String payloadUrl, final b
4243
public static URI validateWebhookDestinationURI(final URI uri, final boolean allowHttp,
4344
final String blocklist, final boolean blockLocalAddresses) {
4445
validateScheme(uri, allowHttp);
45-
InetAddress[] resolved = validateResolvedAddresses(uri, blocklist, blockLocalAddresses);
46-
return buildResolvedUri(uri, resolved[0]);
47-
}
48-
49-
private static URI buildResolvedUri(final URI uri, final InetAddress address) {
50-
try {
51-
return new URI(uri.getScheme(), uri.getUserInfo(), address.getHostAddress(), uri.getPort(),
52-
uri.getPath(), uri.getQuery(), uri.getFragment());
53-
} catch (URISyntaxException e) {
54-
throw new InvalidParameterValueException(
55-
String.format("Failed to build resolved webhook payload URL from [%s]", uri));
56-
}
46+
validateResolvedAddresses(uri, blocklist, blockLocalAddresses);
47+
// Keep hostname for TLS and Host header
48+
return uri;
5749
}
5850

5951
public static void validateScheme(final URI uri, final boolean allowHttp) {
@@ -93,15 +85,22 @@ public static InetAddress[] validateResolvedAddresses(final URI uri, final Strin
9385
String.format("Failed to resolve webhook payload URL host [%s]", host));
9486
}
9587

96-
final String[] blockedCidrs = getNormalizedBlocklist(blocklist);
97-
for (InetAddress address : resolved) {
88+
if (isAnyAddressBlocked(resolved, getNormalizedBlocklist(blocklist), blockLocalAddresses)) {
89+
throw new InvalidParameterValueException(
90+
String.format("Webhook payload URL [%s] resolves to a blocked IP address", uri));
91+
}
92+
return resolved;
93+
}
94+
95+
static boolean isAnyAddressBlocked(final InetAddress[] addresses, final String[] blockedCidrs,
96+
final boolean blockLocalAddresses) {
97+
for (InetAddress address : addresses) {
9898
if ((blockLocalAddresses && isLocalManagementServerAddress(address)) ||
9999
NetUtils.isIpInCidrList(address, blockedCidrs)) {
100-
throw new InvalidParameterValueException(
101-
String.format("Webhook payload URL [%s] resolves to a blocked IP address", uri));
100+
return true;
102101
}
103102
}
104-
return resolved;
103+
return false;
105104
}
106105

107106
static boolean isLocalManagementServerAddress(final InetAddress address) {
@@ -120,4 +119,42 @@ public static String[] getNormalizedBlocklist(final String blocklist) {
120119
.filter(StringUtils::isNotBlank)
121120
.toArray(String[]::new);
122121
}
122+
123+
// DnsResolver can only throw UnknownHostException
124+
public static class BlockedDestinationException extends UnknownHostException {
125+
public BlockedDestinationException(final String host) {
126+
super(String.format("Webhook payload URL host [%s] resolves to a blocked IP address", host));
127+
}
128+
}
129+
130+
// Validates addresses HttpClient connects to (not with a proxy)
131+
public static class DestinationPolicyDnsResolver implements DnsResolver {
132+
private final DnsResolver delegate;
133+
private final String[] blockedCidrs;
134+
private final boolean blockLocalAddresses;
135+
136+
public DestinationPolicyDnsResolver(final String blocklist, final boolean blockLocalAddresses) {
137+
this(SystemDefaultDnsResolver.INSTANCE, blocklist, blockLocalAddresses);
138+
}
139+
140+
public DestinationPolicyDnsResolver(final DnsResolver delegate, final String blocklist,
141+
final boolean blockLocalAddresses) {
142+
this.delegate = delegate;
143+
this.blockedCidrs = getNormalizedBlocklist(blocklist);
144+
this.blockLocalAddresses = blockLocalAddresses;
145+
}
146+
147+
@Override
148+
public InetAddress[] resolve(final String host) throws UnknownHostException {
149+
final InetAddress[] resolved = delegate.resolve(host);
150+
if (resolved == null || resolved.length == 0) {
151+
throw new UnknownHostException(host);
152+
}
153+
// Fail closed if any address is blocked
154+
if (isAnyAddressBlocked(resolved, blockedCidrs, blockLocalAddresses)) {
155+
throw new BlockedDestinationException(host);
156+
}
157+
return resolved;
158+
}
159+
}
123160
}

0 commit comments

Comments
 (0)