This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat-connectors.git
The following commit(s) were added to refs/heads/main by this push:
new 97da2b17a Add additional bounds and error checking when reading AJP
messages.
97da2b17a is described below
commit 97da2b17a08dacfde785cf29c8fba07194e08997
Author: Mark Thomas <[email protected]>
AuthorDate: Wed Sep 6 15:22:31 2023 +0100
Add additional bounds and error checking when reading AJP messages.
schultz/markt
---
native/common/jk_ajp_common.c | 59 +++++++++++++++++++++++++++++++--------
native/common/jk_msg_buff.c | 15 ++++++++--
xdocs/miscellaneous/changelog.xml | 6 +++-
3 files changed, 65 insertions(+), 15 deletions(-)
diff --git a/native/common/jk_ajp_common.c b/native/common/jk_ajp_common.c
index bd6dc46f5..1cb2c6405 100644
--- a/native/common/jk_ajp_common.c
+++ b/native/common/jk_ajp_common.c
@@ -730,6 +730,12 @@ static int ajp_unmarshal_response(jk_msg_buf_t *msg,
JK_TRACE_EXIT(l);
return JK_FALSE;
}
+ if (d->status == 0xFFFF) {
+ jk_log(l, JK_LOG_ERROR,
+ "(%s) Not enough bytes available to read status",
ae->worker->name);
+ JK_TRACE_EXIT(l);
+ return JK_FALSE;
+ }
d->msg = jk_b_get_string(msg);
if (d->msg) {
@@ -743,6 +749,12 @@ static int ajp_unmarshal_response(jk_msg_buf_t *msg,
"(%s) status = %d", ae->worker->name, d->status);
d->num_headers = jk_b_get_int(msg);
+ if (d->num_headers == 0xFFFF) {
+ jk_log(l, JK_LOG_ERROR,
+ "(%s) Not enough bytes available to read header count",
ae->worker->name);
+ JK_TRACE_EXIT(l);
+ return JK_FALSE;
+ }
d->header_names = d->header_values = NULL;
if (JK_IS_DEBUG_LEVEL(l))
@@ -757,9 +769,15 @@ static int ajp_unmarshal_response(jk_msg_buf_t *msg,
if (d->header_names && d->header_values) {
unsigned int i;
for (i = 0; i < d->num_headers; i++) {
+ /*
+ * Looking for a specific value. No need to check for errors.
+ * That will be handled by jk_b_get_string if the specific
+ * value is not found.
+ */
unsigned short name = jk_b_pget_int(msg, msg->pos);
if ((name & 0XFF00) == 0XA000) {
+ /* Consume bytes just peeked with jk_b_pget_int */
jk_b_get_int(msg);
name = name & 0X00FF;
if (name <= SC_RES_HEADERS_NUM) {
@@ -2076,16 +2094,26 @@ static int ajp_process_callback(jk_msg_buf_t *msg,
return JK_AJP13_ERROR;
}
if (!s->response_blocked) {
- unsigned int len = (unsigned int)jk_b_get_int(msg);
+ unsigned int len;
+ /* Check there is sufficient data in the ajp message to read a
valid
+ * length. It must be at least 3 bytes - the magic byte for
+ * JK_AJP13_SEND_BODY_CHUNK (1 byte) and the length of the chunk
+ * (2 bytes). The remaining part of the message is the chunk.
+ */
+ if (msg->len < 3) {
+ jk_log(l, JK_LOG_ERROR,
+ "(%s) Invalid AJP message. Length of AJP message "
+ "is %d, but it should be at least 3.",
+ ae->worker->name, msg->len);
+ JK_TRACE_EXIT(l);
+ return JK_INTERNAL_ERROR;
+ }
/*
- * Do a sanity check on len to prevent write reading beyond buffer
- * boundaries and thus revealing possible sensitive memory
+ * Once we have len, do a sanity check to prevent reading beyond
+ * buffer boundaries and thus revealing possible sensitive memory
* contents to the client.
- * len cannot be larger than msg->len - 3 because the ajp message
- * contains the magic byte for JK_AJP13_SEND_BODY_CHUNK (1 byte)
- * and the length of the chunk (2 bytes). The remaining part of
- * the message is the chunk.
*/
+ len = (unsigned int)jk_b_get_int(msg);
if (len > (unsigned int)(msg->len - 3)) {
jk_log(l, JK_LOG_ERROR,
"(%s) Chunk length too large. Length of AJP message "
@@ -2126,9 +2154,12 @@ static int ajp_process_callback(jk_msg_buf_t *msg,
case JK_AJP13_GET_BODY_CHUNK:
{
int len = (int)jk_b_get_int(msg);
-
- if (len < 0) {
- len = 0;
+ if (len == 0xFFFF) {
+ jk_log(l, JK_LOG_ERROR,
+ "(%s) Invalid AJP message: Not enough bytes available
to read chunk length",
+ ae->worker->name);
+ JK_TRACE_EXIT(l);
+ return JK_AJP13_ERROR;
}
/* the right place to add file storage for upload
@@ -2150,7 +2181,13 @@ static int ajp_process_callback(jk_msg_buf_t *msg,
case JK_AJP13_END_RESPONSE:
ae->reuse = (int)jk_b_get_byte(msg);
- if (!ae->reuse) {
+ if (ae->reuse == 0xFF) {
+ jk_log(l, JK_LOG_ERROR,
+ "(%s) Not enough bytes available to read reuse flag",
+ ae->worker->name);
+ JK_TRACE_EXIT(l);
+ return JK_AJP13_ERROR;
+ } else if (!ae->reuse) {
/*
* AJP13 protocol reuse flag set to false.
* Tomcat will close its side of the connection.
diff --git a/native/common/jk_msg_buff.c b/native/common/jk_msg_buff.c
index c6b4a3c4e..731eb7005 100644
--- a/native/common/jk_msg_buff.c
+++ b/native/common/jk_msg_buff.c
@@ -215,7 +215,7 @@ int jk_b_append_bytes(jk_msg_buf_t *msg, const unsigned
char *param, int len)
unsigned long jk_b_get_long(jk_msg_buf_t *msg)
{
unsigned long i;
- if (msg->pos + 3 > msg->len) {
+ if (msg->pos + 4 > msg->len) {
return 0xFFFFFFFF;
}
i = ((msg->buf[(msg->pos++)] & 0xFF) << 24);
@@ -228,6 +228,9 @@ unsigned long jk_b_get_long(jk_msg_buf_t *msg)
unsigned long jk_b_pget_long(jk_msg_buf_t *msg, int pos)
{
unsigned long i;
+ if (pos + 4 > msg->len) {
+ return 0xFFFFFFFF;
+ }
i = ((msg->buf[(pos++)] & 0xFF) << 24);
i |= ((msg->buf[(pos++)] & 0xFF) << 16);
i |= ((msg->buf[(pos++)] & 0xFF) << 8);
@@ -239,7 +242,7 @@ unsigned long jk_b_pget_long(jk_msg_buf_t *msg, int pos)
unsigned short jk_b_get_int(jk_msg_buf_t *msg)
{
unsigned short i;
- if (msg->pos + 1 > msg->len) {
+ if (msg->pos + 2 > msg->len) {
return 0xFFFF;
}
i = ((msg->buf[(msg->pos++)] & 0xFF) << 8);
@@ -250,6 +253,9 @@ unsigned short jk_b_get_int(jk_msg_buf_t *msg)
unsigned short jk_b_pget_int(jk_msg_buf_t *msg, int pos)
{
unsigned short i;
+ if (pos + 2 > msg->len) {
+ return 0xFFFF;
+ }
i = ((msg->buf[pos++] & 0xFF) << 8);
i += ((msg->buf[pos] & 0xFF));
return i;
@@ -258,7 +264,7 @@ unsigned short jk_b_pget_int(jk_msg_buf_t *msg, int pos)
unsigned char jk_b_get_byte(jk_msg_buf_t *msg)
{
unsigned char rc;
- if (msg->pos > msg->len) {
+ if (msg->pos + 1 > msg->len) {
return 0xFF;
}
rc = msg->buf[msg->pos++];
@@ -268,6 +274,9 @@ unsigned char jk_b_get_byte(jk_msg_buf_t *msg)
unsigned char jk_b_pget_byte(jk_msg_buf_t *msg, int pos)
{
+ if (pos + 1 > msg->len) {
+ return 0xFF;
+ }
return msg->buf[pos];
}
diff --git a/xdocs/miscellaneous/changelog.xml
b/xdocs/miscellaneous/changelog.xml
index a99f7b050..62af3e643 100644
--- a/xdocs/miscellaneous/changelog.xml
+++ b/xdocs/miscellaneous/changelog.xml
@@ -100,7 +100,11 @@
Sam James. (markt)
</fix>
<update>
- Improve XSS hardening i status worker. (rjung)
+ Improve XSS hardening in status worker. (rjung)
+ </update>
+ <update>
+ Add additional bounds and error checking when reading AJP messages.
+ (schultz/markt)
</update>
</changelog>
</subsection>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
