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

[UNDERTOW-2305][UNDERTOW-2271][UNDERTOW-2072] CVE-2023-3223 Backport bug fixes #1523

Merged
merged 6 commits into from
Oct 11, 2023
Merged

[UNDERTOW-2305][UNDERTOW-2271][UNDERTOW-2072] CVE-2023-3223 Backport bug fixes #1523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
37fd60e
UNDERTOW-2305 Fix NPE in Http2ClearClientProvider
TomasHofman Sep 5, 2023
86209e5
[UNDERTOW-2271] CVE-2023-3223 Large uploaded file does not persist to…
gaol May 29, 2023
7094d86
[UNDERTOW-2271] Fix the findbugs error (NP_NULL_ON_SOME_PATH_FROM_RET…
fl4via Oct 11, 2023
104788f
[UNDERTOW-2072] DefaultByteBufferPool.getBuffer() may return null
baranowb Jun 10, 2022
b6a5df8
[UNDERTOW-2072] Add a few changes to the original fix: replace the At…
fl4via Sep 17, 2022
01fa3ef
[UNDERTOW-2316] Unify InMemorySessionManager getSession() method beha…
ropalka Oct 3, 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
2 changes: 1 addition & 1 deletion core/src/main/java/io/undertow/client/http2/Http2ClearClientProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ public static String createSettingsFrame(OptionMap options, ByteBufferPool buffe
if (options.contains(UndertowOptions.HTTP2_SETTINGS_MAX_HEADER_LIST_SIZE)) {
pushOption(currentBuffer, Http2Setting.SETTINGS_MAX_HEADER_LIST_SIZE, options.get(UndertowOptions.HTTP2_SETTINGS_MAX_HEADER_LIST_SIZE));
} else if(options.contains(UndertowOptions.MAX_HEADER_SIZE)) {
pushOption(currentBuffer, Http2Setting.SETTINGS_MAX_HEADER_LIST_SIZE, options.get(UndertowOptions.HTTP2_SETTINGS_MAX_HEADER_LIST_SIZE));
pushOption(currentBuffer, Http2Setting.SETTINGS_MAX_HEADER_LIST_SIZE, options.get(UndertowOptions.MAX_HEADER_SIZE));
}
currentBuffer.flip();
return FlexBase64.encodeStringURL(currentBuffer, false);
Expand Down
16 changes: 10 additions & 6 deletions core/src/main/java/io/undertow/server/DefaultByteBufferPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ public PooledByteBuffer allocate() {
buffer = queue.poll();
if (buffer != null) {
currentQueueLengthUpdater.decrementAndGet(this);
//buffer.clear();
}
}
if (buffer == null) {
Expand Down Expand Up @@ -256,20 +257,23 @@ private static class DefaultPooledBuffer implements PooledByteBuffer {

@Override
public ByteBuffer getBuffer() {
if(referenceCount == 0) {
final ByteBuffer tmp = this.buffer;
//UNDERTOW-2072
if (referenceCount == 0 || tmp == null) {
throw UndertowMessages.MESSAGES.bufferAlreadyFreed();
}
return buffer;
return tmp;
}

@Override
public void close() {
if(referenceCountUpdater.compareAndSet(this, 1, 0)) {
if(leakDetector != null) {
final ByteBuffer tmp = this.buffer;
if (referenceCountUpdater.compareAndSet(this, 1, 0)) {
this.buffer = null;
if (leakDetector != null) {
leakDetector.closed = true;
}
pool.freeInternal(buffer);
this.buffer = null;
pool.freeInternal(tmp);
}
}

Expand Down
127 changes: 70 additions & 57 deletions core/src/main/java/io/undertow/server/handlers/form/MultiPartParserDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.StandardOpenOption;
import java.nio.file.attribute.FileAttribute;
import java.util.ArrayList;
import java.util.Arrays;
Expand All @@ -61,7 +60,13 @@
*/
public class MultiPartParserDefinition implements FormParserFactory.ParserDefinition<MultiPartParserDefinition> {


public static final String MULTIPART_FORM_DATA = "multipart/form-data";
/**
* Proposed default MINSIZE as 16 KB for content in memory before persisting to disk if file content exceeds
* {@link #fileSizeThreshold} and the <i>filename</i> is not specified in the form.
*/
private static final long MINSIZE = Long.getLong("io.undertow.multipart.minsize", 0x4000);

private Executor executor;

Expand All @@ -73,6 +78,12 @@ public class MultiPartParserDefinition implements FormParserFactory.ParserDefini

private long fileSizeThreshold;

/**
* The threshold of form field size to persist to disk.
* It takes effect only for the form fields which do not have <i>filename</i> specified.
*/
private long fieldSizeThreshold = MINSIZE;

public MultiPartParserDefinition() {
tempFileLocation = Paths.get(System.getProperty("java.io.tmpdir"));
}
Expand All @@ -90,7 +101,7 @@ public FormDataParser create(final HttpServerExchange exchange) {
UndertowLogger.REQUEST_LOGGER.debugf("Could not find boundary in multipart request with ContentType: %s, multipart data will not be available", mimeType);
return null;
}
final MultiPartUploadHandler parser = new MultiPartUploadHandler(exchange, boundary, maxIndividualFileSize, fileSizeThreshold, defaultEncoding);
final MultiPartUploadHandler parser = new MultiPartUploadHandler(exchange, boundary, maxIndividualFileSize, fileSizeThreshold, defaultEncoding, fieldSizeThreshold);
exchange.addExchangeCompleteListener(new ExchangeCompletionListener() {
@Override
public void exchangeEvent(final HttpServerExchange exchange, final NextListener nextListener) {
Expand Down Expand Up @@ -119,6 +130,11 @@ public MultiPartParserDefinition setExecutor(final Executor executor) {
return this;
}

public MultiPartParserDefinition setFieldSizeThreshold(long fieldSizeThreshold) {
this.fieldSizeThreshold = fieldSizeThreshold;
return this;
}

public Path getTempFileLocation() {
return tempFileLocation;
}
Expand Down Expand Up @@ -167,13 +183,14 @@ private final class MultiPartUploadHandler implements FormDataParser, MultipartP
private HttpHandler handler;
private long currentFileSize;
private final MultipartParser.ParseState parser;
private final long fieldSizeThreshold;


private MultiPartUploadHandler(final HttpServerExchange exchange, final String boundary, final long maxIndividualFileSize, final long fileSizeThreshold, final String defaultEncoding) {
private MultiPartUploadHandler(final HttpServerExchange exchange, final String boundary, final long maxIndividualFileSize, final long fileSizeThreshold, final String defaultEncoding, final long fieldSizeThreshold) {
this.exchange = exchange;
this.maxIndividualFileSize = maxIndividualFileSize;
this.defaultEncoding = defaultEncoding;
this.fileSizeThreshold = fileSizeThreshold;
this.fieldSizeThreshold = fieldSizeThreshold;
this.data = new FormData(exchange.getConnection().getUndertowOptions().get(UndertowOptions.MAX_PARAMETERS, 1000));
String charset = defaultEncoding;
String contentType = exchange.getRequestHeaders().getFirst(Headers.CONTENT_TYPE);
Expand Down Expand Up @@ -251,56 +268,52 @@ public void beginPart(final HeaderMap headers) {
if (disposition.startsWith("form-data")) {
currentName = Headers.extractQuotedValueFromHeader(disposition, "name");
fileName = Headers.extractQuotedValueFromHeaderWithEncoding(disposition, "filename");
if (fileName != null && fileSizeThreshold == 0) {
try {
if (tempFileLocation != null) {
//Files impl is buggy, hence zero len
final FileAttribute[] emptyFA = new FileAttribute[] {};
final LinkOption[] emptyLO = new LinkOption[] {};
final Path normalized = tempFileLocation.normalize();
if (!Files.exists(normalized)) {
final int pathElementsCount = normalized.getNameCount();
Path tmp = normalized;
LinkedList<Path> dirsToGuard = new LinkedList<>();
for(int i=0;i<pathElementsCount;i++) {
if(Files.exists(tmp, emptyLO)) {
if(!Files.isDirectory(tmp, emptyLO)) {
//First existing element in path is a file,
//this will cause java.nio.file.FileSystemException
throw UndertowMessages.MESSAGES.pathElementIsRegularFile(tmp);
}
break;
} else {
dirsToGuard.addFirst(tmp);
tmp = tmp.getParent();
}
}
try {
Files.createDirectories(normalized, emptyFA);
} finally {
for (Path p : dirsToGuard) {
try {
p.toFile().deleteOnExit();
} catch (Exception e) {
break;
}
}
}
} else if (!Files.isDirectory(normalized, emptyLO)) {
throw new IOException(UndertowMessages.MESSAGES.pathNotADirectory(normalized));
}
file = Files.createTempFile(normalized, "undertow", "upload");
} else {
file = Files.createTempFile("undertow", "upload");
}
}
}

private Path createFile() throws IOException {
if (tempFileLocation != null) {
//Files impl is buggy, hence zero len
final FileAttribute[] emptyFA = new FileAttribute[] {};
final LinkOption[] emptyLO = new LinkOption[] {};
final Path normalized = tempFileLocation.normalize();
if (!Files.exists(normalized)) {
final int pathElementsCount = normalized.getNameCount();
Path tmp = normalized;
LinkedList<Path> dirsToGuard = new LinkedList<>();
for(int i=0;i<pathElementsCount;i++) {
if(Files.exists(tmp, emptyLO)) {
if(!Files.isDirectory(tmp, emptyLO)) {
//First existing element in path is a file,
//this will cause java.nio.file.FileSystemException
throw UndertowMessages.MESSAGES.pathElementIsRegularFile(tmp);
}
break;
} else {
dirsToGuard.addFirst(tmp);
tmp = tmp.getParent();
}
}
try {
Files.createDirectories(normalized, emptyFA);
} finally {
for (Path p : dirsToGuard) {
try {
p.toFile().deleteOnExit();
} catch (Exception e) {
break;
}
createdFiles.add(file);
fileChannel = FileChannel.open(file, StandardOpenOption.READ, StandardOpenOption.WRITE);
} catch (IOException e) {
throw new RuntimeException(e);
}
}
} else if (!Files.isDirectory(normalized, emptyLO)) {
throw new IOException(UndertowMessages.MESSAGES.pathNotADirectory(normalized));
}
file = Files.createTempFile(normalized, "undertow", "upload");
} else {
file = Files.createTempFile("undertow", "upload");
}
return file;
}

@Override
Expand All @@ -309,14 +322,9 @@ public void data(final ByteBuffer buffer) throws IOException {
if (this.maxIndividualFileSize > 0 && this.currentFileSize > this.maxIndividualFileSize) {
throw UndertowMessages.MESSAGES.maxFileSizeExceeded(this.maxIndividualFileSize);
}
if (file == null && fileName != null && fileSizeThreshold < this.currentFileSize) {
if (file == null && fileSizeThreshold < this.currentFileSize && (fileName != null || this.currentFileSize > fieldSizeThreshold)) {
try {
if (tempFileLocation != null) {
file = Files.createTempFile(tempFileLocation, "undertow", "upload");
} else {
file = Files.createTempFile("undertow", "upload");
}
createdFiles.add(file);
createdFiles.add(createFile());

FileOutputStream fileOutputStream = new FileOutputStream(file.toFile());
contentBytes.writeTo(fileOutputStream);
Expand All @@ -339,7 +347,12 @@ public void data(final ByteBuffer buffer) throws IOException {
@Override
public void endPart() {
if (file != null) {
data.add(currentName, file, fileName, headers);
if (fileName != null) {
data.add(currentName, file, fileName, headers);
} else {
final Path fileNamePath = file.getFileName();
data.add(currentName, file, fileNamePath != null ? fileNamePath.toString() : "", headers);
}
file = null;
contentBytes.reset();
try {
Expand Down
11 changes: 7 additions & 4 deletions core/src/main/java/io/undertow/server/session/InMemorySessionManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,15 +226,18 @@ private String createAndSaveNewID(SessionImpl session) {
public Session getSession(final HttpServerExchange serverExchange, final SessionConfig config) {
if (serverExchange != null) {
SessionImpl newSession = serverExchange.getAttachment(NEW_SESSION);
if(newSession != null) {
if (newSession != null) {
return newSession;
}
} else {
return null;
}

if (config == null) {
throw UndertowMessages.MESSAGES.couldNotFindSessionCookieConfig();
}

String sessionId = config.findSessionId(serverExchange);
InMemorySessionManager.SessionImpl session = (SessionImpl) getSession(sessionId);
if(session != null && serverExchange != null) {
if (session != null && serverExchange != null) {
session.requestStarted(serverExchange);
}
return session;
Expand Down
34 changes: 34 additions & 0 deletions core/src/test/java/io/undertow/server/handlers/form/MultipartFormDataParserTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,15 @@
import org.apache.commons.codec.digest.DigestUtils;
import org.apache.commons.io.Charsets;
import org.apache.commons.io.IOUtils;
import org.apache.http.HttpEntity;
import org.apache.http.HttpResponse;
import org.apache.http.client.methods.HttpPost;
import org.apache.http.entity.ContentType;
import org.apache.http.entity.StringEntity;
import org.apache.http.entity.mime.FormBodyPart;
import org.apache.http.entity.mime.HttpMultipartMode;
import org.apache.http.entity.mime.MultipartEntity;
import org.apache.http.entity.mime.MultipartEntityBuilder;
import org.apache.http.entity.mime.content.FileBody;
import org.apache.http.entity.mime.content.StringBody;
import org.junit.Assert;
Expand Down Expand Up @@ -345,6 +348,37 @@ public void testFileUploadWithMediumFileSizeThresholdAndLargeFile() throws Excep
}
}

@Test
public void testLargeContentWithoutFileNameWithSmallFileSizeThreshold() throws Exception {
DefaultServer.setRootHandler(new BlockingHandler(createInMemoryReadingHandler(10)));
File file = new File("tmp_upload_file.txt");
try (TestHttpClient client = new TestHttpClient()) {
file.createNewFile();
// 32 kb content, which exceeds default fieldSizeThreshold, the FormData.FormValue will be a FileItem
writeLargeFileContent(file, 0x4000 * 2);
HttpPost post = new HttpPost(DefaultServer.getDefaultServerURL() + "/path");
HttpEntity entity = MultipartEntityBuilder.create()
.addTextBody("formValue", "myValue", ContentType.TEXT_PLAIN)
.addBinaryBody("file", Files.newInputStream(file.toPath()))
.build();
post.setEntity(entity);
HttpResponse result = client.execute(post);
Assert.assertEquals(StatusCodes.OK, result.getStatusLine().getStatusCode());
String resp = HttpClientUtils.readResponse(result);

Map<String, String> parsedResponse = parse(resp);

Assert.assertEquals("false", parsedResponse.get("in_memory"));
Assert.assertEquals(DigestUtils.md5Hex(Files.newInputStream(file.toPath())), parsedResponse.get("hash"));
Assert.assertTrue(parsedResponse.get("file_name").startsWith("undertow"));
Assert.assertTrue(parsedResponse.get("file_name").endsWith("upload"));
} finally {
if (!file.delete()) {
file.deleteOnExit();
}
}
}

private void writeLargeFileContent(File file, int size) throws IOException {
int textLength = "content".getBytes().length;
FileOutputStream fos = new FileOutputStream(file);
Expand Down
Loading