Skip to content

Commit a3ac1a8

Browse files
infeoinfeo
authored
Merge pull request #122 from cryptomator/hotfix/121-do-not-add-nullbytes
Extends the shortenend names check by basic syntax analysis of the string contained in `name.c9s`. Adds two new results for indicating incorrect syntax (not fixable) and only trailing bytes in string (fixable). Fixes #121.
2 parents a91cc52 + a4efa39 commit a3ac1a8

File tree

7 files changed

+238
-2
lines changed

7 files changed

+238
-2
lines changed

‎src/main/java/org/cryptomator/cryptofs/LongFileNameProvider.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ public void persist() {
113113
private void persistInternal() throws IOException {
114114
Path longNameFile = c9sPath.resolve(INFLATED_FILE_NAME);
115115
Files.createDirectories(c9sPath);
116-
Files.write(longNameFile,UTF_8.encode(longName).array()); //WRITE, CREATE, TRUNCATE_EXISTING
116+
Files.writeString(longNameFile, longName, UTF_8, StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.WRITE, StandardOpenOption.CREATE);
117117
}
118118
}
119119

‎src/main/java/org/cryptomator/cryptofs/health/shortened/NotDecodableLongName.java‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
package org.cryptomator.cryptofs.health.shortened;
2+
3+
import org.cryptomator.cryptofs.health.api.CommonDetailKeys;
4+
import org.cryptomator.cryptofs.health.api.DiagnosticResult;
5+
6+
import java.nio.file.Path;
7+
import java.util.Map;
8+
9+
/**
10+
* A name.c9s file with a syntactical <em>incorrect</em> string.
11+
* <p>
12+
* A string is only correct if
13+
* <ul>
14+
* <li> it ends with {@value org.cryptomator.cryptofs.common.Constants#CRYPTOMATOR_FILE_SUFFIX} and </li>
15+
* <li> excluding the aforementioned suffix, is base64url encoded</li>
16+
* </ul>
17+
* <p>
18+
* A special case represents the diagnostic result {@link TrailingBytesInNameFile}.
19+
*
20+
* @see TrailingBytesInNameFile
21+
*/
22+
public class NotDecodableLongName implements DiagnosticResult {
23+
24+
private final Path nameFile;
25+
private final String longName;
26+
27+
public NotDecodableLongName(Path nameFile, String longName) {
28+
this.nameFile = nameFile;
29+
this.longName = longName;
30+
}
31+
32+
@Override
33+
public Severity getSeverity() {
34+
return Severity.CRITICAL;
35+
}
36+
37+
38+
@Override
39+
public Map<String, String> details() {
40+
return Map.of(CommonDetailKeys.ENCRYPTED_PATH, nameFile.toString(), //
41+
"Stored String", longName);
42+
}
43+
}

‎src/main/java/org/cryptomator/cryptofs/health/shortened/ShortenedNamesCheck.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,16 @@ void checkShortenedName(Path dir) throws IOException {
9595
}
9696

9797
var longName = Files.readString(nameFile, UTF_8);
98+
99+
var syntaxResult = checkSyntax(longName);
100+
if (syntaxResult == SyntaxResult.INVALID) {
101+
resultCollector.accept(new NotDecodableLongName(nameFile, longName));
102+
return;
103+
} else if (syntaxResult == SyntaxResult.TRAILING_BYTES) {
104+
resultCollector.accept(new TrailingBytesInNameFile(nameFile, longName));
105+
return;
106+
}
107+
98108
var expectedShortName = deflate(longName);
99109
if (!dir.getFileName().toString().equals(expectedShortName)) {
100110
resultCollector.accept(new LongShortNamesMismatch(dir, expectedShortName));
@@ -103,6 +113,38 @@ void checkShortenedName(Path dir) throws IOException {
103113
}
104114
}
105115

116+
117+
/**
118+
* Determines if the string stored inside the name file is a base64url encoded ending with {@value Constants#CRYPTOMATOR_FILE_SUFFIX}.
119+
*
120+
* <em>visible for testing</em>
121+
*
122+
* @return {@link SyntaxResult} indicating if it is valid, invalid or affected by https://github.com/cryptomator/cryptofs/issues/121
123+
*/
124+
SyntaxResult checkSyntax(String toAnalyse) {
125+
int posObligatoryC9rString = toAnalyse.indexOf(Constants.CRYPTOMATOR_FILE_SUFFIX);
126+
if (posObligatoryC9rString == -1) {
127+
return SyntaxResult.INVALID;
128+
}
129+
130+
var encryptedFileName = toAnalyse.substring(0, posObligatoryC9rString);
131+
if (!BASE64URL.canDecode(encryptedFileName)) {
132+
return SyntaxResult.INVALID;
133+
}
134+
135+
if (toAnalyse.substring(posObligatoryC9rString).length() > Constants.CRYPTOMATOR_FILE_SUFFIX.length()) {
136+
return SyntaxResult.TRAILING_BYTES;
137+
}
138+
139+
return SyntaxResult.VALID;
140+
}
141+
142+
enum SyntaxResult {
143+
VALID,
144+
INVALID,
145+
TRAILING_BYTES; //to indicate issue https://github.com/cryptomator/cryptofs/issues/121
146+
}
147+
106148
//visible for testing
107149
String deflate(String longFileName) {
108150
byte[] longFileNameBytes = longFileName.getBytes(UTF_8);

‎src/main/java/org/cryptomator/cryptofs/health/shortened/TrailingBytesInNameFile.java‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
package org.cryptomator.cryptofs.health.shortened;
2+
3+
import org.cryptomator.cryptofs.VaultConfig;
4+
import org.cryptomator.cryptofs.health.api.CommonDetailKeys;
5+
import org.cryptomator.cryptofs.health.api.DiagnosticResult;
6+
import org.cryptomator.cryptolib.api.Cryptor;
7+
import org.cryptomator.cryptolib.api.Masterkey;
8+
9+
import java.io.IOException;
10+
import java.nio.charset.StandardCharsets;
11+
import java.nio.file.Files;
12+
import java.nio.file.Path;
13+
import java.nio.file.StandardOpenOption;
14+
import java.util.Map;
15+
16+
import static org.cryptomator.cryptofs.common.Constants.CRYPTOMATOR_FILE_SUFFIX;
17+
18+
/**
19+
* Result and fix for bug https://github.com/cryptomator/cryptofs/issues/121
20+
*/
21+
public class TrailingBytesInNameFile implements DiagnosticResult {
22+
23+
private final Path nameFile;
24+
private final String longName;
25+
26+
public TrailingBytesInNameFile(Path nameFile, String longName) {
27+
this.nameFile = nameFile;
28+
this.longName = longName;
29+
}
30+
31+
@Override
32+
public Severity getSeverity() {
33+
return Severity.WARN;
34+
}
35+
36+
@Override
37+
public void fix(Path pathToVault, VaultConfig config, Masterkey masterkey, Cryptor cryptor) throws IOException {
38+
var startIndexTrailingBytes = longName.indexOf(CRYPTOMATOR_FILE_SUFFIX) + CRYPTOMATOR_FILE_SUFFIX.length();
39+
Files.writeString(pathToVault.resolve(nameFile), //
40+
longName.substring(0, startIndexTrailingBytes), //
41+
StandardCharsets.UTF_8, //
42+
StandardOpenOption.WRITE, StandardOpenOption.TRUNCATE_EXISTING);
43+
}
44+
45+
@Override
46+
public Map<String, String> details() {
47+
return Map.of(CommonDetailKeys.ENCRYPTED_PATH, nameFile.toString(), //
48+
"Encrypted Long Name", longName);
49+
}
50+
}

‎src/test/java/org/cryptomator/cryptofs/LongFileNameProviderTest.java‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import org.mockito.Mockito;
1515

1616
import java.io.IOException;
17+
import java.nio.charset.StandardCharsets;
1718
import java.nio.file.FileVisitResult;
1819
import java.nio.file.Files;
1920
import java.nio.file.NoSuchFileException;
@@ -90,7 +91,7 @@ public void testDeflateMultipleTimes(@TempDir Path tmpPath) {
9091
}
9192

9293
@Test
93-
public void testPerstistCachedFailsOnReadOnlyFileSystems(@TempDir Path tmpPath) {
94+
public void testPersistCachedFailsOnReadOnlyFileSystems(@TempDir Path tmpPath) {
9495
LongFileNameProvider prov = new LongFileNameProvider(readonlyFlag);
9596

9697
String orig = "longName";
@@ -103,4 +104,16 @@ public void testPerstistCachedFailsOnReadOnlyFileSystems(@TempDir Path tmpPath)
103104
});
104105
}
105106

107+
@Test
108+
public void testPersistedStringEqualsLongName(@TempDir Path tmpPath) throws IOException {
109+
LongFileNameProvider prov = new LongFileNameProvider(readonlyFlag);
110+
111+
String orig = "LongNameOf200Chars00_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000_0000";
112+
Path canonicalFileName = tmpPath.resolve(orig);
113+
LongFileNameProvider.DeflatedFileName deflated = prov.deflate(canonicalFileName);
114+
115+
deflated.persist();
116+
Assertions.assertEquals(Files.readString(deflated.c9sPath.resolve("name.c9s"), StandardCharsets.UTF_8),deflated.longName);
117+
}
118+
106119
}

‎src/test/java/org/cryptomator/cryptofs/health/shortened/ShortenedNamesCheckTest.java‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
import java.util.Set;
2424
import java.util.function.Consumer;
2525

26+
import static org.cryptomator.cryptofs.health.shortened.ShortenedNamesCheck.DirVisitor.SyntaxResult.VALID;
27+
2628
public class ShortenedNamesCheckTest {
2729

2830
private FileSystem fs;
@@ -76,6 +78,7 @@ public void testExistingNamesFileAndMatchingNamesProducesValidResult() throws IO
7678

7779
var visitorSpy = Mockito.spy(visitor);
7880
Mockito.doReturn("shortName.c9s").when(visitorSpy).deflate(longName);
81+
Mockito.doReturn(VALID).when(visitorSpy).checkSyntax(longName);
7982

8083
visitorSpy.checkShortenedName(dir);
8184
ArgumentCaptor<DiagnosticResult> resultCaptor = ArgumentCaptor.forClass(DiagnosticResult.class);
@@ -126,12 +129,44 @@ public void testMismatchingNamesProducesMismatchResult() throws IOException {
126129

127130
var visitorSpy = Mockito.spy(visitor);
128131
Mockito.doReturn("otherName.c9s").when(visitorSpy).deflate(longName);
132+
Mockito.doReturn(VALID).when(visitorSpy).checkSyntax(longName);
129133

130134
visitorSpy.checkShortenedName(dir);
131135
ArgumentCaptor<DiagnosticResult> resultCaptor = ArgumentCaptor.forClass(DiagnosticResult.class);
132136
Mockito.verify(resultsCollector).accept(resultCaptor.capture());
133137
MatcherAssert.assertThat(resultCaptor.getValue(), Matchers.instanceOf(LongShortNamesMismatch.class));
134138
}
135139

140+
@Test
141+
@DisplayName("dir with non base64url content in name.c9s produces illegal encoding result")
142+
public void testNonBase64URLCharsInNameFileProducesIllegalEncodingResult() throws IOException {
143+
String longName = "Bug##121\0\0\0";
144+
Path dir = dataRoot.resolve("AA/zzzz/shortName.c9s");
145+
Path nameFile = dir.resolve("name.c9s");
146+
Files.createDirectories(dir);
147+
Files.writeString(nameFile, longName, StandardCharsets.UTF_8, StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE);
148+
149+
visitor.checkShortenedName(dir);
150+
ArgumentCaptor<DiagnosticResult> resultCaptor = ArgumentCaptor.forClass(DiagnosticResult.class);
151+
Mockito.verify(resultsCollector).accept(resultCaptor.capture());
152+
MatcherAssert.assertThat(resultCaptor.getValue(), Matchers.instanceOf(NotDecodableLongName.class));
153+
}
154+
155+
@Test
156+
@DisplayName("dir with non base64url content in name.c9s produces illegal encoding result")
157+
public void testNameFileWithTrailingNullBytesProducesIllegalEncodingResult() throws IOException {
158+
String longName = "VGhpc0lzQVRlc3Q.c9r\0\0\u0002\0"; //" base64url("ThisIsATest") + ".c9r" + "\0\0\0"
159+
160+
Path dir = dataRoot.resolve("AA/zzzz/shortName.c9s");
161+
Path nameFile = dir.resolve("name.c9s");
162+
Files.createDirectories(dir);
163+
Files.writeString(nameFile, longName, StandardCharsets.UTF_8, StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE);
164+
165+
visitor.checkShortenedName(dir);
166+
ArgumentCaptor<DiagnosticResult> resultCaptor = ArgumentCaptor.forClass(DiagnosticResult.class);
167+
Mockito.verify(resultsCollector).accept(resultCaptor.capture());
168+
MatcherAssert.assertThat(resultCaptor.getValue(), Matchers.instanceOf(TrailingBytesInNameFile.class));
169+
}
170+
136171
}
137172
}

‎src/test/java/org/cryptomator/cryptofs/health/shortened/TrailingNullBytesInNameFileTest.java‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
package org.cryptomator.cryptofs.health.shortened;
2+
3+
import org.cryptomator.cryptofs.VaultConfig;
4+
import org.cryptomator.cryptolib.api.Cryptor;
5+
import org.cryptomator.cryptolib.api.Masterkey;
6+
import org.junit.jupiter.api.Assertions;
7+
import org.junit.jupiter.api.BeforeEach;
8+
import org.junit.jupiter.api.DisplayName;
9+
import org.junit.jupiter.api.Test;
10+
import org.junit.jupiter.api.io.TempDir;
11+
import org.mockito.Mockito;
12+
13+
import java.io.IOException;
14+
import java.nio.charset.StandardCharsets;
15+
import java.nio.file.Files;
16+
import java.nio.file.Path;
17+
import java.nio.file.StandardOpenOption;
18+
19+
public class TrailingNullBytesInNameFileTest {
20+
21+
@TempDir
22+
public Path pathToVault;
23+
24+
private TrailingBytesInNameFile result;
25+
private Path dataDir;
26+
private Path cipherDir;
27+
28+
@BeforeEach
29+
public void init() throws IOException {
30+
dataDir = pathToVault.resolve("d");
31+
cipherDir = dataDir.resolve("00/0000");
32+
Files.createDirectories(cipherDir);
33+
}
34+
35+
@Test
36+
@DisplayName("Successful fix only removes trailing null bytes")
37+
public void testSuccessfulFixRemovesTrailingNullBytes() throws IOException {
38+
//prepare
39+
Path c9sDir = cipherDir.resolve("foo==.c9s");
40+
Path nameFile = c9sDir.resolve("name.c9s");
41+
var longName = "bar==.c9r\0\0\0";
42+
result = new TrailingBytesInNameFile(nameFile, longName );
43+
44+
Files.createDirectory(c9sDir);
45+
Files.writeString(nameFile, longName, StandardCharsets.UTF_8, StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE);
46+
47+
//execute
48+
result.fix(pathToVault, Mockito.mock(VaultConfig.class), Mockito.mock(Masterkey.class), Mockito.mock(Cryptor.class));
49+
50+
//evaluate
51+
Assertions.assertEquals("bar==.c9r", Files.readString(nameFile,StandardCharsets.UTF_8));
52+
}
53+
}

0 commit comments

Comments
 (0)