Skip to content

Commit

Permalink
Fix #739
Browse files Browse the repository at this point in the history
  • Loading branch information
cowtowncoder committed Jan 17, 2022
1 parent 143eece commit c5aa1e1
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 1 deletion.
4 changes: 4 additions & 0 deletions release-notes/CREDITS-2.x
Original file line number Diff line number Diff line change
Expand Up @@ -279,3 +279,7 @@ Andrey Somov (asomov@github)
* Contributed #732: Update Maven wrapper
(2.13.2)

Vlad Tatavu (vladt@github)
* Reported #739: `JsonLocation` in 2.13 only uses identity comparison
for "content reference"
(2.13.2)
2 changes: 2 additions & 0 deletions release-notes/VERSION-2.x
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ JSON library.

#732: Update Maven wrapper
(contributed by Andrey S)
#739: `JsonLocation` in 2.13 only uses identity comparison for "content reference"
(reported by Vlad T)

2.13.1 (19-Dec-2021)

Expand Down
5 changes: 5 additions & 0 deletions src/main/java/com/fasterxml/jackson/core/JsonLocation.java
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@
* Object that encapsulates Location information used for reporting
* parsing (or potentially generation) errors, as well as current location
* within input streams.
*<p>
* NOTE: users should be careful if using {@link #equals} implementation as
* it may or may not compare underlying "content reference" for equality.
* Instead if would make sense to explicitly implementing equality checks
* using specific criteria caller desires.
*/
public class JsonLocation
implements java.io.Serializable
Expand Down
20 changes: 20 additions & 0 deletions src/main/java/com/fasterxml/jackson/core/io/ContentReference.java
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
package com.fasterxml.jackson.core.io;

import java.io.File;
import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.net.URI;
import java.net.URL;
import java.nio.charset.Charset;
import java.util.Objects;

Expand Down Expand Up @@ -357,6 +360,23 @@ public boolean equals(Object other)
if (!(other instanceof ContentReference)) return false;
ContentReference otherSrc = (ContentReference) other;

// 16-Jan-2022, tatu: As per [core#739] we'll want to consider some
// but not all content cases with real equality: the concern here is
// to avoid expensive comparisons and/or possible security issues
final Object otherRaw = otherSrc._rawContent;

if (_rawContent == null) {
return (otherRaw == null);
} else if (otherRaw == null) {
return false;
}

if ((_rawContent instanceof File)

This comment has been minimized.

Copy link
@vladt

vladt Jan 18, 2022

The URL and URI classes are final, but File is not. In theory, this means an "attacker" could subclass from File and override the equals method, thus forcing execution of malicious code. I'm not sure if this is possible in practice, but I would check if the _rawcontent's class is File (not instanceof) to avoid any chance of this attack vector becoming possible.

This comment has been minimized.

Copy link
@cowtowncoder

cowtowncoder Jan 18, 2022

Author Member

Hmmh. Interesting. Thank you for suggesting this concern.

Right, I am not sure attacker would be in position to achieve that under most circumstances.
And then again there could be legit use cases where File subtypes should be accepted (meaning that if strict check was made, we'd fail on case we shouldn't).

I am tempted to leave the code as-is here. WDYT?

This comment has been minimized.

Copy link
@vladt

vladt Jan 18, 2022

I am tempted to leave the code as-is here. WDYT?

I don't know enough to have an opinion :) The changes in this commit should resolve the problem I encountered. Thank you.

This comment has been minimized.

Copy link
@cowtowncoder

cowtowncoder Jan 19, 2022

Author Member

Worth checking, still. :)

Thanks!

|| (_rawContent instanceof URL)
|| (_rawContent instanceof URI)
) {
return _rawContent.equals(otherRaw);
}
return _rawContent == otherSrc._rawContent;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
package com.fasterxml.jackson.core;

import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.InputStream;

import com.fasterxml.jackson.core.io.ContentReference;

public class TestLocation extends BaseTest
public class JsonLocationTest extends BaseTest
{
static class Foobar { }

Expand Down Expand Up @@ -121,6 +122,21 @@ public void testDisableSourceInclusion() throws Exception
p.close();
}

// for [jackson-core#739]: try to support equality
public void testLocationEquality() throws Exception
{
// important: create separate but equal instances
File src1 = new File("/tmp/foo");
File src2 = new File("/tmp/foo");
assertEquals(src1, src2);

JsonLocation loc1 = new JsonLocation(_sourceRef(src1),
10L, 10L, 1, 2);
JsonLocation loc2 = new JsonLocation(_sourceRef(src2),
10L, 10L, 1, 2);
assertEquals(loc1, loc2);
}

private ContentReference _sourceRef(String rawSrc) {
return ContentReference.construct(true, rawSrc, 0, rawSrc.length());
}
Expand All @@ -137,6 +153,10 @@ private ContentReference _sourceRef(InputStream rawSrc) {
return ContentReference.construct(true, rawSrc, -1, -1);
}

private ContentReference _sourceRef(File rawSrc) {
return ContentReference.construct(true, rawSrc, -1, -1);
}

private ContentReference _rawSourceRef(boolean textual, Object rawSrc) {
return ContentReference.rawReference(textual, rawSrc);
}
Expand Down

0 comments on commit c5aa1e1

Please sign in to comment.