Browse Source

on-the-fly MAC calculation for better performance (addresses issue #38)
we still need to add some kind of warning on the UI and create an async MAC checker for ranged requests

Sebastian Stenzel 10 năm trước cách đây
mục cha
commit
2849e39e85

+ 4 - 5
main/core/src/main/java/org/cryptomator/webdav/jackrabbit/resources/EncryptedDir.java

@@ -11,7 +11,6 @@ package org.cryptomator.webdav.jackrabbit.resources;
 import java.io.IOException;
 import java.nio.channels.SeekableByteChannel;
 import java.nio.file.DirectoryStream;
-import java.nio.file.FileAlreadyExistsException;
 import java.nio.file.FileVisitResult;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -79,12 +78,10 @@ public class EncryptedDir extends AbstractEncryptedNode {
 
 	private void addMemberFile(DavResource resource, InputContext inputContext) throws DavException {
 		final Path childPath = ResourcePathUtils.getPhysicalPath(resource);
-		try (final SeekableByteChannel channel = Files.newByteChannel(childPath, StandardOpenOption.WRITE, StandardOpenOption.CREATE_NEW)) {
+		try (final SeekableByteChannel channel = Files.newByteChannel(childPath, StandardOpenOption.WRITE, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING)) {
 			cryptor.encryptFile(inputContext.getInputStream(), channel);
 		} catch (SecurityException e) {
 			throw new DavException(DavServletResponse.SC_FORBIDDEN, e);
-		} catch (FileAlreadyExistsException e) {
-			throw new DavException(DavServletResponse.SC_CONFLICT, e);
 		} catch (IOException e) {
 			LOG.error("Failed to create file.", e);
 			throw new IORuntimeException(e);
@@ -124,7 +121,9 @@ public class EncryptedDir extends AbstractEncryptedNode {
 	public void removeMember(DavResource member) throws DavException {
 		final Path memberPath = ResourcePathUtils.getPhysicalPath(member);
 		try {
-			Files.walkFileTree(memberPath, new DeletingFileVisitor());
+			if (Files.exists(memberPath)) {
+				Files.walkFileTree(memberPath, new DeletingFileVisitor());
+			}
 		} catch (SecurityException e) {
 			throw new DavException(DavServletResponse.SC_FORBIDDEN, e);
 		} catch (IOException e) {

+ 8 - 2
main/core/src/main/java/org/cryptomator/webdav/jackrabbit/resources/EncryptedFile.java

@@ -29,6 +29,7 @@ import org.apache.jackrabbit.webdav.property.DavPropertyName;
 import org.apache.jackrabbit.webdav.property.DefaultDavProperty;
 import org.cryptomator.crypto.Cryptor;
 import org.cryptomator.crypto.exceptions.DecryptFailedException;
+import org.cryptomator.crypto.exceptions.MacAuthenticationFailedException;
 import org.cryptomator.webdav.exceptions.IORuntimeException;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpHeaderValue;
@@ -70,12 +71,17 @@ public class EncryptedFile extends AbstractEncryptedNode {
 			outputContext.setModificationTime(Files.getLastModifiedTime(path).toMillis());
 			outputContext.setProperty(HttpHeader.ACCEPT_RANGES.asString(), HttpHeaderValue.BYTES.asString());
 			try (final SeekableByteChannel channel = Files.newByteChannel(path, StandardOpenOption.READ)) {
-				outputContext.setContentLength(cryptor.decryptedContentLength(channel));
+				final Long contentLength = cryptor.decryptedContentLength(channel);
+				if (contentLength != null) {
+					outputContext.setContentLength(contentLength);
+				}
 				if (outputContext.hasStream()) {
-					cryptor.decryptedFile(channel, outputContext.getOutputStream());
+					cryptor.decryptFile(channel, outputContext.getOutputStream());
 				}
 			} catch (EOFException e) {
 				LOG.warn("Unexpected end of stream (possibly client hung up).");
+			} catch (MacAuthenticationFailedException e) {
+				LOG.warn("MAC authentication failed, file content {} might be compromised.", getLocator().getResourcePath());
 			} catch (DecryptFailedException e) {
 				throw new IOException("Error decrypting file " + path.toString(), e);
 			}

+ 56 - 69
main/crypto-aes/src/main/java/org/cryptomator/crypto/aes256/Aes256Cryptor.java

@@ -47,6 +47,7 @@ import org.bouncycastle.crypto.generators.SCrypt;
 import org.cryptomator.crypto.AbstractCryptor;
 import org.cryptomator.crypto.CryptorIOSupport;
 import org.cryptomator.crypto.exceptions.DecryptFailedException;
+import org.cryptomator.crypto.exceptions.MacAuthenticationFailedException;
 import org.cryptomator.crypto.exceptions.UnsupportedKeyLengthException;
 import org.cryptomator.crypto.exceptions.WrongPasswordException;
 import org.cryptomator.crypto.io.SeekableByteChannelInputStream;
@@ -369,34 +370,6 @@ public class Aes256Cryptor extends AbstractCryptor implements AesCryptographicCo
 		ioSupport.writePathSpecificMetadata(metadataFile, objectMapper.writeValueAsBytes(metadata));
 	}
 
-	private void authenticateContent(SeekableByteChannel encryptedFile) throws IOException, DecryptFailedException {
-		// init mac:
-		final Mac calculatedMac = this.hmacSha256(hMacMasterKey);
-
-		// read stored mac:
-		encryptedFile.position(16);
-		final ByteBuffer storedMac = ByteBuffer.allocate(calculatedMac.getMacLength());
-		final int numMacBytesRead = encryptedFile.read(storedMac);
-
-		// check validity of header:
-		if (numMacBytesRead != calculatedMac.getMacLength()) {
-			throw new IOException("Failed to read file header.");
-		}
-
-		// read all encrypted data and calculate mac:
-		encryptedFile.position(64);
-		final InputStream in = new SeekableByteChannelInputStream(encryptedFile);
-		final InputStream macIn = new MacInputStream(in, calculatedMac);
-		IOUtils.copyLarge(macIn, new NullOutputStream());
-
-		// compare (in constant time):
-		boolean macMatches = MessageDigest.isEqual(storedMac.array(), calculatedMac.doFinal());
-
-		if (!macMatches) {
-			throw new DecryptFailedException("MAC authentication failed.");
-		}
-	}
-
 	@Override
 	public Long decryptedContentLength(SeekableByteChannel encryptedFile) throws IOException {
 		// skip 128bit IV + 256 bit MAC:
@@ -422,24 +395,49 @@ public class Aes256Cryptor extends AbstractCryptor implements AesCryptographicCo
 		}
 	}
 
+	private void encryptedContentLength(SeekableByteChannel encryptedFile, Long contentLength) throws IOException {
+		final ByteBuffer encryptedFileSizeBuffer;
+
+		// encrypt content length in ECB mode (content length is less than one block):
+		try {
+			final ByteBuffer fileSizeBuffer = ByteBuffer.allocate(Long.BYTES);
+			fileSizeBuffer.putLong(contentLength);
+			final Cipher sizeCipher = aesEcbCipher(primaryMasterKey, Cipher.ENCRYPT_MODE);
+			final byte[] encryptedFileSize = sizeCipher.doFinal(fileSizeBuffer.array());
+			encryptedFileSizeBuffer = ByteBuffer.wrap(encryptedFileSize);
+		} catch (IllegalBlockSizeException | BadPaddingException e) {
+			throw new IllegalStateException("Block size must be valid, as padding is requested. BadPaddingException not possible in encrypt mode.", e);
+		}
+
+		// skip 128bit IV + 256 bit MAC:
+		encryptedFile.position(48);
+
+		// write result:
+		encryptedFile.write(encryptedFileSizeBuffer);
+	}
+
 	@Override
-	public Long decryptedFile(SeekableByteChannel encryptedFile, OutputStream plaintextFile) throws IOException, DecryptFailedException {
+	public Long decryptFile(SeekableByteChannel encryptedFile, OutputStream plaintextFile) throws IOException, DecryptFailedException {
 		// read iv:
 		encryptedFile.position(0);
 		final ByteBuffer countingIv = ByteBuffer.allocate(AES_BLOCK_LENGTH);
 		final int numIvBytesRead = encryptedFile.read(countingIv);
 
+		// init mac:
+		final Mac calculatedMac = this.hmacSha256(hMacMasterKey);
+
+		// read stored mac:
+		final ByteBuffer storedMac = ByteBuffer.allocate(calculatedMac.getMacLength());
+		final int numMacBytesRead = encryptedFile.read(storedMac);
+
 		// read file size:
 		final Long fileSize = decryptedContentLength(encryptedFile);
 
 		// check validity of header:
-		if (numIvBytesRead != AES_BLOCK_LENGTH || fileSize == null) {
+		if (numIvBytesRead != AES_BLOCK_LENGTH || numMacBytesRead != calculatedMac.getMacLength() || fileSize == null) {
 			throw new IOException("Failed to read file header.");
 		}
 
-		// check MAC:
-		this.authenticateContent(encryptedFile);
-
 		// go to begin of content:
 		encryptedFile.position(64);
 
@@ -448,8 +446,25 @@ public class Aes256Cryptor extends AbstractCryptor implements AesCryptographicCo
 
 		// read content
 		final InputStream in = new SeekableByteChannelInputStream(encryptedFile);
-		final InputStream cipheredIn = new CipherInputStream(in, cipher);
-		return IOUtils.copyLarge(cipheredIn, plaintextFile, 0, fileSize);
+		final InputStream macIn = new MacInputStream(in, calculatedMac);
+		final InputStream cipheredIn = new CipherInputStream(macIn, cipher);
+		final long bytesDecrypted = IOUtils.copyLarge(cipheredIn, plaintextFile, 0, fileSize);
+
+		// drain remaining bytes to /dev/null to complete MAC calculation:
+		IOUtils.copyLarge(macIn, new NullOutputStream());
+
+		// compare (in constant time):
+		final boolean macMatches = MessageDigest.isEqual(storedMac.array(), calculatedMac.doFinal());
+		if (!macMatches) {
+			// This exception will be thrown AFTER we sent the decrypted content to the user.
+			// This has two advantages:
+			// - we don't need to read files twice
+			// - we can still restore files suffering from non-malicious bit rotting
+			// Anyway me MUST make sure to warn the user. This will be done by the UI when catching this exception.
+			throw new MacAuthenticationFailedException("MAC authentication failed.");
+		}
+
+		return bytesDecrypted;
 	}
 
 	@Override
@@ -464,9 +479,6 @@ public class Aes256Cryptor extends AbstractCryptor implements AesCryptographicCo
 			throw new IOException("Failed to read file header.");
 		}
 
-		// check MAC:
-		this.authenticateContent(encryptedFile);
-
 		// seek relevant position and update iv:
 		long firstRelevantBlock = pos / AES_BLOCK_LENGTH; // cut of fraction!
 		long beginOfFirstRelevantBlock = firstRelevantBlock * AES_BLOCK_LENGTH;
@@ -493,7 +505,6 @@ public class Aes256Cryptor extends AbstractCryptor implements AesCryptographicCo
 		// use an IV, whose last 8 bytes store a long used in counter mode and write initial value to file.
 		final ByteBuffer countingIv = ByteBuffer.wrap(randomData(AES_BLOCK_LENGTH));
 		countingIv.putLong(AES_BLOCK_LENGTH - Long.BYTES, 0l);
-		countingIv.position(0);
 		encryptedFile.write(countingIv);
 
 		// init crypto stuff:
@@ -504,20 +515,8 @@ public class Aes256Cryptor extends AbstractCryptor implements AesCryptographicCo
 		final ByteBuffer macBuffer = ByteBuffer.allocate(mac.getMacLength());
 		encryptedFile.write(macBuffer);
 
-		// write initial file size = 0 into the next 16 bytes
-		final ByteBuffer encryptedFileSizeBuffer = ByteBuffer.allocate(AES_BLOCK_LENGTH);
-		try {
-			final ByteBuffer fileSizeBuffer = ByteBuffer.allocate(Long.BYTES);
-			fileSizeBuffer.putLong(0l);
-			final Cipher sizeCipher = aesEcbCipher(primaryMasterKey, Cipher.ENCRYPT_MODE);
-			final byte[] encryptedFileSize = sizeCipher.doFinal(fileSizeBuffer.array());
-			encryptedFileSizeBuffer.position(0);
-			encryptedFileSizeBuffer.put(encryptedFileSize);
-			encryptedFileSizeBuffer.position(0);
-			encryptedFile.write(encryptedFileSizeBuffer);
-		} catch (IllegalBlockSizeException | BadPaddingException e) {
-			throw new IllegalStateException("Block size must be valid, as padding is requested. BadPaddingException not possible in encrypt mode.", e);
-		}
+		// encrypt and write "zero length" as a placeholder, which will be read by concurrent requests, as long as encryption didn't finish:
+		encryptedContentLength(encryptedFile, 0l);
 
 		// write content:
 		final OutputStream out = new SeekableByteChannelOutputStream(encryptedFile);
@@ -540,26 +539,14 @@ public class Aes256Cryptor extends AbstractCryptor implements AesCryptographicCo
 		blockSizeBufferedOut.flush();
 
 		// write MAC of total ciphertext:
-		macBuffer.position(0);
+		macBuffer.clear();
 		macBuffer.put(mac.doFinal());
-		macBuffer.position(0);
+		macBuffer.flip();
 		encryptedFile.position(16); // right behind the IV
 		encryptedFile.write(macBuffer); // 256 bit MAC
 
-		// encrypt and write plaintextSize
-		try {
-			final ByteBuffer fileSizeBuffer = ByteBuffer.allocate(Long.BYTES);
-			fileSizeBuffer.putLong(plaintextSize);
-			final Cipher sizeCipher = aesEcbCipher(primaryMasterKey, Cipher.ENCRYPT_MODE);
-			final byte[] encryptedFileSize = sizeCipher.doFinal(fileSizeBuffer.array());
-			encryptedFileSizeBuffer.position(0);
-			encryptedFileSizeBuffer.put(encryptedFileSize);
-			encryptedFileSizeBuffer.position(0);
-			encryptedFile.position(48); // right behind the IV and MAC
-			encryptedFile.write(encryptedFileSizeBuffer);
-		} catch (IllegalBlockSizeException | BadPaddingException e) {
-			throw new IllegalStateException("Block size must be valid, as padding is requested. BadPaddingException not possible in encrypt mode.", e);
-		}
+		// encrypt and write plaintextSize:
+		encryptedContentLength(encryptedFile, plaintextSize);
 
 		return plaintextSize;
 	}

+ 2 - 2
main/crypto-aes/src/test/java/org/cryptomator/crypto/aes256/Aes256CryptorTest.java

@@ -98,7 +98,7 @@ public class Aes256CryptorTest {
 		// decrypt modified content (should fail with DecryptFailedException):
 		final SeekableByteChannel encryptedIn = new ByteBufferBackedSeekableChannel(encryptedData);
 		final ByteArrayOutputStream plaintextOut = new ByteArrayOutputStream();
-		cryptor.decryptedFile(encryptedIn, plaintextOut);
+		cryptor.decryptFile(encryptedIn, plaintextOut);
 	}
 
 	@Test
@@ -126,7 +126,7 @@ public class Aes256CryptorTest {
 
 		// decrypt:
 		final ByteArrayOutputStream plaintextOut = new ByteArrayOutputStream();
-		final Long numDecryptedBytes = cryptor.decryptedFile(encryptedIn, plaintextOut);
+		final Long numDecryptedBytes = cryptor.decryptFile(encryptedIn, plaintextOut);
 		IOUtils.closeQuietly(encryptedIn);
 		IOUtils.closeQuietly(plaintextOut);
 		Assert.assertEquals(filesize.longValue(), numDecryptedBytes.longValue());

+ 1 - 1
main/crypto-api/src/main/java/org/cryptomator/crypto/Cryptor.java

@@ -79,7 +79,7 @@ public interface Cryptor extends SensitiveDataSwipeListener {
 	 * @return Number of decrypted bytes. This might not be equal to the encrypted file size due to optional metadata written to it.
 	 * @throws DecryptFailedException If decryption failed
 	 */
-	Long decryptedFile(SeekableByteChannel encryptedFile, OutputStream plaintextFile) throws IOException, DecryptFailedException;
+	Long decryptFile(SeekableByteChannel encryptedFile, OutputStream plaintextFile) throws IOException, DecryptFailedException;
 
 	/**
 	 * @param pos First byte (inclusive)

+ 2 - 2
main/crypto-api/src/main/java/org/cryptomator/crypto/SamplingDecorator.java

@@ -82,9 +82,9 @@ public class SamplingDecorator implements Cryptor, CryptorIOSampling {
 	}
 
 	@Override
-	public Long decryptedFile(SeekableByteChannel encryptedFile, OutputStream plaintextFile) throws IOException, DecryptFailedException {
+	public Long decryptFile(SeekableByteChannel encryptedFile, OutputStream plaintextFile) throws IOException, DecryptFailedException {
 		final OutputStream countingInputStream = new CountingOutputStream(decryptedBytes, plaintextFile);
-		return cryptor.decryptedFile(encryptedFile, countingInputStream);
+		return cryptor.decryptFile(encryptedFile, countingInputStream);
 	}
 
 	@Override

+ 11 - 0
main/crypto-api/src/main/java/org/cryptomator/crypto/exceptions/MacAuthenticationFailedException.java

@@ -0,0 +1,11 @@
+package org.cryptomator.crypto.exceptions;
+
+public class MacAuthenticationFailedException extends DecryptFailedException {
+
+	private static final long serialVersionUID = -5577052361643658772L;
+
+	public MacAuthenticationFailedException(String msg) {
+		super(msg);
+	}
+
+}