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.

Sign up for GitHub

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

Jump to bottom

backport CVE-2023-6378 #747

Closed
wants to merge 3 commits into from
Closed

backport CVE-2023-6378 #747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7674f18
prevent DOS attacks using on malicious serialized input
ceki Nov 27, 2023
fbbe64d
cater for array size marked with -1
ceki Nov 27, 2023
f022bfc
ensure JDK 8 compatibility
ceki Nov 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions logback-classic/src/main/java/ch/qos/logback/classic/spi/LoggingEventVO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package ch.qos.logback.classic.spi;

import java.io.IOException;
import java.io.InvalidObjectException;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.Serializable;
Expand All @@ -25,6 +26,7 @@
import ch.qos.logback.classic.Level;

// http://www.riehle.org/computer-science/research/1998/ubilab-tr-1998-10-1.html
// See also the paper https://www.riehle.org/computer-science/research/1998/ubilab-tr-1998-10-1.pdf

/**
* A read-only and serializable implementation of {@link ILoggingEvent}.
Expand All @@ -38,6 +40,7 @@ public class LoggingEventVO implements ILoggingEvent, Serializable {

private static final int NULL_ARGUMENT_ARRAY = -1;
private static final String NULL_ARGUMENT_ARRAY_ELEMENT = "NULL_ARGUMENT_ARRAY_ELEMENT";
private static final int ARGUMENT_ARRAY_DESERIALIZATION_LIMIT = 128;

private String threadName;
private String loggerName;
Expand Down Expand Up @@ -181,6 +184,12 @@ private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundE
level = Level.toLevel(levelInt);

int argArrayLen = in.readInt();

// Prevent DOS attacks via large or negative arrays
if (argArrayLen < NULL_ARGUMENT_ARRAY || argArrayLen > ARGUMENT_ARRAY_DESERIALIZATION_LIMIT) {
throw new InvalidObjectException("Argument array length is invalid: " + argArrayLen);
}

if (argArrayLen != NULL_ARGUMENT_ARRAY) {
argumentArray = new String[argArrayLen];
for (int i = 0; i < argArrayLen; i++) {
Expand Down
16 changes: 9 additions & 7 deletions logback-core/src/main/java/ch/qos/logback/core/net/HardenedObjectInputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link

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.

final private static String[] JAVA_PACKAGES = new String[] { "java.lang", "java.util" };
final private static int DEPTH_LIMIT = 16;
final private static int ARRAY_LIMIT = 10000;
Comment on lines +25 to +26
Copy link

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 •
edited
Loading

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


public HardenedObjectInputStream(InputStream in, String[] whilelist) throws IOException {
public HardenedObjectInputStream(InputStream in, String[] whitelist) throws IOException {
super(in);

this.whitelistedClassNames = new ArrayList<String>();
if (whilelist != null) {
for (int i = 0; i < whilelist.length; i++) {
this.whitelistedClassNames.add(whilelist[i]);
if (whitelist != null) {
for (int i = 0; i < whitelist.length; i++) {
this.whitelistedClassNames.add(whitelist[i]);
}
}
}


public HardenedObjectInputStream(InputStream in, List<String> whitelist) throws IOException {
super(in);

Expand Down