[UNDERTOW-2451] fix the information leakage issues #1668
Conversation
AJP Parser: Do not share the decodeBuffer StringBuilder instance between responses
fix the message leak issue in HPACK decoder
baranowb
added
bug fix
|
@BFionaccept could you please update commit messages? Add [UNDERTOW-2451] as prefix and reword first message, since that CVE is very specific and does not cover class you make change to. |
| @@ -260,12 +258,12 @@ private String readHpackString(ByteBuffer buffer) throws HpackException { | |||
| } | |||
|
|
|||
| private String readHuffmanString(int length, ByteBuffer buffer) throws HpackException { | |||
| StringBuilder stringBuilder = new StringBuilder(length); | |||
There was a problem hiding this comment.
In this fix, I referenced the patch for a similar vulnerability (CVE-2020-17527) in the corresponding functional module of Tomcat, as shown in this commit: apache/tomcat@d56293f#diff-98723542790b05d2e3c2dadeef7c632e641e3118b3b809660e9b1ef41686cfe0R226. I removed the final from the StringBuilder stringBuilder declaration, enhancing code flexibility and allowing for future reassignment if needed.
Certainly, I think it is also acceptable to retain the final modifier.
There was a problem hiding this comment.
Oh, I’m sorry, I accidentally deleted a previous conversation.
baranowb
changed the title
|
@baranowb Hi baranowb, I'm unable to view this issue: https://issues.redhat.com/browse/UNDERTOW-2451. Could you please help add me to it? |
@BFionaccept I have opened the Jira. You can try again. |
| /** | ||
| * The current string being read | ||
| */ | ||
| public StringBuilder currentString; |
There was a problem hiding this comment.
You can't remove currentString here. The parser is invoked multiple times as bytes are made available and, when the last byte is read, it wil need the whole currentString to form a parsed result, it will neeed information from previous parse invocations. Otherwise, you risk losing bytes. It all depends on how many bytes the parser has received on every invocation.
| @@ -73,8 +73,6 @@ public class HpackDecoder { | |||
|
|
|||
| private boolean first = true; | |||
|
|
|||
| private final StringBuilder stringBuilder = new StringBuilder(); | |||
There was a problem hiding this comment.
There is no leakage in this class either. In this case, we are reusing the same builder across multiple decode invocations, but we are clearing the buffer right after we use it on every call. It just prevents us from having to create a new builder everytime we are invoked, which is a good thing.
There was a problem hiding this comment.
The root cause for this leak is very similar to the vulnerability CVE-2020-17527 in Tomcat. I added additional details at https://issues.redhat.com/browse/UNDERTOW-2451.
fl4via
added
invalid
|
My understanding from https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2024-4109 is that this issue has assigned CVE-2024-4109 ? |
|
@fl4via , sorry for pinging you here, but I did not find any relevant thread to ask about CVE-2024-4109 |
|
I had the same questions as @carnil and @romabaz if this is the fix for CVE-2024-4109 (or, based on exchange between @fl4via and @BFionaccept -- it seems that this CVE may have been created in error?), the undertow team may be interested to know that -- as of today -- this is still listed as a vulnerability in many CVE databases and CVE scanning tools for both undertow 2.1.37 and 2.1.38 e.g.:
Both versions still have 2024-4109 listed as direct vulnerabilities |
|
Hi! @carnil @romabaz @miklish @BFionaccept The fix for CVE-2024-4109 is in #1715 . We will be doing a release of that within the next few days. |
Issue: https://issues.redhat.com/browse/UNDERTOW-2451
fix the information leakage issues