Skip to content

Commit 45def54

Browse files
committed
Merge branch '2.19' into 2.20
2 parents 0e414cc + 4e6f3f5 commit 45def54

6 files changed

Lines changed: 283 additions & 3 deletions

File tree

release-notes/CREDITS-2.x

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,10 @@ Shanchao Li (@tonghuaroot)
370370
* Reported #696: (ion) Incomplete number length validation in Ion decoder
371371
(2.18.8)
372372

373+
@tinyb0y
374+
* Reported #726: (smile) Ensure `maxNameLength` limit enforced for Smile parser
375+
(2.18.10)
376+
373377
Steven Fackler (@sfackler)
374378
* Reported #300: (smile) Floats are encoded with sign extension while
375379
doubles without

release-notes/VERSION-2.x

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,8 @@ No changes since 2.19.1
101101
2.18.10 (not yet released)
102102

103103
#725: (cbor) Ensure `maxNameLength` limit enforced for CBOR parser
104+
#726: (smile) Ensure `maxNameLength` limit enforced for Smile parser
105+
(reported by @tinyb0y)
104106
#727: (cbor) `CBORParser.nextFieldName(SerializableString)` confuses 5-bit length
105107
marker 23 with 24 ("1-byte length suffix follows")
106108
#728: (cbor) `CBORParser.nextFieldName(SerializableString)` consumes Object entry

smile/src/main/java/com/fasterxml/jackson/dataformat/smile/SmileParser.java

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -668,6 +668,12 @@ public boolean nextFieldName(SerializableString str) throws IOException
668668

669669
byte[] nameBytes = str.asQuotedUTF8();
670670
final int byteLen = nameBytes.length;
671+
// NOTE: [dataformats-binary#726] `maxNameLength` deliberately NOT
672+
// enforced by the fast path below: length matched is that of the
673+
// name caller asked for, so it is not attacker-controlled. Note
674+
// too that the "long name" encoding (0x34) is not handled here at
675+
// all, but falls through to `nextToken()` -> `_handleFieldName()`,
676+
// which does enforce the limit
671677
// need room for type byte, name bytes, possibly end marker, so:
672678
if ((_inputPtr + byteLen + 1) < _inputEnd) { // maybe...
673679
int ptr = _inputPtr;
@@ -1483,6 +1489,14 @@ protected final JsonToken _handleFieldName() throws IOException
14831489
Integer.toHexString(_typeAsInt));
14841490
}
14851491

1492+
// NOTE: [dataformats-binary#726] `maxNameLength` NOT enforced for these
1493+
// shortest names: enforcement is approximate, effective minimum limit
1494+
// being 64 bytes here (57 for short Unicode names, and the length of the
1495+
// name caller asked for, for the fast path of
1496+
// `nextFieldName(SerializableString)`). Length is capped by the 6-bit
1497+
// length field of the type byte, so it is not attacker-controlled beyond
1498+
// that; longer names use the "long name" encoding, decoded by
1499+
// `_handleLongFieldName()`, which does enforce the limit
14861500
private String _findOrDecodeShortAsciiName(final int len) throws IOException
14871501
{
14881502
// First things first: must ensure all in buffer
@@ -1503,6 +1517,8 @@ private String _findOrDecodeShortAsciiName(final int len) throws IOException
15031517
return _decodeShortAsciiName(len);
15041518
}
15051519

1520+
// NOTE: [dataformats-binary#726] see `_findOrDecodeShortAsciiName()` for
1521+
// why `maxNameLength` is not enforced for short names
15061522
private String _findOrDecodeShortUnicodeName(final int len) throws IOException
15071523
{
15081524
// First things first: must ensure all in buffer
@@ -1798,20 +1814,24 @@ private final String _handleLongFieldName() throws IOException
17981814
}
17991815
q = (q << 8) | (b & 0xFF);
18001816
if (quads >= _quadBuffer.length) {
1801-
_quadBuffer = _growArrayTo(_quadBuffer, _quadBuffer.length + 256); // grow by 1k
1817+
_quadBuffer = _growNameDecodeBuffer(_quadBuffer, 256); // grow by 1k
18021818
}
18031819
_quadBuffer[quads++] = q;
18041820
}
18051821
// and if we have more bytes, append those too
18061822
int byteLen = (quads << 2);
18071823
if (bytes > 0) {
18081824
if (quads >= _quadBuffer.length) {
1809-
_quadBuffer = _growArrayTo(_quadBuffer, _quadBuffer.length + 256);
1825+
_quadBuffer = _growNameDecodeBuffer(_quadBuffer, 256);
18101826
}
18111827
q = _padLastQuad(q, bytes);
18121828
_quadBuffer[quads++] = q;
18131829
byteLen += bytes;
18141830
}
1831+
// [dataformats-binary#726]: verify name length before looking it up or
1832+
// decoding it: must be checked before symbol table lookup since a hit
1833+
// would otherwise bypass validation for all but the first occurrence
1834+
_streamReadConstraints.validateNameLength(byteLen);
18151835
// Know this name already?
18161836
String name = _symbolsCanonical ?
18171837
_symbols.findName(_quadBuffer, quads) : null;
@@ -1956,6 +1976,25 @@ private static int[] _growArrayTo(int[] arr, int minSize) {
19561976
return Arrays.copyOf(arr, size);
19571977
}
19581978

1979+
/**
1980+
* Helper method for expanding "quad" buffer used for decoding long Object
1981+
* property names: also verifies that name length does not exceed maximum
1982+
* allowed, so that an unbounded name cannot be buffered in full before
1983+
* being checked ([dataformats-binary#726]).
1984+
*<p>
1985+
* Enforcement here is incremental and hence approximate: check is made
1986+
* against the capacity of the buffer being grown, so a name may exceed
1987+
* {@code maxNameLength} by up to the growth increment before it is caught.
1988+
* The exact length is verified once the whole name has been read.
1989+
*
1990+
* @since 2.18.10
1991+
*/
1992+
private int[] _growNameDecodeBuffer(int[] arr, int more) throws IOException {
1993+
// the following check will fail if the array is already bigger than is allowed for names
1994+
_streamReadConstraints.validateNameLength(arr.length << 2);
1995+
return _growArrayTo(arr, arr.length + more);
1996+
}
1997+
19591998
// Helper methods needed to fix [dataformats-binary#312], masking of 0x00 character
19601999

19612000
private final static int _padLastQuad(int q, int bytes) {

smile/src/main/java/com/fasterxml/jackson/dataformat/smile/async/NonBlockingByteArrayParser.java

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -655,7 +655,12 @@ private final JsonToken _finishLongFieldName(int outPtr) throws IOException
655655
_inputCopyLen = outPtr;
656656
return _updateTokenToNA();
657657
}
658-
// otherwise increase copy buffer length
658+
// otherwise increase copy buffer length; but before that, verify
659+
// name length does not exceed maximum allowed, so that an unbounded
660+
// name cannot be buffered in full first ([dataformats-binary#726]).
661+
// Note that buffer is exactly full at this point, so `outPtr` is
662+
// the length of the name decoded so far.
663+
_streamReadConstraints.validateNameLength(outPtr);
659664
int oldLen = copyBuffer.length;
660665
int incr = Math.min(64000, oldLen >> 1);
661666
_inputCopy = copyBuffer = Arrays.copyOf(_inputCopy, oldLen + incr);
@@ -664,6 +669,9 @@ private final JsonToken _finishLongFieldName(int outPtr) throws IOException
664669

665670
// But if we get here, we got it all, only need to create quads etc
666671
_inputPtr = srcPtr;
672+
// [dataformats-binary#726]: check before symbol table lookup below, since
673+
// a hit would otherwise bypass validation for all but the first occurrence
674+
_streamReadConstraints.validateNameLength(outPtr);
667675
int[] quads = _quadBuffer;
668676
int qlen = (outPtr + 3) >> 2; // last quad may be partial
669677

Lines changed: 221 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,221 @@
1+
package com.fasterxml.jackson.dataformat.smile.constraints;
2+
3+
import java.io.ByteArrayInputStream;
4+
import java.io.ByteArrayOutputStream;
5+
6+
import com.fasterxml.jackson.core.JsonGenerator;
7+
import com.fasterxml.jackson.core.JsonParser;
8+
import com.fasterxml.jackson.core.JsonToken;
9+
import com.fasterxml.jackson.core.StreamReadConstraints;
10+
import com.fasterxml.jackson.core.exc.StreamConstraintsException;
11+
12+
import org.junit.jupiter.api.Test;
13+
14+
import com.fasterxml.jackson.dataformat.smile.SmileFactory;
15+
import com.fasterxml.jackson.dataformat.smile.async.AsyncReaderWrapper;
16+
import com.fasterxml.jackson.dataformat.smile.async.AsyncTestBase;
17+
18+
import static org.junit.jupiter.api.Assertions.*;
19+
20+
// [dataformats-binary#726]: `maxNameLength` was not enforced by Smile parsers
21+
public class LongNameSmileReadTest extends AsyncTestBase
22+
{
23+
private final static int MAX_NAME_LEN = 1000;
24+
25+
private final SmileFactory F_VANILLA = new SmileFactory();
26+
27+
private final SmileFactory F_CONSTRAINED = SmileFactory.builder()
28+
.streamReadConstraints(StreamReadConstraints.builder()
29+
.maxNameLength(MAX_NAME_LEN)
30+
.build())
31+
.build();
32+
33+
// Names of 64 bytes or less use the "short name" encodings, which are
34+
// length-bounded by the format itself; anything longer uses the "long
35+
// name" encoding, which was unbounded before the fix.
36+
37+
// Two separate checks guard the long-name path, and the sizes below are
38+
// chosen to exercise both:
39+
//
40+
// * "just over" fits within the already-allocated decode buffer, so it is
41+
// only caught by the exact check made once the whole name is known;
42+
// * the larger ones fill the buffer and are caught incrementally, while
43+
// it is being grown, before the whole name has been buffered.
44+
private final static int LEN_JUST_OVER = MAX_NAME_LEN + 20;
45+
private final static int LEN_OVER = MAX_NAME_LEN + 100;
46+
private final static int LEN_WAY_OVER = 400_000;
47+
48+
@Test
49+
public void testLongNameBlocking() throws Exception
50+
{
51+
for (boolean stream : new boolean[] { true, false }) {
52+
for (int len : new int[] { LEN_JUST_OVER, LEN_OVER, LEN_WAY_OVER }) {
53+
_verifyFails(_nameDoc(len), stream);
54+
}
55+
}
56+
}
57+
58+
@Test
59+
public void testLongNameAsync() throws Exception
60+
{
61+
// vary feed sizes to exercise both the single-chunk and the
62+
// split-across-feeds paths
63+
for (int bytesPerFeed : new int[] { 1, 7, 1000, 100_000 }) {
64+
for (int len : new int[] { LEN_JUST_OVER, LEN_OVER, LEN_WAY_OVER }) {
65+
_verifyFailsAsync(_nameDoc(len), bytesPerFeed);
66+
}
67+
}
68+
}
69+
70+
// Names at or below the limit must still be accepted
71+
@Test
72+
public void testNameWithinLimitBlocking() throws Exception
73+
{
74+
for (boolean stream : new boolean[] { true, false }) {
75+
for (int len : new int[] { 100, MAX_NAME_LEN }) {
76+
_verifyPasses(_nameDoc(len), _name(len), stream);
77+
}
78+
}
79+
}
80+
81+
@Test
82+
public void testNameWithinLimitAsync() throws Exception
83+
{
84+
for (int bytesPerFeed : new int[] { 1, 7, 1000, 100_000 }) {
85+
for (int len : new int[] { 100, MAX_NAME_LEN }) {
86+
_verifyPassesAsync(_nameDoc(len), _name(len), bytesPerFeed);
87+
}
88+
}
89+
}
90+
91+
// The checks made while the decode buffer is grown are what keep an
92+
// over-long name from being buffered -- and decoded -- in full before it
93+
// gets rejected. Whether parsing fails does not show this, since the final
94+
// check would catch the name either way; what shows it is the length the
95+
// failure reports, which is how much had been read when it gave up. For a
96+
// name this far over the limit that has to be a small fraction of the whole.
97+
@Test
98+
public void testLongNameRejectedBeforeBufferedInFull() throws Exception
99+
{
100+
final byte[] doc = _nameDoc(LEN_WAY_OVER);
101+
102+
for (boolean stream : new boolean[] { true, false }) {
103+
int reported = _verifyFails(doc, stream);
104+
assertTrue(reported < (LEN_WAY_OVER / 4),
105+
"Should have given up well before reading all "+LEN_WAY_OVER
106+
+" bytes of name, but reported length was "+reported);
107+
}
108+
for (int bytesPerFeed : new int[] { 1, 100_000 }) {
109+
int reported = _verifyFailsAsync(doc, bytesPerFeed);
110+
assertTrue(reported < (LEN_WAY_OVER / 4),
111+
"Should have given up well before reading all "+LEN_WAY_OVER
112+
+" bytes of name, but reported length was "+reported);
113+
}
114+
}
115+
116+
// The symbol table is per-factory and shared by all parsers it creates, so
117+
// a name decoded by one parser can be served to the next straight from the
118+
// table, without being decoded again. The length check therefore has to
119+
// happen before the lookup rather than during decoding -- otherwise only
120+
// the very first occurrence of a name would ever be checked.
121+
// Both directions matter: repeated legal names must keep working, and
122+
// repeated over-long ones must be rejected every time.
123+
@Test
124+
public void testRepeatedNamesViaSymbolTable() throws Exception
125+
{
126+
// both sizes fit the already-allocated buffer, so the lookup, and not
127+
// the incremental check, is what these have to get past
128+
final byte[] okDoc = _nameDoc(MAX_NAME_LEN);
129+
final byte[] badDoc = _nameDoc(LEN_JUST_OVER);
130+
131+
// repeated across parsers of the same factory: 2nd one is a cache hit
132+
for (int i = 0; i < 2; ++i) {
133+
_verifyPasses(okDoc, _name(MAX_NAME_LEN), true);
134+
_verifyFails(badDoc, true);
135+
}
136+
}
137+
138+
// @return Name length the failure reported
139+
private int _verifyFails(byte[] doc, boolean stream) throws Exception
140+
{
141+
try (JsonParser p = stream
142+
? F_CONSTRAINED.createParser(new ByteArrayInputStream(doc))
143+
: F_CONSTRAINED.createParser(doc, 0, doc.length)) {
144+
while (p.nextToken() != null) { }
145+
fail("expected StreamConstraintsException");
146+
return -1;
147+
} catch (StreamConstraintsException e) {
148+
return _verifyNameLengthException(e);
149+
}
150+
}
151+
152+
// @return Name length the failure reported
153+
private int _verifyFailsAsync(byte[] doc, int bytesPerFeed) throws Exception
154+
{
155+
AsyncReaderWrapper p = asyncForBytes(F_CONSTRAINED, bytesPerFeed, doc, 0);
156+
try {
157+
while (p.nextToken() != null) { }
158+
fail("expected StreamConstraintsException (bytesPerFeed: "+bytesPerFeed+")");
159+
return -1;
160+
} catch (StreamConstraintsException e) {
161+
return _verifyNameLengthException(e);
162+
}
163+
}
164+
165+
private int _verifyNameLengthException(StreamConstraintsException e)
166+
{
167+
final String msg = e.getMessage();
168+
assertTrue(msg.contains("Name length ("), "Unexpected message: "+msg);
169+
assertTrue(msg.contains("exceeds the maximum allowed ("+MAX_NAME_LEN),
170+
"Unexpected message: "+msg);
171+
int start = msg.indexOf('(') + 1;
172+
return Integer.parseInt(msg.substring(start, msg.indexOf(')', start)));
173+
}
174+
175+
private void _verifyPasses(byte[] doc, String expName, boolean stream) throws Exception
176+
{
177+
try (JsonParser p = stream
178+
? F_CONSTRAINED.createParser(new ByteArrayInputStream(doc))
179+
: F_CONSTRAINED.createParser(doc, 0, doc.length)) {
180+
assertToken(JsonToken.START_OBJECT, p.nextToken());
181+
assertToken(JsonToken.FIELD_NAME, p.nextToken());
182+
assertEquals(expName, p.currentName());
183+
assertToken(JsonToken.VALUE_STRING, p.nextToken());
184+
assertToken(JsonToken.END_OBJECT, p.nextToken());
185+
assertNull(p.nextToken());
186+
}
187+
}
188+
189+
private void _verifyPassesAsync(byte[] doc, String expName, int bytesPerFeed) throws Exception
190+
{
191+
AsyncReaderWrapper p = asyncForBytes(F_CONSTRAINED, bytesPerFeed, doc, 0);
192+
assertToken(JsonToken.START_OBJECT, p.nextToken());
193+
assertToken(JsonToken.FIELD_NAME, p.nextToken());
194+
assertEquals(expName, p.currentName());
195+
assertToken(JsonToken.VALUE_STRING, p.nextToken());
196+
assertToken(JsonToken.END_OBJECT, p.nextToken());
197+
assertNull(p.nextToken());
198+
}
199+
200+
private byte[] _nameDoc(int nameLen) throws Exception
201+
{
202+
ByteArrayOutputStream bytes = new ByteArrayOutputStream(nameLen + 100);
203+
try (JsonGenerator g = F_VANILLA.createGenerator(bytes)) {
204+
g.writeStartObject();
205+
g.writeFieldName(_name(nameLen));
206+
g.writeString("v");
207+
g.writeEndObject();
208+
}
209+
return bytes.toByteArray();
210+
}
211+
212+
// ASCII name, so byte length == character length
213+
private String _name(int len)
214+
{
215+
StringBuilder sb = new StringBuilder(len);
216+
for (int i = 0; i < len; ++i) {
217+
sb.append((char) ('a' + (i % 26)));
218+
}
219+
return sb.toString();
220+
}
221+
}

smile/src/test/java/com/fasterxml/jackson/dataformat/smile/parse/SymbolTableTest.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,14 @@ static class Point {
1919
public int x, y;
2020
}
2121

22+
// Note: names used below deliberately exceed the default `maxNameLength`
23+
// of 50,000 bytes (see [dataformats-binary#726]), since the point is to
24+
// exercise buffer growth for very long names; so raise the limit here.
2225
private final SmileMapper NO_CAN_MAPPER = SmileMapper.builder(SmileFactory.builder()
2326
.disable(JsonFactory.Feature.CANONICALIZE_FIELD_NAMES)
27+
.streamReadConstraints(StreamReadConstraints.builder()
28+
.maxNameLength(100_000)
29+
.build())
2430
.build())
2531
.build();
2632

0 commit comments

Comments
 (0)