Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backport CVE-2023-6378 #747

Closed
wants to merge 3 commits into from
Closed

Conversation

bvahdat
Copy link

@bvahdat bvahdat commented Dec 1, 2023

With JDK8 all tests pass through mvn test:

image

Signed-off-by: Ceki Gulcu <ceki@qos.ch>
Signed-off-by: Ceki Gulcu <ceki@qos.ch>
Signed-off-by: Ceki Gulcu <ceki@qos.ch>
@bvahdat bvahdat changed the title CVE 2023 6378 CVE-2023-6378 Dec 1, 2023
@bvahdat bvahdat changed the title CVE-2023-6378 backport CVE-2023-6378 Dec 1, 2023
Comment on lines +25 to +26
final private static int DEPTH_LIMIT = 16;
final private static int ARRAY_LIMIT = 10000;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you forgot to use those. Did you have a merge conflict when cherry-picking 9c782b4 and 2cd8cab ?

Copy link
Author

@bvahdat bvahdat Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you forgot to use those. Did you have a merge conflict when cherry-picking 9c782b4 and 2cd8cab ?

Just for the record, although the PR has been already closed.

No I had no merge conflict for them. The fix of this CVE has been first announced for 1.13.12:

https://logback.qos.ch/news.html#1.3.12

Which is also reflected by the GitHub Advisory board accordingly:

GHSA-vmq6-5m68-f53m

So that I only cherry-picked the green commits below (from branch_1.3.x), but apparently there was more to it (the red commit by the 1.13.14 release) which you're asking about.

But apparently the fix had even much more to it than just cherry-picking as we see in bb09515.

Screenshot 2023-12-04 at 08 48 54

@@ -20,20 +20,22 @@
*/
public class HardenedObjectInputStream extends ObjectInputStream {

final List<String> whitelistedClassNames;
final static String[] JAVA_PACKAGES = new String[] { "java.lang", "java.util" };
final private List<String> whitelistedClassNames;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are currently unused, you're missing

    private void initObjectFilter() {
        this.setObjectInputFilter(ObjectInputFilter.Config.createFilter(
                "maxarray=" + ARRAY_LIMIT + ";maxdepth=" + DEPTH_LIMIT + ";"
        ));
    }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are currently unused, you're missing

    private void initObjectFilter() {
        this.setObjectInputFilter(ObjectInputFilter.Config.createFilter(
                "maxarray=" + ARRAY_LIMIT + ";maxdepth=" + DEPTH_LIMIT + ";"
        ));
    }

Please see my answer above.

@ceki
Copy link
Member

ceki commented Dec 1, 2023

Please see if commit bb09515 works for you

@bvahdat
Copy link
Author

bvahdat commented Dec 1, 2023

Please see if commit bb09515 works for you

Thanks, then I simply close this PR as you're already on it. Looking forward to the upcoming 1.2.13 release

@bvahdat bvahdat closed this Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants