Change StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION default to false in Jackson 2.16

[quinlam]

The INCLUDE_SOURCE_IN_LOCATION flag defaults to true, which means for improperly formatted input, the input will be logged as part of the stack trace. If the input contains PII data this can be a potential security vulnerability or violation of data handling standards for given services.

This issue/ticket is requesting that the flag be changed to default to false, and align with a "Secure by default" approach to the library. Given how widely this library is used it could provide a wide impacting improvement to the security of applications across the industry.

I acknowledge that this comes at the cost of;

• A breaking change for many users
• Increased difficulty in debugging

I believe this cost should be paid now in advance of some potential exploit in the future. While developers always have to option to set this flag to false themselves manually, grepping repositories and seeing how infrequently this flag is altered leads me to conclude that there is a lot of data out there inappropriately logged.

[pjfanning]

Not great timing. v2.15 is advanced in its release and very very late to add this. You have also not explained where the PII risk is. Is it logging? Jackson doesn't do much logging. If you log, Jackson exceptions, maybe it's your responsibility to filter out PII. You can't make every compliance issue the responsibility of external lib maintainers.

For more instance, can you explain how a 'location' can even expose PII?

[quinlam]

Not great timing. v2.15 is advanced in its release and very very late to add this

Discussing release timing is preemptive, lets first discuss the viability of the change. If it needs to be in a follow up release then so be it.

You have also not explained where the PII risk is. Is it logging? Jackson doesn't do much logging. If you log, Jackson exceptions, maybe it's your responsibility to filter out PII

Please read the description of the flag to understand. This will add the source to the exception message, which will be logged as part of the stack trace. It is very standard for stack traces to be logged. If the source was part of some custom field on the exception it would not be logged, and could instead be voluntarily logged, or inspected in debugger.

You can't make every compliance issue the responsibility of external lib maintainers

100% agree, but I think making this particular one a responsibility of the lib is the right call, which is why I am raising this ticket. You can see for yourself how infrequently this flag is set to false, when in reality you should expect a much higher incidence of its use.

Additionally "Secure by default" should be the position of all external lib maintainers. This is not about accommodating a security vulnerability, but by default preventing it.

[pjfanning]

@quinlam you have not provided any real info to justify your request. Please provide real examples.

[quinlam]

For more instance, can you explain how a 'location' can even expose PII?

Take the following code snippet/example

public SampleDataInput parseInput(final String input) {
    final ObjectMapper mapper = new ObjectMapper();

    try {
        return mapper.readValue(input, SampleDataInput.class);
    } catch (JsonProcessingException e) {
        // Exception can be caught and handled, but will typically added back to stack trace
        throw new RuntimeException("my custom exception message", e);
    }
}

This could be used to parse customer input, or parse stored serialized json input.

If I pass in the following string {"personalData":"myPrivateData"

The following will be part of the stack trace;

Caused by: com.fasterxml.jackson.core.io.JsonEOFException: Unexpected end-of-input: expected close marker for Object (start marker at [Source: (String)"{"personalData":"myPrivateData""; line: 1, column: 1])
 at [Source: (String)"{"personalData":"myPrivateData""; line: 1, column: 32]

This can result in PII data being exposed in logs. As a base case this exposes PII data to those who are intended to have log access (but should not have access to PII data), at the worst case this in conjunction with N other exploits could result in customer data exposed externally.

Many systems and services have stringent controls on logging PII data for this reason, this edge case can easily slip by teams.

[pjfanning]

@cowtowncoder any appetite to make this change in a v2.15.0-rc4? I sort of agree with the OP that the information that is given in the stacktrace by default is too much. Now that this has been raised publicly, we'll have people pressing for the change and even looking for CVEs or Sonatype issues until it is changed.

I've been looking for the number parsing changes out since September 2022 when we got the initial report of that parsing exploit - so I, more than anyone, would love to see v2.15 released. Unfortunately, we are now in an era where this sort of logging is regarded as a problem.

[cowtowncoder]

No, I don't think this should be changed at this point, and I don't plan on such a last minute change.

I am not yet 100% sure I agree with changes during 2.x -- Jackson itself logs nothing. Its caller that prints out stack trace, if anyone. But then again, I can see how inclusion of source (... for some types of input) can be problematic, given there is no mechanism for filtering.

I would be more open to changes in 3.x, but once again it's a bigger discussion: stack trace information is very important for developers, and it's up to them to decide where and how to disclose it.

I'll flag this as 2.16 at this point.

[quinlam]

I appreciate that this change would be difficult for a 2.15 delivery and I don't have visibility of the practicality of making such a last minute change and so have no intention to push here. I would still advocate for the change as part of 2.x and 2.16 specifically, and appreciate it being tagged as such so we can continue to evaluate the viability of the change.

Jackson itself logs nothing. Its caller that prints out stack trace, if anyone

This behavior is ubiquitous enough that I would consider "Jackson adds the source to the logs on formatting exceptions" to be a relatively truthful statement (ignoring semantics)

stack trace information is very important for developers, and it's up to them to decide where and how to disclose it.

I agree, but I believe they should opt into this, and not have to opt out. I see this as akin to DEBUG/TRACE logs. The developer must opt in and enable the extra verbosity.

[cowtowncoder]

Ok, fair enough. I think we should consider defaults for 2.16, with some discussion: possibly including verbiage in place of source document to indicate feature to enable to get snippet (to help devs that are now puzzled by sudden disappearance).

[dtpett]

Is there any way to change this configuration universally? We have several components where we have implemented custom filtration as needed, so in all other logging scenarios we want to have the SOURCE_IN_LOCATION available for debugging. From what I found in the docs the only possibility seems to be in the constructor: https://github.com/FasterXML/jackson-core/wiki/StreamReadFeatures

That is quite impractical when we have many places where we use many different constructors across several different applications.

[pjfanning]

Is there any way to change this configuration universally? We have several components where we have implemented custom filtration as needed, so in all other logging scenarios we want to have the SOURCE_IN_LOCATION available for debugging. From what I found in the docs the only possibility seems to be in the constructor: https://github.com/FasterXML/jackson-core/wiki/StreamReadFeatures

That is quite impractical when we have many places where we use many different constructors across several different applications.

Please don't comment on closed issues. Open a discussion (preferred) or a new issue instead.

[cowtowncoder]

Aside from comment by @pjfanning that I agree with, I might well answer here: no. Jackson is a component used by many frameworks, other libraries, and global defaulting is typically a bad idea for such cases. We have limited number of exceptions, but those do not include StreamReadFeatures.