Skip to content

Commit 747badd

Browse files
baevbaev
authored
fix path traversals for attachments (via #3272)
1 parent a6876fb commit 747badd

File tree

20 files changed

+1894
-28
lines changed

20 files changed

+1894
-28
lines changed

‎allure-generator/src/main/java/io/qameta/allure/allure1/Allure1Plugin.java‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import java.io.InputStream;
5050
import java.math.BigInteger;
5151
import java.nio.file.Files;
52+
import java.nio.file.LinkOption;
5253
import java.nio.file.Path;
5354
import java.security.MessageDigest;
5455
import java.security.NoSuchAlgorithmException;
@@ -64,6 +65,7 @@
6465
import java.util.TreeSet;
6566
import java.util.function.Predicate;
6667
import java.util.function.Supplier;
68+
import java.util.regex.Pattern;
6769
import java.util.stream.Collectors;
6870
import java.util.stream.Stream;
6971

@@ -109,6 +111,7 @@ public class Allure1Plugin implements Reader {
109111
private static final Comparator<Parameter> PARAMETER_COMPARATOR =
110112
comparing(Parameter::getName, nullsFirst(naturalOrder()))
111113
.thenComparing(Parameter::getValue, nullsFirst(naturalOrder()));
114+
private static final Pattern ATTACHMENT_SOURCE_PATTERN = Pattern.compile("^[a-zA-Z0-9._-]{1,100}$");
112115

113116
public static final String ENVIRONMENT_BLOCK_NAME = "environment";
114117
public static final String ALLURE1_RESULTS_FORMAT = "allure1";
@@ -328,8 +331,20 @@ private Parameter convert(final ru.yandex.qatools.allure.model.Parameter paramet
328331
private Attachment convert(final Path source,
329332
final ResultsVisitor visitor,
330333
final ru.yandex.qatools.allure.model.Attachment attachment) {
331-
final Path attachmentFile = source.resolve(attachment.getSource());
332-
if (Files.isRegularFile(attachmentFile)) {
334+
final String attachmentSource = attachment.getSource();
335+
336+
if (!isValidAttachmentFileName(attachmentSource)) {
337+
visitor.error("Invalid attachment source is provided: " + attachmentSource);
338+
return new Attachment()
339+
.setType(attachment.getType())
340+
.setName(attachment.getTitle())
341+
.setSize(0L);
342+
}
343+
344+
final Path attachmentFile = source.resolve(attachmentSource).normalize();
345+
346+
if (attachmentFile.startsWith(source)
347+
&& Files.isRegularFile(attachmentFile, LinkOption.NOFOLLOW_LINKS)) {
333348
final Attachment found = visitor.visitAttachmentFile(attachmentFile);
334349
if (Objects.nonNull(attachment.getType())) {
335350
found.setType(attachment.getType());
@@ -343,7 +358,7 @@ private Attachment convert(final Path source,
343358
}
344359
return found;
345360
} else {
346-
visitor.error("Could not find attachment " + attachment.getSource() + " in directory " + source);
361+
visitor.error("Could not find attachment " + attachmentSource + " in directory " + source);
347362
return new Attachment()
348363
.setType(attachment.getType())
349364
.setName(attachment.getTitle())
@@ -554,4 +569,9 @@ private Map<String, String> processEnvironmentXml(final Path directory) {
554569
}
555570
return items;
556571
}
572+
573+
private static boolean isValidAttachmentFileName(final String fileName) {
574+
return Objects.nonNull(fileName) && ATTACHMENT_SOURCE_PATTERN.matcher(fileName).matches();
575+
576+
}
557577
}

‎allure-generator/src/main/java/io/qameta/allure/allure2/Allure2Plugin.java‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import java.io.InputStream;
4343
import java.nio.file.DirectoryStream;
4444
import java.nio.file.Files;
45+
import java.nio.file.LinkOption;
4546
import java.nio.file.Path;
4647
import java.util.ArrayList;
4748
import java.util.Comparator;
@@ -54,6 +55,7 @@
5455
import java.util.TreeSet;
5556
import java.util.concurrent.ConcurrentHashMap;
5657
import java.util.function.Supplier;
58+
import java.util.regex.Pattern;
5759
import java.util.stream.Collectors;
5860
import java.util.stream.Stream;
5961
import java.util.stream.StreamSupport;
@@ -87,6 +89,8 @@ public class Allure2Plugin implements Reader {
8789

8890
private static final Logger LOGGER = LoggerFactory.getLogger(Allure2Plugin.class);
8991

92+
private static final Pattern ATTACHMENT_SOURCE_PATTERN = Pattern.compile("^[a-zA-Z0-9._-]{1,100}$");
93+
9094
private static final Comparator<StageResult> BY_START = comparing(
9195
StageResult::getTime,
9296
nullsLast(comparing(Time::getStart, nullsLast(naturalOrder())))
@@ -264,8 +268,19 @@ private Parameter convert(final io.qameta.allure.model.Parameter parameter) {
264268
private Attachment convert(final Path source,
265269
final ResultsVisitor visitor,
266270
final io.qameta.allure.model.Attachment attachment) {
267-
final Path attachmentFile = source.resolve(attachment.getSource());
268-
if (Files.isRegularFile(attachmentFile)) {
271+
final String attachmentSource = attachment.getSource();
272+
if (!isValidAttachmentFileName(attachmentSource)) {
273+
visitor.error("Invalid attachment source is provided: " + attachmentSource);
274+
return new Attachment()
275+
.setType(attachment.getType())
276+
.setName(attachment.getName())
277+
.setSize(0L);
278+
}
279+
280+
final Path attachmentFile = source.resolve(attachmentSource).normalize();
281+
282+
if (attachmentFile.startsWith(source)
283+
&& Files.isRegularFile(attachmentFile, LinkOption.NOFOLLOW_LINKS)) {
269284
final Attachment found = visitor.visitAttachmentFile(attachmentFile);
270285
if (nonNull(attachment.getType())) {
271286
found.setType(attachment.getType());
@@ -279,7 +294,7 @@ private Attachment convert(final Path source,
279294
}
280295
return found;
281296
} else {
282-
visitor.error("Could not find attachment " + attachment.getSource() + " in directory " + source);
297+
visitor.error("Could not find attachment " + attachmentSource + " in directory " + source);
283298
return new Attachment()
284299
.setType(attachment.getType())
285300
.setName(attachment.getName())
@@ -420,4 +435,9 @@ private Stream<Path> listFiles(final Path directory, final String glob) {
420435
return Stream.empty();
421436
}
422437
}
438+
439+
private static boolean isValidAttachmentFileName(final String fileName) {
440+
return nonNull(fileName) && ATTACHMENT_SOURCE_PATTERN.matcher(fileName).matches();
441+
442+
}
423443
}

‎allure-generator/src/test/java/io/qameta/allure/allure1/Allure1PluginTest.java‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,61 @@ void shouldPreserveContentTypeFromAttachment() throws IOException {
355355
assertThat(attachments.get(0).getSource()).endsWith(".txt");
356356
}
357357

358+
359+
@Test
360+
void shouldNotAllowInvalidCharactersInAttachmentSource() throws IOException {
361+
final LaunchResults results = process(
362+
"allure1/text-attachment-bad-source.xml", generateTestSuiteXmlName(),
363+
"allure1/secret-file.txt", "secret-file.txt"
364+
);
365+
366+
assertThat(results.getAttachments())
367+
.isEmpty();
368+
369+
}
370+
371+
@Test
372+
void shouldNotAllowAttachmentSourcePathTraversal() throws IOException {
373+
final Path allureResultsDir = directory.resolve("allure-results");
374+
Files.createDirectory(allureResultsDir);
375+
376+
copyFile(allureResultsDir, "allure1/text-attachment-path-traversal.xml", generateTestSuiteXmlName());
377+
copyFile(directory, "allure1/secret-file.txt", "secret-file.txt");
378+
379+
final Allure1Plugin reader = new Allure1Plugin();
380+
final Configuration configuration = ConfigurationBuilder.bundled().build();
381+
final DefaultResultsVisitor resultsVisitor = new DefaultResultsVisitor(configuration);
382+
reader.readResults(configuration, resultsVisitor, allureResultsDir);
383+
384+
final LaunchResults results = resultsVisitor.getLaunchResults();
385+
386+
assertThat(results.getAttachments())
387+
.isEmpty();
388+
389+
}
390+
391+
@Test
392+
void shouldNotAllowAttachmentSourceSymbolicLink() throws IOException {
393+
final Path allureResultsDir = directory.resolve("allure-results");
394+
Files.createDirectory(allureResultsDir);
395+
396+
copyFile(allureResultsDir, "allure1/text-attachment-link.xml", generateTestSuiteXmlName());
397+
copyFile(allureResultsDir, "allure1/secret-file.txt", "secret-file.txt");
398+
399+
Files.createSymbolicLink(allureResultsDir.resolve("link.txt"), allureResultsDir.resolve("secret-file.txt"));
400+
401+
final Allure1Plugin reader = new Allure1Plugin();
402+
final Configuration configuration = ConfigurationBuilder.bundled().build();
403+
final DefaultResultsVisitor resultsVisitor = new DefaultResultsVisitor(configuration);
404+
reader.readResults(configuration, resultsVisitor, allureResultsDir);
405+
406+
final LaunchResults results = resultsVisitor.getLaunchResults();
407+
408+
assertThat(results.getAttachments())
409+
.isEmpty();
410+
411+
}
412+
358413
private LaunchResults process(String... strings) throws IOException {
359414
Iterator<String> iterator = Arrays.asList(strings).iterator();
360415
while (iterator.hasNext()) {

‎allure-generator/src/test/java/io/qameta/allure/allure2/Allure2PluginTest.java‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,60 @@ void shouldPreserveContentTypeFromAttachment() throws IOException {
341341
assertThat(attachments.get(0).getSource()).endsWith(".txt");
342342
}
343343

344+
@Test
345+
void shouldNotAllowInvalidCharactersInAttachmentSource() throws IOException {
346+
final LaunchResults results = process(
347+
"allure2/text-attachment-bad-source.json", generateTestResultName(),
348+
"allure2/secret-file.txt", "secret-file.txt"
349+
);
350+
351+
assertThat(results.getAttachments())
352+
.isEmpty();
353+
354+
}
355+
356+
@Test
357+
void shouldNotAllowAttachmentSourcePathTraversal() throws IOException {
358+
final Path allureResultsDir = directory.resolve("allure-results");
359+
Files.createDirectory(allureResultsDir);
360+
361+
copyFile(allureResultsDir, "allure2/text-attachment-path-traversal.json", generateTestResultName());
362+
copyFile(directory, "allure2/secret-file.txt", "secret-file.txt");
363+
364+
final Allure2Plugin reader = new Allure2Plugin();
365+
final Configuration configuration = ConfigurationBuilder.bundled().build();
366+
final DefaultResultsVisitor resultsVisitor = new DefaultResultsVisitor(configuration);
367+
reader.readResults(configuration, resultsVisitor, allureResultsDir);
368+
369+
final LaunchResults results = resultsVisitor.getLaunchResults();
370+
371+
assertThat(results.getAttachments())
372+
.isEmpty();
373+
374+
}
375+
376+
@Test
377+
void shouldNotAllowAttachmentSourceSymbolicLink() throws IOException {
378+
final Path allureResultsDir = directory.resolve("allure-results");
379+
Files.createDirectory(allureResultsDir);
380+
381+
copyFile(allureResultsDir, "allure2/text-attachment-link.json", generateTestResultName());
382+
copyFile(allureResultsDir, "allure2/secret-file.txt", "secret-file.txt");
383+
384+
Files.createSymbolicLink(allureResultsDir.resolve("link.txt"), allureResultsDir.resolve("secret-file.txt"));
385+
386+
final Allure2Plugin reader = new Allure2Plugin();
387+
final Configuration configuration = ConfigurationBuilder.bundled().build();
388+
final DefaultResultsVisitor resultsVisitor = new DefaultResultsVisitor(configuration);
389+
reader.readResults(configuration, resultsVisitor, allureResultsDir);
390+
391+
final LaunchResults results = resultsVisitor.getLaunchResults();
392+
393+
assertThat(results.getAttachments())
394+
.isEmpty();
395+
396+
}
397+
344398
private LaunchResults process(String... strings) throws IOException {
345399
Iterator<String> iterator = Arrays.asList(strings).iterator();
346400
while (iterator.hasNext()) {

‎allure-generator/src/test/resources/allure1/secret-file.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
SECRET CONTENTS

‎allure-generator/src/test/resources/allure1/text-attachment-bad-source.xml‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
2+
<ns2:test-suite xmlns:ns2="urn:model.allure.qatools.yandex.ru" start="1412949538848" stop="1412949560045" version="1.4.4-SNAPSHOT">
3+
<name>my.company.AlwaysPassingTest</name>
4+
<test-cases>
5+
<test-case start="1412949539363" stop="1412949539730" status="passed">
6+
<name>testFour</name>
7+
<steps/>
8+
<attachments>
9+
<attachment title="Attachment 1" source="./secret-file.txt" type="text/plain"/>
10+
<attachment title="Attachment 2" source="nested/path/secret-file.txt" type="text/plain"/>
11+
<attachment title="Attachment 3" source=" " type="text/plain"/>
12+
<attachment title="Attachment 4" source="#####.txt" type="text/plain"/>
13+
</attachments>
14+
</test-case>
15+
</test-cases>
16+
</ns2:test-suite>

‎allure-generator/src/test/resources/allure1/text-attachment-link.xml‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
2+
<ns2:test-suite xmlns:ns2="urn:model.allure.qatools.yandex.ru" start="1412949538848" stop="1412949560045" version="1.4.4-SNAPSHOT">
3+
<name>my.company.AlwaysPassingTest</name>
4+
<test-cases>
5+
<test-case start="1412949539363" stop="1412949539730" status="passed">
6+
<name>testFour</name>
7+
<steps/>
8+
<attachments>
9+
<attachment title="String attachment in test" source="link.txt" type="text/plain"/>
10+
</attachments>
11+
</test-case>
12+
</test-cases>
13+
</ns2:test-suite>

‎allure-generator/src/test/resources/allure1/text-attachment-path-traversal.xml‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
2+
<ns2:test-suite xmlns:ns2="urn:model.allure.qatools.yandex.ru" start="1412949538848" stop="1412949560045" version="1.4.4-SNAPSHOT">
3+
<name>my.company.AlwaysPassingTest</name>
4+
<test-cases>
5+
<test-case start="1412949539363" stop="1412949539730" status="passed">
6+
<name>testFour</name>
7+
<steps/>
8+
<attachments>
9+
<attachment title="String attachment in test" source="../secret-file.txt" type="text/plain"/>
10+
</attachments>
11+
</test-case>
12+
</test-cases>
13+
</ns2:test-suite>

‎allure-generator/src/test/resources/allure2/secret-file.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
SECRET CONTENTS

‎allure-generator/src/test/resources/allure2/text-attachment-bad-source.json‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
{
2+
"status": "passed",
3+
"name": "shouldCreate",
4+
"start": 1479552611395,
5+
"stop": 1479552611399,
6+
"attachments": [
7+
{
8+
"name": "Attachment 1",
9+
"source": "./secret-file.txt",
10+
"type": "text/plain"
11+
},
12+
{
13+
"name": "Attachment 2",
14+
"source": "nested/path/secret-file.txt",
15+
"type": "text/plain"
16+
},
17+
{
18+
"name": "Attachment 3",
19+
"source": " ",
20+
"type": "text/plain"
21+
},
22+
{
23+
"name": "Attachment 4",
24+
"source": "#####.txt",
25+
"type": "text/plain"
26+
}
27+
]
28+
}

0 commit comments

Comments
 (0)