Skip to content

Commit b72b777

Browse files
authored
Merge pull request #1552 from fl4via/2.2.x_backport_bug_fixes
[UNDERTOW-2309][UNDERTOW-2330][UNDERTOW-2331][UNDERTOW-2337][UNDERTOW-2342][UNDERTOW-2304][UNDERTOW-2338] CVE-2023-4639 Backport bug fixes
2 parents 77d7686 + 34724c4 commit b72b777

15 files changed

Lines changed: 239 additions & 35 deletions

File tree

‎core/src/main/java/io/undertow/protocols/ssl/SslConduit.java‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,9 @@ public void run() {
206206
this.sink = delegate.getSinkChannel().getConduit();
207207
this.source = delegate.getSourceChannel().getConduit();
208208
this.engine = engine;
209-
this.delegatedTaskExecutor = delegatedTaskExecutor;
209+
// if delegatedTaskExecutor is the same as delegate.getWorker, don't set it (see handshake task execution below,
210+
// we need to pick a thread that is different from the current thread running or run the handshake task immediately at the same thread)
211+
this.delegatedTaskExecutor = this.delegate.getWorker() != delegatedTaskExecutor? delegatedTaskExecutor : null;
210212
this.bufferPool = bufferPool;
211213
delegate.getSourceChannel().getConduit().setReadReadyHandler(readReadyHandler = new SslReadReadyHandler(null));
212214
delegate.getSinkChannel().getConduit().setWriteReadyHandler(writeReadyHandler = new SslWriteReadyHandler(null));
@@ -1168,7 +1170,16 @@ public void run() {
11681170
}
11691171
};
11701172
try {
1171-
getDelegatedTaskExecutor().execute(wrappedTask);
1173+
// we want to pick a thread different from the one we are running, to prevent starvation scenarios
1174+
// for example where we repeatedly attempt to write or read but are stuck waiting on a handshake task
1175+
// and the handshake task rarely has the chance to run because it needs the read/write thread to complete execution
1176+
// so, if io thread (read and write thread) is the same as current thread, and getDelegatedTaskExecutor will delegate
1177+
// execution to the same set of threads we are currently at, just run the handshake task right away to prevent blocking
1178+
if (delegate.getIoThread().equals(Thread.currentThread()) && delegatedTaskExecutor == null) {
1179+
wrappedTask.run();
1180+
} else {
1181+
getDelegatedTaskExecutor().execute(wrappedTask);
1182+
}
11721183
} catch (RejectedExecutionException e) {
11731184
UndertowLogger.REQUEST_IO_LOGGER.sslEngineDelegatedTaskRejected(e);
11741185
IoUtils.safeClose(connection);

‎core/src/main/java/io/undertow/server/DefaultByteBufferPool.java‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@
2626
import java.nio.ByteBuffer;
2727
import java.util.ArrayDeque;
2828
import java.util.ArrayList;
29+
import java.util.HashMap;
2930
import java.util.List;
31+
import java.util.Map;
3032
import java.util.concurrent.ConcurrentLinkedQueue;
3133
import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
3234

@@ -39,7 +41,7 @@
3941
*/
4042
public class DefaultByteBufferPool implements ByteBufferPool {
4143

42-
private final ThreadLocal<ThreadLocalData> threadLocalCache = new ThreadLocal<>();
44+
private final ThreadLocalCache threadLocalCache = new ThreadLocalCache();
4345
// Access requires synchronization on the threadLocalDataList instance
4446
private final List<WeakReference<ThreadLocalData>> threadLocalDataList = new ArrayList<>();
4547
private final ConcurrentLinkedQueue<ByteBuffer> queue = new ConcurrentLinkedQueue<>();
@@ -229,6 +231,7 @@ public void close() {
229231
local.buffers.clear();
230232
}
231233
ref.clear();
234+
threadLocalCache.remove(local);
232235
}
233236
threadLocalDataList.clear();
234237
}
@@ -327,4 +330,29 @@ protected void finalize() throws Throwable {
327330
}
328331
}
329332

333+
// This is used instead of Java ThreadLocal class. Unlike in the ThreadLocal class, the remove() method in this
334+
// class can be called by a different thread than the one that initialized the data.
335+
private static class ThreadLocalCache {
336+
337+
Map<Thread, ThreadLocalData> localsByThread = new HashMap<>();
338+
339+
ThreadLocalData get() {
340+
return localsByThread.get(Thread.currentThread());
341+
}
342+
343+
void set(ThreadLocalData threadLocalData) {
344+
localsByThread.put(Thread.currentThread(), threadLocalData);
345+
}
346+
347+
void remove(ThreadLocalData threadLocalData) {
348+
// Find the entry containing given data instance and remove it from the map.
349+
for (Map.Entry<Thread, ThreadLocalData> entry: localsByThread.entrySet()) {
350+
if (threadLocalData.equals(entry.getValue())) {
351+
localsByThread.remove(entry.getKey(), entry.getValue());
352+
break;
353+
}
354+
}
355+
}
356+
}
357+
330358
}

‎core/src/main/java/io/undertow/server/handlers/RequestDumpingHandler.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ private void dumpRequestBody(HttpServerExchange exchange, StringBuilder sb) {
164164
sb.append(formField)
165165
.append("=");
166166
for (FormData.FormValue formValue : formValues) {
167-
sb.append(formValue.isFileItem() ? "[file-content]" : formValue.getValue());
167+
sb.append(formValue.isFileItem() && !formValue.isBigField() ? "[file-content]" : formValue.getValue());
168168
sb.append("\n");
169169

170170
if (formValue.getHeaders() != null) {

‎core/src/main/java/io/undertow/server/handlers/form/FormData.java‎

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import java.util.Map;
3535

3636
import io.undertow.UndertowMessages;
37+
import io.undertow.util.FileUtils;
3738
import io.undertow.util.HeaderMap;
3839

3940
/**
@@ -102,11 +103,15 @@ public void add(String name, String value, String charset, final HeaderMap heade
102103
}
103104

104105
public void add(String name, Path value, String fileName, final HeaderMap headers) {
106+
add(name, value, fileName, headers, false, null);
107+
}
108+
109+
public void add(String name, Path value, String fileName, final HeaderMap headers, boolean bigField, String charset) {
105110
Deque<FormValue> values = this.values.get(name);
106111
if (values == null) {
107112
this.values.put(name, values = new ArrayDeque<>(1));
108113
}
109-
values.add(new FormValueImpl(value, fileName, headers));
114+
values.add(new FormValueImpl(value, fileName, headers, bigField, charset));
110115
if (values.size() > maxValues) {
111116
throw new RuntimeException(UndertowMessages.MESSAGES.tooManyParameters(maxValues));
112117
}
@@ -211,6 +216,16 @@ public interface FormValue {
211216
* @return The headers that were present in the multipart request, or null if this was not a multipart request
212217
*/
213218
HeaderMap getHeaders();
219+
220+
/**
221+
* @return true if size of the FormValue comes from a multipart request exceeds the fieldSizeThreshold of
222+
* {@link MultiPartParserDefinition} without filename specified.
223+
*
224+
* A big field is stored in disk, it is a file based FileItem.
225+
*
226+
* getValue() returns getCharset() decoded string from the file if it is true.
227+
*/
228+
boolean isBigField();
214229
}
215230

216231
public static class FileItem {
@@ -284,13 +299,15 @@ static class FormValueImpl implements FormValue {
284299
private final HeaderMap headers;
285300
private final FileItem fileItem;
286301
private final String charset;
302+
private final boolean bigField;
287303

288304
FormValueImpl(String value, HeaderMap headers) {
289305
this.value = value;
290306
this.headers = headers;
291307
this.fileName = null;
292308
this.fileItem = null;
293309
this.charset = null;
310+
this.bigField = false;
294311
}
295312

296313
FormValueImpl(String value, String charset, HeaderMap headers) {
@@ -299,14 +316,16 @@ static class FormValueImpl implements FormValue {
299316
this.headers = headers;
300317
this.fileName = null;
301318
this.fileItem = null;
319+
this.bigField = false;
302320
}
303321

304-
FormValueImpl(Path file, final String fileName, HeaderMap headers) {
322+
FormValueImpl(Path file, final String fileName, HeaderMap headers, boolean bigField, String charset) {
305323
this.fileItem = new FileItem(file);
306324
this.headers = headers;
307325
this.fileName = fileName;
308326
this.value = null;
309-
this.charset = null;
327+
this.charset = charset;
328+
this.bigField = bigField;
310329
}
311330

312331
FormValueImpl(byte[] data, String fileName, HeaderMap headers) {
@@ -315,12 +334,20 @@ static class FormValueImpl implements FormValue {
315334
this.headers = headers;
316335
this.value = null;
317336
this.charset = null;
337+
this.bigField = false;
318338
}
319339

320340

321341
@Override
322342
public String getValue() {
323343
if (value == null) {
344+
if (bigField) {
345+
try (InputStream inputStream = getFileItem().getInputStream()) {
346+
return FileUtils.readFile(inputStream, this.charset);
347+
} catch (IOException e) {
348+
// ignore
349+
}
350+
}
324351
throw UndertowMessages.MESSAGES.formValueIsAFile();
325352
}
326353
return value;
@@ -360,6 +387,10 @@ public FileItem getFileItem() {
360387
return fileItem;
361388
}
362389

390+
public boolean isBigField() {
391+
return bigField;
392+
}
393+
363394
@Override
364395
public boolean isFileItem() {
365396
return fileItem != null;

‎core/src/main/java/io/undertow/server/handlers/form/MultiPartParserDefinition.java‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -350,8 +350,7 @@ public void endPart() {
350350
if (fileName != null) {
351351
data.add(currentName, file, fileName, headers);
352352
} else {
353-
final Path fileNamePath = file.getFileName();
354-
data.add(currentName, file, fileNamePath != null ? fileNamePath.toString() : "", headers);
353+
data.add(currentName, file, null, headers, true, getCharset());
355354
}
356355
file = null;
357356
contentBytes.reset();
@@ -365,18 +364,8 @@ public void endPart() {
365364
data.add(currentName, Arrays.copyOf(contentBytes.toByteArray(), contentBytes.size()), fileName, headers);
366365
contentBytes.reset();
367366
} else {
368-
369-
370367
try {
371-
String charset = defaultEncoding;
372-
String contentType = headers.getFirst(Headers.CONTENT_TYPE);
373-
if (contentType != null) {
374-
String cs = Headers.extractQuotedValueFromHeader(contentType, "charset");
375-
if (cs != null) {
376-
charset = cs;
377-
}
378-
}
379-
368+
String charset = getCharset();
380369
data.add(currentName, new String(contentBytes.toByteArray(), charset), charset, headers);
381370
} catch (UnsupportedEncodingException e) {
382371
throw new RuntimeException(e);
@@ -385,6 +374,18 @@ public void endPart() {
385374
}
386375
}
387376

377+
private String getCharset() {
378+
String charset = defaultEncoding;
379+
String contentType = headers.getFirst(Headers.CONTENT_TYPE);
380+
if (contentType != null) {
381+
String cs = Headers.extractQuotedValueFromHeader(contentType, "charset");
382+
if (cs != null) {
383+
charset = cs;
384+
}
385+
}
386+
return charset;
387+
}
388+
388389
public List<Path> getCreatedFiles() {
389390
return createdFiles;
390391
}

‎core/src/main/java/io/undertow/server/protocol/http/HttpReadListener.java‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -139,12 +139,21 @@ public void handleEvent(final ConduitStreamSourceChannel channel) {
139139
public void handleEventWithNoRunningRequest(final ConduitStreamSourceChannel channel) {
140140
PooledByteBuffer existing = connection.getExtraBytes();
141141
if ((existing == null && connection.getOriginalSourceConduit().isReadShutdown()) || connection.getOriginalSinkConduit().isWriteShutdown()) {
142+
UndertowLogger.REQUEST_IO_LOGGER.debug("Connection is closing, cancelling handling of request");
143+
IoUtils.safeClose(connection);
144+
channel.suspendReads();
145+
return;
146+
}
147+
final PooledByteBuffer pooled;
148+
try {
149+
pooled = existing == null ? connection.getByteBufferPool().allocate() : existing;
150+
} catch (IllegalStateException e) {
151+
UndertowLogger.REQUEST_IO_LOGGER.debug("Connection is closing, cancelling handling of request", e);
152+
// shutdown started after previous if statement, so treat it like previous statement
142153
IoUtils.safeClose(connection);
143154
channel.suspendReads();
144155
return;
145156
}
146-
147-
final PooledByteBuffer pooled = existing == null ? connection.getByteBufferPool().allocate() : existing;
148157
final ByteBuffer buffer = pooled.getBuffer();
149158
boolean free = true;
150159

‎core/src/main/java/io/undertow/util/Cookies.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,9 @@ private static void parseCookie(final String cookie, final Set<Cookie> parsedCoo
316316
cookieCount = createCookie(name, containsEscapedQuotes ? unescapeDoubleQuotes(cookie.substring(start, i)) : cookie.substring(start, i), maxCookies, cookieCount, cookies, additional);
317317
state = 0;
318318
start = i + 1;
319+
} else if (c == ';' || (commaIsSeperator && c == ',')) {
320+
state = 0;
321+
start = i + 1;
319322
}
320323
// Skip the next double quote char '"' when it is escaped by backslash '\' (i.e. \") inside the quoted value
321324
if (c == '\\' && (i + 1 < cookie.length()) && cookie.charAt(i + 1) == '"') {

‎core/src/main/java/io/undertow/util/FileUtils.java‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,9 @@
2222
import java.io.IOException;
2323
import java.io.InputStream;
2424
import java.net.URL;
25+
import java.nio.charset.Charset;
2526
import java.nio.charset.StandardCharsets;
27+
import java.nio.charset.UnsupportedCharsetException;
2628
import java.nio.file.FileVisitResult;
2729
import java.nio.file.Files;
2830
import java.nio.file.Path;
@@ -51,16 +53,32 @@ public static String readFile(URL url) {
5153
}
5254
}
5355

56+
public static String readFile(InputStream file, String charset) {
57+
try {
58+
Charset charSet = charset != null ? Charset.forName(charset) : StandardCharsets.UTF_8;
59+
return readFile(file, charSet);
60+
} catch (UnsupportedCharsetException e) {
61+
throw new RuntimeException(e);
62+
}
63+
}
64+
5465
/**
5566
* Reads the {@link InputStream file} and converting it to {@link String} using UTF-8 encoding.
5667
*/
5768
public static String readFile(InputStream file) {
69+
return readFile(file, StandardCharsets.UTF_8);
70+
}
71+
72+
/**
73+
* Reads the {@link InputStream file} and converting it to {@link String} using <code>charSet</code> encoding.
74+
*/
75+
public static String readFile(InputStream file, Charset charSet) {
5876
try (BufferedInputStream stream = new BufferedInputStream(file)) {
5977
byte[] buff = new byte[1024];
6078
StringBuilder builder = new StringBuilder();
6179
int read;
6280
while ((read = stream.read(buff)) != -1) {
63-
builder.append(new String(buff, 0, read, StandardCharsets.UTF_8));
81+
builder.append(new String(buff, 0, read, charSet));
6482
}
6583
return builder.toString();
6684
} catch (IOException e) {

‎core/src/test/java/io/undertow/protocols/http2/RapidResetDDoSUnitTestCase.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ public void assertDoSRstFramesHandled(int totalNumberOfRequests, int rstStreamLi
243243

244244
// server sent go away before processing and responding client frames, sometimes this happens, depends on the order of threads
245245
// being executed
246-
if (responses.isEmpty()) {
246+
if (responses.size() < totalNumberOfRequests) {
247247
Assert.assertTrue(errorExpected);
248248
Assert.assertNotNull(exception);
249249
Assert.assertTrue(exception instanceof ClosedChannelException);

‎core/src/test/java/io/undertow/server/handlers/form/MultipartFormDataParserTestCase.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -370,8 +370,7 @@ public void testLargeContentWithoutFileNameWithSmallFileSizeThreshold() throws E
370370

371371
Assert.assertEquals("false", parsedResponse.get("in_memory"));
372372
Assert.assertEquals(DigestUtils.md5Hex(Files.newInputStream(file.toPath())), parsedResponse.get("hash"));
373-
Assert.assertTrue(parsedResponse.get("file_name").startsWith("undertow"));
374-
Assert.assertTrue(parsedResponse.get("file_name").endsWith("upload"));
373+
Assert.assertEquals(parsedResponse.get("file_name"), "null");
375374
} finally {
376375
if (!file.delete()) {
377376
file.deleteOnExit();

0 commit comments

Comments
 (0)