[COMPRESS-633] Add encryption support for SevenZ #332

Dougniel · 2022-11-28T06:08:55Z

👉🏼 The purpose is to provide password based encryption for 7z compression in the same way of decryption already supported, so go forward of one know limitation

☝️In this way, I would like to submit my contribution based on the existing implementation of decryption and the C++ implementation of 7z

✅ I added one unit test
Implementation of password-based encryption for 7z compressor

Let me know what do you think about this implementation 👍

https://issues.apache.org/jira/browse/COMPRESS-633

src/main/java/org/apache/commons/compress/archivers/sevenz/AES256SHA256Decoder.java

codecov-commenter · 2022-11-28T06:33:50Z

Codecov Report

Merging #332 (f22b216) into master (346e8a4) will increase coverage by 0.03%.
The diff coverage is 83.92%.

@@             Coverage Diff              @@
##             master     #332      +/-   ##
============================================
+ Coverage     80.22%   80.25%   +0.03%     
- Complexity     6640     6657      +17     
============================================
  Files           341      342       +1     
  Lines         25184    25266      +82     
  Branches       4102     4112      +10     
============================================
+ Hits          20203    20278      +75     
- Misses         3402     3404       +2     
- Partials       1579     1584       +5

Impacted Files	Coverage Δ
...compress/archivers/sevenz/AES256SHA256Decoder.java	`75.96% <79.10%> (+6.15%)`	⬆️
...mmons/compress/archivers/sevenz/AES256Options.java	`83.33% <83.33%> (ø)`
.../commons/compress/archivers/sevenz/SevenZFile.java	`75.96% <100.00%> (+0.20%)`	⬆️
...ns/compress/archivers/sevenz/SevenZOutputFile.java	`97.46% <100.00%> (+0.51%)`	⬆️
...ommons/compress/archivers/sevenz/LZMA2Decoder.java	`63.15% <0.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Dougniel · 2022-11-29T07:26:05Z

I pushed an alternate implementation to throw out use of AES/CBC/PKCS5Padding raised by Code scanning
/ CodeQL as weak

kinow

Hi @Dougniel

Thanks a lot for your contribution.

I normally don't contribute much to Commons Compress, so I just did a quick review, and left some comments. But some of these are not necessarily must-haves (e.g. questions about storing password, random). Take a look at the other ones for misspellings, and javadocs 👍

I stopped before taking a look at AES256SHA256Decoder as I realized it would take a bit longer to understand the change there.

Cheers
-Bruno

src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFileTest.java

src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFile.java

src/main/java/org/apache/commons/compress/utils/ByteUtils.java

src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFile.java

src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java

src/main/java/org/apache/commons/compress/archivers/sevenz/AES256Options.java

kinow · 2022-12-01T19:23:37Z

src/main/java/org/apache/commons/compress/archivers/sevenz/AES256Options.java

+    int numCyclesPower = 19;
+
+    public AES256Options(byte[] password) {
+        this.password = password;


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)

I consider myself not enought skilled in security concerns, but I asked myself about that too when I saw this mechanism that clears password as soon as possible 🤔.

I have chosen to implement the encryption like decryption done in SevenZFile.java (that stores password too) for homogeneous reasons 🤷.

By the way, as the password is required to decrypt each file independently (the sevenZ archive is not entirely encrypted - we can access metadata like file names without password - only files are encrypted). I don't know how to manage that without storing it somewhere in memory - asking user to fulfill it for each file to compress/decompress ? ~~Even in this way it will be stored inside the Input/OutputStream used by the library user.~~ (Update: I checked through my debugger and I'm not so sure about that 😅)

👉🏼 What I could try is to store it in less places (I stored it in AES256Options as in SevenZOutputFile) and even try to initialise a Cipher early as it seems a secure place for it

src/main/java/org/apache/commons/compress/archivers/sevenz/AES256Options.java

kinow · 2022-12-01T19:25:22Z

src/main/java/org/apache/commons/compress/archivers/sevenz/AES256SHA256Decoder.java

+            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?)",


Unnecessary string concatenation?

Right, a stupid copy/paste that I made. Maybe it was done that way for formatting purposes

Why is a new Random instance allocated each time?

I rework the implementation and now I only use SecureRandom one time

As in Java 8 it's not clear that SecureRandom is thread safe or not, I didn't put In a static member (even if the JavaDocs 9 seems to tell that it could be ThreadSafe)

What do you think ?

kinow · 2022-12-01T19:28:37Z

(Also needs a JIRA issue for the changelog)

tcurdt · 2022-12-01T21:48:25Z

@kinow It seems like you have tagged the wrong person.

kinow · 2022-12-01T21:56:01Z

@kinow It seems like you have tagged the wrong person.

Oh, apologies @tcurdt . No idea why GitHub suggested your user. it's normally pretty clever to show the handle of the opener of the issue first at the list. Fixed!

Dougniel · 2022-12-01T22:48:42Z

(Also needs a JIRA issue for the changelog)

Hi @kinow,

Thank you for your rigorous and interesting review, I will correct these and respond

The JIRA in description is not right for the purpose of changelog ? (https://issues.apache.org/jira/browse/COMPRESS-633)

Cheers,
Daniel

src/main/java/org/apache/commons/compress/archivers/sevenz/AES256Options.java

garydgregory · 2022-11-30T14:41:18Z

src/main/java/org/apache/commons/compress/archivers/sevenz/AES256Options.java

+import java.util.Random;
+
+public class AES256Options {
+    byte[] password;


Instance variables should be private and final when possible.

src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFile.java

src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFileTest.java

garydgregory · 2022-12-01T23:07:44Z

src/main/java/org/apache/commons/compress/archivers/sevenz/AES256SHA256Decoder.java

+            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?)",


Why is a new Random instance allocated each time?

src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFileTest.java

garydgregory · 2022-12-02T02:41:45Z

src/main/java/org/apache/commons/compress/archivers/sevenz/AES256SHA256Decoder.java

+        try {
+            digest = MessageDigest.getInstance("SHA-256");
+        } catch (final NoSuchAlgorithmException noSuchAlgorithmException) {
+            throw new IOException("SHA-256 is unsupported by your Java implementation", noSuchAlgorithmException);


This is not possible since SHA-256 is required by the JRE specification, see https://docs.oracle.com/javase/8/docs/api/java/security/MessageDigest.html, therefore this could be an IllegalStateException.

src/main/java/org/apache/commons/compress/utils/ByteUtils.java

Dougniel · 2022-12-02T07:10:53Z

@kinow, @garydgregory,

Thanks for your reviews, I just pushed corrections that you suggested and I changed the place where Cipher is initialized in order to not store password in a clear way (following @kinow advice), let me know what do you think about that.

With these changes, the implementation between decryption and encryption is less homogeneous. In my opinion the decryption could be aligned in the encryption way for the same concern about password storage, but also to reduce the complexity of decode method (ex: replace coder and password parameters with options as done in encode method). But that could introduce a lot of changes that could be better held in another PR

Cheers,
Daniel

kinow · 2022-12-02T08:06:13Z

(Also needs a JIRA issue for the changelog)

Hi @kinow,

Thank you for your rigorous and interesting review, I will correct these and respond

The JIRA in description is not right for the purpose of changelog ? (https://issues.apache.org/jira/browse/COMPRESS-633)

Cheers, Daniel

Hi Daniel,\My bad, I hadn't seen that there was already a JIRA created for this issue. Thanks for updating the PR!

src/main/java/org/apache/commons/compress/archivers/sevenz/AES256SHA256Decoder.java

src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java

garydgregory · 2022-12-02T14:04:43Z

src/main/java/org/apache/commons/compress/utils/ByteUtils.java

@@ -266,4 +270,24 @@ private static void checkReadLength(final int length) {
            throw new IllegalArgumentException("Can't read more than eight bytes into a long value");
        }
    }
+


It looks like this method is only used from the sevenz package so let's keep it package-private there and avoid increasing the public API surface.

Right, I move it on AES256SHA256Decoder because it is used to convert password from char to byte for encryption/decryption

kinow

Just one comment about the VSCode files. But everything else looks OK to me. I would need to find some spare time now to sit down, read some of the surrounding API in Compress, and then really understand what's going on in order to do a proper review 👍 Thanks!

kinow · 2022-12-08T14:53:23Z

.vscode/settings.json

@@ -0,0 +1,3 @@
+{
+    "java.configuration.updateBuildConfiguration": "automatic"
+}


This file does not need to be added. I believe the repository does not include anything specific to Eclipse/IntelliJ/etc. If you use this file locally, you can add a global .gitignore to prevent Git from including it with your changes (or just ignore it when committing your changes).

Right, It was a mistake 👍

Dougniel · 2022-12-08T21:15:17Z

It seems that there is an issue with actions/setup-java : actions/setup-java#422

garydgregory · 2022-12-08T21:35:10Z

Just rebase on git master to test with the latest, that will help us get closer ;-)

Implementation of password-based encryption for 7z compressor COMPRESS-633

As `AES/CBC/PKCS5Padding` is raised as weak of security, a manual implementation to fill cither block size is done COMPRESS-633

- implementation without storing password in a clear way - several corrections suggeested by reviewers COMPRESS-633

typo COMPRESS-633

Avoid incrasing the public API surface with uneccessary method COMPRESS-633

no IDE specifi config files COMPRESS-633

garydgregory · 2022-12-09T14:48:14Z

src/main/java/org/apache/commons/compress/archivers/sevenz/AES256Options.java

+    private final int numCyclesPower;
+    private final Cipher cipher;
+
+    /**


Please complete the Javadoc comments. You need a starting sentence.

garydgregory · 2022-12-10T15:57:20Z

@Dougniel
Merged. Thank you for your work and patience. Please verify.

Dougniel · 2022-12-11T22:05:00Z

@Dougniel
Merged. Thank you for your work and patience. Please verify.

Thank you for your rigor, all is ok 👍

I noticed that you switched AES256Options to package-private, I hesitated because, as public, it offers more possibilities to advanced uses (per file Initialization Vector, per file password.. when setting it at SevenZArchiveEntry level). But this increases the public API surface and is not documented

garydgregory · 2022-12-12T00:22:28Z

@Dougniel
Right. We can always document it and make it public later. But also then make sure the API is right.

Dougniel · 2022-12-19T23:57:14Z

Hi @garydgregory, do you have an idea of next release date ?

garydgregory · 2022-12-20T11:32:01Z

Nope, we have other components to work through and Commons Net just had a release.

Dougniel mentioned this pull request Nov 28, 2022

🤐 Compression / décompression LZMA avec mot de passe opt-nc/j7zip#1

Closed

5 tasks

github-advanced-security bot found potential problems Nov 28, 2022

View reviewed changes

src/main/java/org/apache/commons/compress/archivers/sevenz/AES256SHA256Decoder.java Fixed Show fixed Hide fixed

Dougniel changed the title ~~feat: Encyrption support for Seven7~~ feat: Encryption support for Seven7 Nov 28, 2022

garydgregory changed the title ~~feat: Encryption support for Seven7~~ Add encryption support for Seven7 Nov 30, 2022

garydgregory changed the title ~~Add encryption support for Seven7~~ Add encryption support for SevenZ Nov 30, 2022

kinow reviewed Dec 1, 2022

View reviewed changes

garydgregory reviewed Dec 2, 2022

View reviewed changes

src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZOutputFileTest.java Outdated Show resolved Hide resolved

garydgregory reviewed Dec 2, 2022

View reviewed changes

src/main/java/org/apache/commons/compress/utils/ByteUtils.java Outdated Show resolved Hide resolved

kinow changed the title ~~Add encryption support for SevenZ~~ [COMPRESS-633] Add encryption support for SevenZ Dec 2, 2022

garydgregory reviewed Dec 2, 2022

View reviewed changes

src/main/java/org/apache/commons/compress/archivers/sevenz/AES256SHA256Decoder.java Outdated Show resolved Hide resolved

garydgregory reviewed Dec 2, 2022

View reviewed changes

src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java Outdated Show resolved Hide resolved

garydgregory reviewed Dec 2, 2022

View reviewed changes

Dougniel requested review from garydgregory and kinow and removed request for garydgregory December 7, 2022 23:37

kinow requested changes Dec 8, 2022

View reviewed changes

Dougniel added 2 commits December 9, 2022 14:02

feat: Encyrption support for Seven7

bb3e29a

Implementation of password-based encryption for 7z compressor COMPRESS-633

feat: Encyrption support for Seven7 without AES/CBC/PKCS5Padding

e857bda

As `AES/CBC/PKCS5Padding` is raised as weak of security, a manual implementation to fill cither block size is done COMPRESS-633

Dougniel added 4 commits December 9, 2022 14:05

feat: Encyrption support for SevenZ

92c4db1

- implementation without storing password in a clear way - several corrections suggeested by reviewers COMPRESS-633

feat: Encyrption support for SevenZ

0f36b34

typo COMPRESS-633

feat: Encyrption support for SevenZ

2debc4e

Avoid incrasing the public API surface with uneccessary method COMPRESS-633

feat: Encyrption support for SevenZ

c2d1d1d

no IDE specifi config files COMPRESS-633

Dougniel force-pushed the seven-z-password-encryption-support branch from 03eaf41 to c2d1d1d Compare December 9, 2022 03:05

Fix spelling

c7dafb3

garydgregory reviewed Dec 9, 2022

View reviewed changes

garydgregory added 4 commits December 10, 2022 10:05

Update super class from master

a577c1a

Merge branch 'master' into seven-z-password-encryption-support

51aca67

AES256Options does not need to be public

26dc4af

Fix spelling in Javadoc

f22b216

garydgregory merged commit f0d13f9 into apache:master Dec 10, 2022

asfgit pushed a commit that referenced this pull request Dec 10, 2022

[COMPRESS-633] Add encryption support for SevenZ #332

bdce6c6

Dougniel deleted the seven-z-password-encryption-support branch December 11, 2022 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[COMPRESS-633] Add encryption support for SevenZ #332

[COMPRESS-633] Add encryption support for SevenZ #332

Dougniel commented Nov 28, 2022

codecov-commenter commented Nov 28, 2022 •

edited

Loading

Dougniel commented Nov 29, 2022 •

edited

Loading

kinow left a comment •

edited

Loading

kinow Dec 1, 2022

Dougniel Dec 2, 2022 •

edited

Loading

kinow Dec 1, 2022

Dougniel Dec 1, 2022

garydgregory Dec 1, 2022 •

edited

Loading

Dougniel Dec 2, 2022

kinow commented Dec 1, 2022

tcurdt commented Dec 1, 2022

kinow commented Dec 1, 2022

Dougniel commented Dec 1, 2022

garydgregory Nov 30, 2022

garydgregory Dec 1, 2022 •

edited

Loading

garydgregory Dec 2, 2022

Dougniel commented Dec 2, 2022 •

edited

Loading

kinow commented Dec 2, 2022

garydgregory Dec 2, 2022

Dougniel Dec 3, 2022

kinow left a comment

kinow Dec 8, 2022

Dougniel Dec 8, 2022 •

edited

Loading

Dougniel commented Dec 8, 2022

garydgregory commented Dec 8, 2022

garydgregory Dec 9, 2022

garydgregory commented Dec 10, 2022 •

edited

Loading

Dougniel commented Dec 11, 2022

garydgregory commented Dec 12, 2022

Dougniel commented Dec 19, 2022 •

edited

Loading

garydgregory commented Dec 20, 2022

[COMPRESS-633] Add encryption support for SevenZ #332

[COMPRESS-633] Add encryption support for SevenZ #332

Conversation

Dougniel commented Nov 28, 2022

codecov-commenter commented Nov 28, 2022 • edited Loading

Codecov Report

Dougniel commented Nov 29, 2022 • edited Loading

kinow left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dougniel Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garydgregory Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinow commented Dec 1, 2022

tcurdt commented Dec 1, 2022

kinow commented Dec 1, 2022

Dougniel commented Dec 1, 2022

Choose a reason for hiding this comment

garydgregory Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dougniel commented Dec 2, 2022 • edited Loading

kinow commented Dec 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dougniel Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

Dougniel commented Dec 8, 2022

garydgregory commented Dec 8, 2022

Choose a reason for hiding this comment

garydgregory commented Dec 10, 2022 • edited Loading

Dougniel commented Dec 11, 2022

garydgregory commented Dec 12, 2022

Dougniel commented Dec 19, 2022 • edited Loading

garydgregory commented Dec 20, 2022

codecov-commenter commented Nov 28, 2022 •

edited

Loading

Dougniel commented Nov 29, 2022 •

edited

Loading

kinow left a comment •

edited

Loading

Dougniel Dec 2, 2022 •

edited

Loading

garydgregory Dec 1, 2022 •

edited

Loading

garydgregory Dec 1, 2022 •

edited

Loading

Dougniel commented Dec 2, 2022 •

edited

Loading

Dougniel Dec 8, 2022 •

edited

Loading

garydgregory commented Dec 10, 2022 •

edited

Loading

Dougniel commented Dec 19, 2022 •

edited

Loading