ARTEMIS-3593: Be defensive when reading data from ActiveMQBuffer and allocating memory.
#3862
Conversation
…emory. Or else, an adversary may handcraft the packet causing OOM situation for a running a JVM.
|
FAO: @andytaylor |
|
Otherwise look good: it's a good change but I would like to see a test and probably worth creating an exception without stack trace to save denial of service due to stack trace bombing |
artemis-core-client/src/main/java/org/apache/activemq/artemis/utils/XidPayloadException.java
Show resolved
Hide resolved
|
@franz1981 - CI tests have passed. Please let me know if my changes make sense. |
|
@franz1981, @clebertsuconic, @jbertram - I would like to follow-up on this PR. Could you kindly review my work and let me know what you think? My interest in this work is not leisured. We have experienced a security vulnerability in our product and I hope that the fix I am making will be included into the next version of Artemis Active MQ. |
artemis-core-client/src/main/java/org/apache/activemq/artemis/utils/XidCodecSupport.java
Show resolved
Hide resolved
|
@vkolomeyko what led you to do this? did you see this particularly happen anywhere? this needs a JIRA... and I would not use the Exception you created..I would just use ActiveMQAMQPIllegalStateException here. this is a way exceptional case.. this should not happen at all.. if this happens it seems a bug to me, hence I'm asking why you did this PR. |
vkolomeyko
changed the title
ActiveMQBuffer and allocating memory.ActiveMQBuffer and allocating memory.
|
@clebertsuconic - thanks for your input. I have raised a Jira: https://issues.apache.org/jira/browse/ARTEMIS-3593 explaining the reason for this change. |
|
@vkolomeyko Why you went the extra mile to remove the stack trace from the exception? I'm raising a new PR based on this one with some refactoring... I will ask your review. |
as per @franz1981 point up above "...worth creating an exception without stack trace to save denial of service due to stack trace bombing" |
|
It may make sense to whack the connection in this case, it is a protocol violation. 5.x would error out with an IOException and close the connection. It is sensible behaviour if the client is sending rogue or invalid data. A good citizen may even notice and stop. |
|
I agree with Gary.. if you actually throw any RuntimeException on the decode that will kill the connection @vkolomeyko let me handle this PR? I will do some tweaking to the PR, and I will set you as the author on the commit... (with a note of my collaboration) |
|
I am refactoring the safeRead into BytebufferUtil BTW. so we can reuse the method in other packets |
@clebertsuconic - sure. Please feel free to add me as a reviewer as soon as you have PR ready. |
|
PR #3871 replaces this one |
|
@vkolomeyko if you could review #3871 please? I am now closing this one. |
Or else, an adversary may handcraft the packet causing OOM situation for a running a JVM.