You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/12/01 19:28:29 UTC
[GitHub] [commons-compress] kinow commented on a diff in pull request #332: Add encryption support for SevenZ
kinow commented on code in PR #332:
URL: https://github.com/apache/commons-compress/pull/332#discussion_r1037477537
##########
src/main/java/org/apache/commons/compress/utils/ByteUtils.java:
##########
@@ -266,4 +270,17 @@ private static void checkReadLength(final int length) {
throw new IllegalArgumentException("Can't read more than eight bytes into a long value");
}
}
+
+ public static byte[] utf16Decode(final char[] chars) {
Review Comment:
Missing javadoc for this public method. See other public methods for examples.
##########
src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFileTest.java:
##########
@@ -468,6 +473,40 @@ public void testArchiveWithMixedMethods() throws Exception {
}
}
+ /**
+ * Test password-baed encryption
Review Comment:
s/baed/based
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/AES256Options.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.archivers.sevenz;
+
+import java.util.Arrays;
+import java.util.Random;
+
+public class AES256Options {
+ byte[] password;
+ byte[] salt = new byte[0];
+ byte[] iv = new byte[16];
+ int numCyclesPower = 19;
+
+ public AES256Options(byte[] password) {
+ this.password = password;
Review Comment:
I don't know the Commons Compress API well enough, but I guess it'd be better if the password were never stored in an object in memory or serialized, if possible. This means instead of storing the object in the class, instead trying to design a method `public void decompress(byte[] data, byte[] password) {...}` that never stores the password.
But if other classes are already doing that, then I guess that should be fine. (Didn't have time to check, sorry)
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/AES256Options.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.archivers.sevenz;
+
+import java.util.Arrays;
+import java.util.Random;
+
+public class AES256Options {
+ byte[] password;
+ byte[] salt = new byte[0];
+ byte[] iv = new byte[16];
+ int numCyclesPower = 19;
+
+ public AES256Options(byte[] password) {
+ this.password = password;
+ new Random(Arrays.hashCode(password)).nextBytes(salt);
+ new Random(Arrays.hashCode(password)).nextBytes(iv);
Review Comment:
Do we need to worry about the `Random` instead of `SecureRandom` or some other random class from Commons Rng/etc?
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/AES256Options.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.commons.compress.archivers.sevenz;
+
+import java.util.Arrays;
+import java.util.Random;
+
+public class AES256Options {
Review Comment:
Missing javadoc for public class. Worth documenting its members (properties, methods) too, whenever possible (something short is fine).
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/AES256SHA256Decoder.java:
##########
@@ -126,4 +115,121 @@ public void close() throws IOException {
}
};
}
+
+ @Override
+ OutputStream encode(OutputStream out, Object options) throws IOException {
+ AES256Options opts = (AES256Options) options;
+ final byte[] aesKeyBytes = sha256Password(opts.password, opts.numCyclesPower, opts.salt);
+ final SecretKey aesKey = new SecretKeySpec(aesKeyBytes, "AES");
+
+ final Cipher cipher;
+ try {
+ cipher = Cipher.getInstance("AES/CBC/NoPadding");
+ cipher.init(Cipher.ENCRYPT_MODE, aesKey, new IvParameterSpec(opts.iv));
+ } catch (final GeneralSecurityException generalSecurityException) {
+ throw new IOException(
+ "Encryption error " + "(do you have the JCE Unlimited Strength Jurisdiction Policy Files installed?)",
Review Comment:
Unnecessary string concatenation?
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFile.java:
##########
@@ -68,17 +72,34 @@ public class SevenZOutputFile implements Closeable {
private Iterable<? extends SevenZMethodConfiguration> contentMethods =
Collections.singletonList(new SevenZMethodConfiguration(SevenZMethod.LZMA2));
private final Map<SevenZArchiveEntry, long[]> additionalSizes = new HashMap<>();
+ private byte[] password;
/**
* Opens file to write a 7z archive to.
*
* @param fileName the file to write to
+ * @param password optional password if the archive have to be encrypted
Review Comment:
s/if the archive have/if the archive has
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFile.java:
##########
@@ -93,8 +114,27 @@ public SevenZOutputFile(final File fileName) throws IOException {
* @since 1.13
*/
public SevenZOutputFile(final SeekableByteChannel channel) throws IOException {
+ this(channel, null);
+ }
+
+ /**
+ * Prepares channel to write a 7z archive to.
+ *
+ * <p>{@link
+ * org.apache.commons.compress.utils.SeekableInMemoryByteChannel}
+ * allows you to write to an in-memory archive.</p>
+ *
+ * @param channel the channel to write to
+ * @param password optional password if the archive have to be encrypted
+ * @throws IOException if the channel cannot be positioned properly
+ * @since 1.13
Review Comment:
s/1.13/1.23 (see `pom.xml` for the current snapshot, i.e. next release)
##########
src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFileTest.java:
##########
@@ -468,6 +473,40 @@ public void testArchiveWithMixedMethods() throws Exception {
}
}
+ /**
+ * Test password-baed encryption
+ *
+ * As AES/CBC Cipher requires a minimum of 16 bytes file data to be encrypted, some padding logic have been implemented.
Review Comment:
I think it needs a `<p>` wrapping it to be a separate paragraph.
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFile.java:
##########
@@ -93,8 +114,27 @@ public SevenZOutputFile(final File fileName) throws IOException {
* @since 1.13
*/
public SevenZOutputFile(final SeekableByteChannel channel) throws IOException {
+ this(channel, null);
+ }
+
+ /**
+ * Prepares channel to write a 7z archive to.
+ *
+ * <p>{@link
+ * org.apache.commons.compress.utils.SeekableInMemoryByteChannel}
+ * allows you to write to an in-memory archive.</p>
+ *
+ * @param channel the channel to write to
+ * @param password optional password if the archive have to be encrypted
Review Comment:
s/the archive have/the archive has
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java:
##########
@@ -2054,19 +2054,6 @@ public String getDefaultName() {
return lastSegment + "~";
}
- private static byte[] utf16Decode(final char[] chars) {
- if (chars == null) {
- return null;
- }
- final ByteBuffer encoded = UTF_16LE.encode(CharBuffer.wrap(chars));
- if (encoded.hasArray()) {
- return encoded.array();
- }
- final byte[] e = new byte[encoded.remaining()];
- encoded.get(e);
- return e;
- }
-
Review Comment:
:+1: I think it makes sense to move this method to `ByteUtils`, thanks!
##########
src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFileTest.java:
##########
@@ -468,6 +473,40 @@ public void testArchiveWithMixedMethods() throws Exception {
}
}
+ /**
+ * Test password-baed encryption
+ *
+ * As AES/CBC Cipher requires a minimum of 16 bytes file data to be encrypted, some padding logic have been implemented.
Review Comment:
And s/some padding logic have/some padding logic has
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org